From e5a72ffa372c86f1eb425eb2dc8478954afcbdc3 Mon Sep 17 00:00:00 2001 From: Daniel Date: Mon, 12 Oct 2020 13:54:14 +0200 Subject: [PATCH] Implement review suggestions --- database/database_test.go | 8 ++--- database/interface.go | 62 +++++++++++++++++++++++++++---------- database/interface_cache.go | 14 ++++++--- 3 files changed, 60 insertions(+), 24 deletions(-) diff --git a/database/database_test.go b/database/database_test.go index f625884..b6d793e 100644 --- a/database/database_test.go +++ b/database/database_test.go @@ -35,7 +35,9 @@ func TestMain(m *testing.M) { exitCode := m.Run() - os.RemoveAll(testDir) // clean up + // Clean up the test directory. + // Do not defer, as we end this function with a os.Exit call. + os.RemoveAll(testDir) os.Exit(exitCode) } @@ -239,9 +241,7 @@ func TestDatabaseSystem(t *testing.T) { // panic after 10 seconds, to check for locks finished := make(chan struct{}) - defer func() { - close(finished) - }() + defer close(finished) go func() { select { case <-finished: diff --git a/database/interface.go b/database/interface.go index f921e60..04f1830 100644 --- a/database/interface.go +++ b/database/interface.go @@ -32,12 +32,32 @@ type Interface struct { // Options holds options that may be set for an Interface instance. type Options struct { - Local bool - Internal bool - AlwaysMakeSecret bool - AlwaysMakeCrownjewel bool + // Local specifies if the interface is used by an actor on the local device. + // Setting both the Local and Internal flags will bring performance + // improvements because less checks are needed. + Local bool + + // Internal specifies if the interface is used by an actor within the + // software. Setting both the Local and Internal flags will bring performance + // improvements because less checks are needed. + Internal bool + + // AlwaysMakeSecret will have the interface mark all saved records as secret. + // This means that they will be only accessible by an internal interface. + AlwaysMakeSecret bool + + // AlwaysMakeCrownjewel will have the interface mark all saved records as + // crown jewels. This means that they will be only accessible by a local + // interface. + AlwaysMakeCrownjewel bool + + // AlwaysSetRelativateExpiry will have the interface set a relative expiry, + // based on the current time, on all saved records. AlwaysSetRelativateExpiry int64 - AlwaysSetAbsoluteExpiry int64 + + // AlwaysSetAbsoluteExpiry will have the interface set an absolute expiry on + // all saved records. + AlwaysSetAbsoluteExpiry int64 // CacheSize defines that a cache should be used for this interface and // defines it's size. @@ -47,7 +67,7 @@ type Options struct { // evicted from the cache. CacheSize int - // DelayCachedWrites defines a database storage for which cache writes should + // DelayCachedWrites defines a database name for which cache writes should // be cached and batched. The database backend must support the Batcher // interface. This option is only valid if used with a cache. // Additionally, this may only be used for internal and local interfaces. @@ -72,6 +92,11 @@ func (o *Options) Apply(r record.Record) { } } +// HasAllPermissions returns whether the options specify the highest possible permissions for operations. +func (o *Options) HasAllPermissions() bool { + return o.Local && o.Internal +} + // NewInterface returns a new Interface to the database. func NewInterface(opts *Options) *Interface { if opts == nil { @@ -83,9 +108,9 @@ func NewInterface(opts *Options) *Interface { } if opts.CacheSize > 0 { cacheBuilder := gcache.New(opts.CacheSize).ARC() - if len(opts.DelayCachedWrites) > 0 { + if opts.DelayCachedWrites != "" { cacheBuilder.EvictedFunc(new.cacheEvictHandler) - new.writeCache = make(map[string]record.Record) + new.writeCache = make(map[string]record.Record, opts.CacheSize/2) new.triggerCacheWrite = make(chan struct{}) } new.cache = cacheBuilder.Build() @@ -97,10 +122,10 @@ func NewInterface(opts *Options) *Interface { func (i *Interface) Exists(key string) (bool, error) { _, err := i.Get(key) if err != nil { - switch err { - case ErrNotFound: + switch { + case errors.Is(err, ErrNotFound): return false, nil - case ErrPermissionDenied: + case errors.Is(err, ErrPermissionDenied): return true, nil default: return false, err @@ -131,8 +156,13 @@ func (i *Interface) getRecord(dbName string, dbKey string, mustBeWriteable bool) r = i.checkCache(dbName + ":" + dbKey) if r != nil { - if !r.Meta().CheckPermission(i.options.Local, i.options.Internal) { - return nil, db, ErrPermissionDenied + if !i.options.HasAllPermissions() { + r.Lock() + permitted := r.Meta().CheckPermission(i.options.Local, i.options.Internal) + r.Unlock() + if !permitted { + return nil, db, ErrPermissionDenied + } } return r, db, nil } @@ -183,7 +213,7 @@ func (i *Interface) InsertValue(key string, attribute string, value interface{}) func (i *Interface) Put(r record.Record) (err error) { // get record or only database var db *Controller - if !i.options.Internal || !i.options.Local { + if !i.options.HasAllPermissions() { _, db, err = i.getRecord(r.DatabaseName(), r.DatabaseKey(), true) if err != nil && err != ErrNotFound { return err @@ -216,7 +246,7 @@ func (i *Interface) Put(r record.Record) (err error) { func (i *Interface) PutNew(r record.Record) (err error) { // get record or only database var db *Controller - if !i.options.Internal || !i.options.Local { + if !i.options.HasAllPermissions() { _, db, err = i.getRecord(r.DatabaseName(), r.DatabaseKey(), true) if err != nil && err != ErrNotFound { return err @@ -253,7 +283,7 @@ func (i *Interface) PutMany(dbName string) (put func(record.Record) error) { interfaceBatch := make(chan record.Record, 100) // permission check - if !i.options.Internal || !i.options.Local { + if !i.options.HasAllPermissions() { return func(r record.Record) error { return ErrPermissionDenied } diff --git a/database/interface_cache.go b/database/interface_cache.go index c85882b..3e349e5 100644 --- a/database/interface_cache.go +++ b/database/interface_cache.go @@ -12,7 +12,7 @@ import ( // DelayedCacheWriter must be run by the caller of an interface that uses delayed cache writing. func (i *Interface) DelayedCacheWriter(ctx context.Context) error { // Check if the DelayedCacheWriter should be run at all. - if i.options.CacheSize <= 0 || len(i.options.DelayCachedWrites) == 0 { + if i.options.CacheSize <= 0 || i.options.DelayCachedWrites == "" { return errors.New("delayed cache writer is not applicable to this database interface") } @@ -25,7 +25,7 @@ func (i *Interface) DelayedCacheWriter(ctx context.Context) error { } // percentThreshold defines the minimum percentage of entries in the write cache in relation to the cache size that need to be present in order for flushing the cache to the database storage. - percentThreshold := 25 // % + percentThreshold := 25 thresholdWriteTicker := time.NewTicker(5 * time.Second) forceWriteTicker := time.NewTicker(5 * time.Minute) @@ -105,12 +105,14 @@ func (i *Interface) cacheEvictHandler(keyData, _ interface{}) { } // Check if the evicted record is one that is to be written. + // Lock the write cache until the end of the function. + // The read cache is locked anyway for the whole duration. i.writeCacheLock.Lock() + defer i.writeCacheLock.Unlock() r, ok := i.writeCache[key] if ok { delete(i.writeCache, key) } - i.writeCacheLock.Unlock() if !ok { return } @@ -122,6 +124,10 @@ func (i *Interface) cacheEvictHandler(keyData, _ interface{}) { log.Warningf("database: failed to write evicted cache entry %q: database %q does not exist", key, r.DatabaseName()) return } + + r.Lock() + defer r.Unlock() + err = db.Put(r) if err != nil { log.Warningf("database: failed to write evicted cache entry %q to database: %s", key, err) @@ -186,7 +192,7 @@ func (i *Interface) updateCache(r record.Record, write bool) (written bool) { // 1. The record is being written. // 2. Write delaying is active. // 3. Write delaying is active for the database of this record. - if write && len(i.options.DelayCachedWrites) > 0 && r.DatabaseName() == i.options.DelayCachedWrites { + if write && r.DatabaseName() == i.options.DelayCachedWrites { i.writeCacheLock.Lock() defer i.writeCacheLock.Unlock() i.writeCache[r.Key()] = r