cleanup locking, add named mutex and make cache atomically

.drone.star

@@ -265,7 +265,7 @@ def testOcisModule(ctx, module):
   steps = makeGenerate(module) + [
     {
       'name': 'vet',
-      'image': 'webhippie/golang:1.14',
+      'image': 'webhippie/golang:1.15',
       'pull': 'always',
       'commands': [
         'make -C %s vet' % (module),
@@ -274,7 +274,7 @@ def testOcisModule(ctx, module):
     },
     {
       'name': 'staticcheck',
-      'image': 'webhippie/golang:1.14',
+      'image': 'webhippie/golang:1.15',
       'pull': 'always',
       'commands': [
         'make -C %s staticcheck' % (module),
@@ -283,7 +283,7 @@ def testOcisModule(ctx, module):
     },
     {
       'name': 'lint',
-      'image': 'webhippie/golang:1.14',
+      'image': 'webhippie/golang:1.15',
       'pull': 'always',
       'commands': [
         'make -C %s lint' % (module),
@@ -292,7 +292,7 @@ def testOcisModule(ctx, module):
     },
     {
         'name': 'test',
-        'image': 'webhippie/golang:1.14',
+        'image': 'webhippie/golang:1.15',
         'pull': 'always',
         'commands': [
           'make -C %s test' % (module),
@@ -923,15 +923,15 @@ def binaryRelease(ctx, name):
       makeGenerate('ocis') + [
       {
         'name': 'build',
-        'image': 'webhippie/golang:1.14',
+        'image': 'webhippie/golang:1.15',
         'pull': 'always',
         'commands': [
           'make -C ocis release-%s' % (name),
         ],
       },
       {
         'name': 'finish',
-        'image': 'webhippie/golang:1.14',
+        'image': 'webhippie/golang:1.15',
         'pull': 'always',
         'commands': [
           'make -C ocis release-finish',
@@ -1094,7 +1094,7 @@ def changelog(ctx):
     'steps': [
       {
         'name': 'generate',
-        'image': 'webhippie/golang:1.14',
+        'image': 'webhippie/golang:1.15',
         'pull': 'always',
         'commands': [
           'make -C ocis changelog',
@@ -1232,7 +1232,7 @@ def docs(ctx):
     'steps': [
       {
         'name': 'generate-config-docs',
-        'image': 'webhippie/golang:1.14',
+        'image': 'webhippie/golang:1.15',
         'commands': ['make -C %s config-docs-generate' % (module) for module in config['modules']],
       },
       {
@@ -1313,7 +1313,7 @@ def makeGenerate(module):
   return [
     {
       'name': 'generate',
-      'image': 'webhippie/golang:1.14',
+      'image': 'webhippie/golang:1.15',
       'pull': 'always',
       'commands': [
         'make -C %s generate' % (module),
@@ -1507,7 +1507,7 @@ def build():
   return [
     {
       'name': 'build',
-      'image': 'webhippie/golang:1.14',
+      'image': 'webhippie/golang:1.15',
       'pull': 'always',
       'commands': [
         'make -C ocis build',

Dockerfile

@@ -1,11 +1,11 @@
-FROM webhippie/golang:1.14 as build
+FROM webhippie/golang:1.15 as build
 
 COPY ./ /ocis/
 ENV CGO_ENABLED=0
 ENV GOOS=linux
 
 RUN apk update && \
-	apk upgrade && \
+	apk upgrade --ignore musl-dev && \
 	apk add make gcc bash && \
 	rm -rf /var/cache/apk/*
 

accounts/pkg/service/v0/accounts.go

@@ -6,12 +6,11 @@ import (
 	"crypto/sha256"
 	"encoding/hex"
 	"fmt"
-	"github.com/owncloud/ocis/ocis-pkg/cache"
+	"github.com/owncloud/ocis/ocis-pkg/sync"
 	"golang.org/x/crypto/bcrypt"
 	"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 = sync.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
 
@@ -188,7 +178,7 @@ func (s Service) ListAccounts(ctx context.Context, in *proto.ListAccountsRequest
 			vh.Write([]byte(a.PasswordProfile.Password))
 			v := vh.Sum([]byte(password))
 
-			e := passwordValidCache.Get(k)
+			e := passwordValidCache.Load(k)
 
 			if e == nil {
 				suspicious = !isPasswordValid(s.log, a.PasswordProfile.Password, password)
@@ -197,12 +187,12 @@ func (s Service) ListAccounts(ctx context.Context, in *proto.ListAccountsRequest
 			}
 
 			if suspicious {
-				passwordValidCache.Unset(k)
+				passwordValidCache.Delete(k)
 				return merrors.Unauthorized(s.id, "account not found or invalid credentials")
 			}
 
 			if e == nil {
-				passwordValidCache.Set(k, v, time.Now().Add(passwordValidCacheExpiration))
+				passwordValidCache.Store(k, v, time.Now().Add(passwordValidCacheExpiration))
 			}
 		}
 
@@ -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/sync-package.md

@@ -0,0 +1,17 @@
+Enhancement: add named locks and refactor cache
+
+Tags: ocis-pkg, accounts
+
+We had the case that we needed kind of a named locking mechanism which enables us to lock only under certain conditions.
+It's used in the indexer package where we do not need to lock everything, instead just lock the requested parts and differentiate between reads and writes.
+
+This made it possible to entirely remove locks from the accounts service and move them to the ocis-pkg indexer.
+Another part of this refactor was to make the cache atomic and write tests for it.
+
+- remove locking from accounts service
+- add sync package with named mutex
+- add named locking to indexer
+- move cache to sync package
+
+https://github.com/owncloud/ocis/pull/1212
+https://github.com/owncloud/ocis/issues/966