cleanup locking, add named mutex and make cache atomically

accounts/pkg/service/v0/accounts.go

@@ -11,7 +11,6 @@ import (
 	"path"
 	"regexp"
 	"strconv"
-	"sync"
 	"time"
 
 	"github.com/owncloud/ocis/ocis-pkg/log"
@@ -33,11 +32,8 @@ import (
 	"google.golang.org/protobuf/types/known/timestamppb"
 )
 
-// accLock mutually exclude readers from writers on account files
-var accLock sync.Mutex
-
 // passwordValidCache caches basic auth password validations
-var passwordValidCache = cache.NewCache(cache.Size(1024))
+var passwordValidCache = cache.NewCache(1024)
 
 // passwordValidCacheExpiration defines the entry lifetime
 const passwordValidCacheExpiration = 10 * time.Minute
@@ -133,9 +129,6 @@ func (s Service) ListAccounts(ctx context.Context, in *proto.ListAccountsRequest
 	}
 	onlySelf := hasSelf && !hasManagement
 
-	accLock.Lock()
-	defer accLock.Unlock()
-
 	teardownServiceUser := s.serviceUserToIndex()
 	defer teardownServiceUser()
 	match, authRequest := getAuthQueryMatch(in.Query)
@@ -172,11 +165,8 @@ func (s Service) ListAccounts(ctx context.Context, in *proto.ListAccountsRequest
 		// - it checks if the cache already contains an entry that matches found account Id // account PasswordProfile.LastPasswordChangeDateTime (k)
 		// - if no entry exists it runs the bcrypt.CompareHashAndPassword as before and if everything is ok it stores the
 		//   result by the (k) as key and (v) as value. If not it errors
-		// - if a entry is found it checks if the given value matches (v). If it doesnt match the cache entry gets removed
+		// - if a entry is found it checks if the given value matches (v). If it doesnt match, the cache entry gets removed
 		//   and it errors.
-		//
-		// if many concurrent requests from the same user come in within a short period of time, it's possible that e is still nil.
-		// This is why all this needs to be wrapped within a sync.Mutex locking to prevent calling bcrypt.CompareHashAndPassword unnecessarily too often.
 		{
 			var suspicious bool
 
@@ -284,8 +274,6 @@ func (s Service) GetAccount(ctx context.Context, in *proto.GetAccountRequest, ou
 	}
 	onlySelf := hasSelf && !hasManagement
 
-	accLock.Lock()
-	defer accLock.Unlock()
 	var id string
 	if id, err = cleanupID(in.Id); err != nil {
 		return merrors.InternalServerError(s.id, "could not clean up account id: %v", err.Error())
@@ -331,8 +319,6 @@ func (s Service) CreateAccount(ctx context.Context, in *proto.CreateAccountReque
 		return merrors.Forbidden(s.id, "no permission for CreateAccount")
 	}
 
-	accLock.Lock()
-	defer accLock.Unlock()
 	var id string
 
 	if in.Account == nil {
@@ -468,8 +454,6 @@ func (s Service) UpdateAccount(ctx context.Context, in *proto.UpdateAccountReque
 	}
 	onlySelf := hasSelf && !hasManagement
 
-	accLock.Lock()
-	defer accLock.Unlock()
 	var id string
 	if in.Account == nil {
 		return merrors.BadRequest(s.id, "account missing")
@@ -633,8 +617,6 @@ func (s Service) DeleteAccount(ctx context.Context, in *proto.DeleteAccountReque
 		return merrors.Forbidden(s.id, "no permission for DeleteAccount")
 	}
 
-	accLock.Lock()
-	defer accLock.Unlock()
 	var id string
 	if id, err = cleanupID(in.Id); err != nil {
 		return merrors.InternalServerError(s.id, "could not clean up account id: %v", err.Error())

changelog/unreleased/account-locking.md

@@ -0,0 +1,16 @@
+Enhancement: remove locking from accounts service
+
+Tags: ocis
+
+In the past we locked every request in the accounts service. This is problematic as soon as we start to hammer the system with many users at the same time.
+The locking is now removed from the accounts service and is moved to the indexer.
+
+Instead of doing locking for reads and writes we now differentiate them by using a named RWLock. 
+
+- remove locking from accounts service
+- add a cached named rwlock pkg
+- use sync.map in the cache pkg
+- use named rwlock in indexer pkg
+- use sync.map in indexer pkg
+
+https://github.com/owncloud/ocis/issues/966

ocis-pkg/cache/cache.go

@@ -13,68 +13,64 @@ type Entry struct {
 
 // Cache is a barebones cache implementation.
 type Cache struct {
-	entries map[string]*Entry
-	size    int
-	m       sync.Mutex
+	sync.Map
+	sizeTotal   int
+	sizeCurrent int
 }
 
 // NewCache returns a new instance of Cache.
-func NewCache(o ...Option) Cache {
-	opts := newOptions(o...)
-
+func NewCache(sizeTotal int) Cache {
 	return Cache{
-		size:    opts.size,
-		entries: map[string]*Entry{},
+		sizeTotal: sizeTotal,
 	}
 }
 
 // Get gets an entry by given key
 func (c *Cache) Get(k string) *Entry {
-	c.m.Lock()
-	defer c.m.Unlock()
-
-	if _, ok := c.entries[k]; ok {
-		if c.expired(c.entries[k]) {
-			delete(c.entries, k)
+	if sme, ok := c.Load(k); ok {
+		e := sme.(*Entry)
+		if c.expired(e) {
+			c.Delete(k)
 			return nil
 		}
-		return c.entries[k]
+		return e
 	}
 	return nil
 }
 
 // Set adds an entry for given key and value
 func (c *Cache) Set(k string, val interface{}, expiration time.Time) {
-	c.m.Lock()
-	defer c.m.Unlock()
-
 	if !c.fits() {
 		c.evict()
 	}
-
-	c.entries[k] = &Entry{
+	c.Store(k, &Entry{
 		val,
 		expiration,
-	}
+	})
+	c.sizeCurrent++
 }
 
 // Unset removes an entry by given key
 func (c *Cache) Unset(k string) bool {
-	if _, ok := c.entries[k]; !ok {
+	if _, ok := c.Load(k); !ok {
 		return false
 	}
 
-	delete(c.entries, k)
+	c.Delete(k)
+	c.sizeCurrent--
 	return true
 }
 
 // evict frees memory from the cache by removing entries that exceeded the cache TTL.
 func (c *Cache) evict() {
-	for i := range c.entries {
-		if c.expired(c.entries[i]) {
-			delete(c.entries, i)
+	c.Range(func(k, sme interface{}) bool {
+		e := sme.(*Entry)
+		if c.expired(e) {
+			c.Delete(k)
+			c.sizeCurrent--
 		}
-	}
+		return true
+	})
 }
 
 // expired checks if an entry is expired
@@ -84,5 +80,5 @@ func (c *Cache) expired(e *Entry) bool {
 
 // fits returns whether the cache fits more entries.
 func (c *Cache) fits() bool {
-	return c.size > len(c.entries)
+	return c.sizeTotal > c.sizeCurrent
 }