Merge pull request #93 from safing/fix/interface-cache-update-record-locking

Fix locking when updating the interface cache in Put methods

database/interface.go

@@ -188,6 +188,16 @@ func (i *Interface) getRecord(dbName string, dbKey string, mustBeWriteable bool)
 		return nil, db, ErrPermissionDenied
 	}
 
+	r.Lock()
+	ttl := r.Meta().GetRelativeExpiry()
+	r.Unlock()
+	i.updateCache(
+		r,
+		false, // writing
+		false, // remove
+		ttl,   // expiry
+	)
+
 	return r, db, nil
 }
 
@@ -243,14 +253,19 @@ func (i *Interface) Put(r record.Record) (err error) {
 	}
 
 	r.Lock()
-	defer r.Unlock()
 	i.options.Apply(r)
+	remove := r.Meta().IsDeleted()
+	ttl := r.Meta().GetRelativeExpiry()
+	r.Unlock()
 
-	written := i.updateCache(r, true)
+	// The record may not be locked when updating the cache.
+	written := i.updateCache(r, true, remove, ttl)
 	if written {
 		return nil
 	}
 
+	r.Lock()
+	defer r.Unlock()
 	return db.Put(r)
 }
 
@@ -271,18 +286,22 @@ func (i *Interface) PutNew(r record.Record) (err error) {
 	}
 
 	r.Lock()
-	defer r.Unlock()
-
 	if r.Meta() != nil {
 		r.Meta().Reset()
 	}
 	i.options.Apply(r)
+	remove := r.Meta().IsDeleted()
+	ttl := r.Meta().GetRelativeExpiry()
+	r.Unlock()
 
-	written := i.updateCache(r, true)
+	// The record may not be locked when updating the cache.
+	written := i.updateCache(r, true, remove, ttl)
 	if written {
 		return nil
 	}
 
+	r.Lock()
+	defer r.Unlock()
 	return db.Put(r)
 }
 

database/interface_cache.go

@@ -159,14 +159,19 @@ func (i *Interface) checkCache(key string) record.Record {
 	return nil
 }
 
-func (i *Interface) updateCache(r record.Record, write bool) (written bool) {
+// updateCache updates an entry in the interface cache. The given record may
+// not be locked, as updating the cache might write an (unrelated) evicted
+// record to the database in the process. If this happens while the
+// DelayedCacheWriter flushes the write cache with the same record present,
+// this will deadlock.
+func (i *Interface) updateCache(r record.Record, write bool, remove bool, ttl int64) (written bool) {
 	// Check if cache is in use.
 	if i.cache == nil {
 		return false
 	}
 
 	// Check if record should be deleted
-	if r.Meta().IsDeleted() {
+	if remove {
 		// Remove entry from cache.
 		i.cache.Remove(r.Key())
 		// Let write through to database storage.
@@ -174,7 +179,6 @@ func (i *Interface) updateCache(r record.Record, write bool) (written bool) {
 	}
 
 	// Update cache with record.
-	ttl := r.Meta().GetRelativeExpiry()
 	if ttl >= 0 {
 		_ = i.cache.SetWithExpire(
 			r.Key(),

database/record/base.go

@@ -6,6 +6,7 @@ import (
 	"github.com/safing/portbase/container"
 	"github.com/safing/portbase/database/accessor"
 	"github.com/safing/portbase/formats/dsd"
+	"github.com/safing/portbase/log"
 )
 
 // Base provides a quick way to comply with the Model interface.
@@ -15,37 +16,46 @@ type Base struct {
 	meta   *Meta
 }
 
+// SetKey sets the key on the database record. The key may only be set once and
+// future calls to SetKey will be ignored. If you want to copy/move the record
+// to another database key, you will need to create a copy and assign a new key.
+// A key must be set before the record is used in any database operation.
+func (b *Base) SetKey(key string) {
+	if !b.KeyIsSet() {
+		b.dbName, b.dbKey = ParseKey(key)
+	} else {
+		log.Errorf("database: key is already set: tried to replace %q with %q", b.Key(), key)
+	}
+}
+
 // Key returns the key of the database record.
+// As the key must be set before any usage and can only be set once, this
+// function may be used without locking the record.
 func (b *Base) Key() string {
 	return b.dbName + ":" + b.dbKey
 }
 
 // KeyIsSet returns true if the database key is set.
+// As the key must be set before any usage and can only be set once, this
+// function may be used without locking the record.
 func (b *Base) KeyIsSet() bool {
-	return len(b.dbName) > 0 && len(b.dbKey) > 0
+	return b.dbName != ""
 }
 
 // DatabaseName returns the name of the database.
+// As the key must be set before any usage and can only be set once, this
+// function may be used without locking the record.
 func (b *Base) DatabaseName() string {
 	return b.dbName
 }
 
 // DatabaseKey returns the database key of the database record.
+// As the key must be set before any usage and can only be set once, this
+// function may be used without locking the record.
 func (b *Base) DatabaseKey() string {
 	return b.dbKey
 }
 
-// SetKey sets the key on the database record, it should only be called after loading the record. Use MoveTo to save the record with another key.
-func (b *Base) SetKey(key string) {
-	b.dbName, b.dbKey = ParseKey(key)
-}
-
-// MoveTo sets a new key for the record and resets all metadata, except for the secret and crownjewel status.
-func (b *Base) MoveTo(key string) {
-	b.SetKey(key)
-	b.meta.Reset()
-}
-
 // Meta returns the metadata object for this record.
 func (b *Base) Meta() *Meta {
 	return b.meta
@@ -59,7 +69,7 @@ func (b *Base) CreateMeta() {
 // UpdateMeta creates the metadata if it does not exist and updates it.
 func (b *Base) UpdateMeta() {
 	if b.meta == nil {
-		b.meta = &Meta{}
+		b.CreateMeta()
 	}
 	b.meta.Update()
 }

database/record/record.go

@@ -6,13 +6,12 @@ import (
 
 // Record provides an interface for uniformally handling database records.
 type Record interface {
-	Key() string // test:config
+	SetKey(key string) // test:config
+	Key() string       // test:config
 	KeyIsSet() bool
 	DatabaseName() string // test
 	DatabaseKey() string  // config
 
-	SetKey(key string) // test:config
-	MoveTo(key string) // test:config
 	Meta() *Meta
 	SetMeta(meta *Meta)
 	CreateMeta()