Merge pull request #1399 from mtrmac/no-Locked

API break: Remove Lockfile.Locked
containers · Oct 25, 2022 · 9c1bd67 · 9c1bd67
2 parents 301a806 + d4462aa
commit 9c1bd67
Show file tree

Hide file tree

Showing 7 changed files with 127 additions and 65 deletions.
diff --git a/containers.go b/containers.go
@@ -191,7 +191,7 @@ func (r *containerStore) startWritingWithReload(canReload bool) error {
 	}()
 
 	if canReload {
-		if err := r.ReloadIfChanged(); err != nil {
+		if err := r.reloadIfChanged(true); err != nil {
 			return err
 		}
 	}
@@ -222,7 +222,7 @@ func (r *containerStore) startReading() error {
 		}
 	}()
 
-	if err := r.ReloadIfChanged(); err != nil {
+	if err := r.reloadIfChanged(false); err != nil {
 		return err
 	}
 
@@ -235,14 +235,17 @@ func (r *containerStore) stopReading() {
 	r.lockfile.Unlock()
 }
 
-// ReloadIfChanged reloads the contents of the store from disk if it is changed.
-func (r *containerStore) ReloadIfChanged() error {
+// reloadIfChanged reloads the contents of the store from disk if it is changed.
+//
+// The caller must hold r.lockfile for reading _or_ writing; lockedForWriting is true
+// if it is held for writing.
+func (r *containerStore) reloadIfChanged(lockedForWriting bool) error {
 	r.loadMut.Lock()
 	defer r.loadMut.Unlock()
 
 	modified, err := r.lockfile.Modified()
 	if err == nil && modified {
-		return r.Load()
+		return r.load(lockedForWriting)
 	}
 	return err
 }
@@ -267,9 +270,11 @@ func (r *containerStore) datapath(id, key string) string {
 	return filepath.Join(r.datadir(id), makeBigDataBaseName(key))
 }
 
-// Load reloads the contents of the store from disk.  It should be called
-// with the lock held.
-func (r *containerStore) Load() error {
+// load reloads the contents of the store from disk.
+//
+// The caller must hold r.lockfile for reading _or_ writing; lockedForWriting is true
+// if it is held for writing.
+func (r *containerStore) load(lockedForWriting bool) error {
 	needSave := false
 	rpath := r.containerspath()
 	data, err := os.ReadFile(rpath)
@@ -302,17 +307,19 @@ func (r *containerStore) Load() error {
 	r.bylayer = layers
 	r.byname = names
 	if needSave {
+		if !lockedForWriting {
+			// Eventually, the callers should be modified to retry with a write lock, instead.
+			return errors.New("container store is inconsistent and the current caller does not hold a write lock")
+		}
 		return r.Save()
 	}
 	return nil
 }
 
 // Save saves the contents of the store to disk.  It should be called with
-// the lock held.
+// the lock held, locked for writing.
 func (r *containerStore) Save() error {
-	if !r.lockfile.Locked() {
-		return errors.New("container store is not locked")
-	}
+	r.lockfile.AssertLockedForWriting()
 	rpath := r.containerspath()
 	if err := os.MkdirAll(filepath.Dir(rpath), 0700); err != nil {
 		return err
@@ -347,7 +354,7 @@ func newContainerStore(dir string) (rwContainerStore, error) {
 		return nil, err
 	}
 	defer cstore.stopWriting()
-	if err := cstore.Load(); err != nil {
+	if err := cstore.load(true); err != nil {
 		return nil, err
 	}
 	return &cstore, nil

diff --git a/images.go b/images.go
@@ -1,7 +1,6 @@
 package storage
 
 import (
-	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -154,7 +153,7 @@ type rwImageStore interface {
 }
 
 type imageStore struct {
-	lockfile Locker
+	lockfile Locker // lockfile.IsReadWrite can be used to distinguish between read-write and read-only image stores.
 	dir      string
 	images   []*Image
 	idindex  *truncindex.TruncIndex
@@ -209,7 +208,7 @@ func (r *imageStore) startWritingWithReload(canReload bool) error {
 	}()
 
 	if canReload {
-		if err := r.ReloadIfChanged(); err != nil {
+		if err := r.reloadIfChanged(true); err != nil {
 			return err
 		}
 	}
@@ -244,7 +243,7 @@ func (r *imageStore) startReadingWithReload(canReload bool) error {
 	}()
 
 	if canReload {
-		if err := r.ReloadIfChanged(); err != nil {
+		if err := r.reloadIfChanged(false); err != nil {
 			return err
 		}
 	}
@@ -264,14 +263,17 @@ func (r *imageStore) stopReading() {
 	r.lockfile.Unlock()
 }
 
-// ReloadIfChanged reloads the contents of the store from disk if it is changed.
-func (r *imageStore) ReloadIfChanged() error {
+// reloadIfChanged reloads the contents of the store from disk if it is changed.
+//
+// The caller must hold r.lockfile for reading _or_ writing; lockedForWriting is true
+// if it is held for writing.
+func (r *imageStore) reloadIfChanged(lockedForWriting bool) error {
 	r.loadMut.Lock()
 	defer r.loadMut.Unlock()
 
 	modified, err := r.lockfile.Modified()
 	if err == nil && modified {
-		return r.Load()
+		return r.load(lockedForWriting)
 	}
 	return err
 }
@@ -336,9 +338,11 @@ func (i *Image) recomputeDigests() error {
 	return nil
 }
 
-// Load reloads the contents of the store from disk.  It should be called
-// with the lock held.
-func (r *imageStore) Load() error {
+// load reloads the contents of the store from disk.
+//
+// The caller must hold r.lockfile for reading _or_ writing; lockedForWriting is true
+// if it is held for writing.
+func (r *imageStore) load(lockedForWriting bool) error {
 	shouldSave := false
 	rpath := r.imagespath()
 	data, err := os.ReadFile(rpath)
@@ -376,7 +380,8 @@ func (r *imageStore) Load() error {
 			image.ReadOnly = !r.lockfile.IsReadWrite()
 		}
 	}
-	if shouldSave && (!r.lockfile.IsReadWrite() || !r.lockfile.Locked()) {
+	if shouldSave && (!r.lockfile.IsReadWrite() || !lockedForWriting) {
+		// Eventually, the callers should be modified to retry with a write lock if IsReadWrite && !lockedForWriting, instead.
 		return ErrDuplicateImageNames
 	}
 	r.images = images
@@ -391,14 +396,12 @@ func (r *imageStore) Load() error {
 }
 
 // Save saves the contents of the store to disk.  It should be called with
-// the lock held.
+// the lock held, locked for writing.
 func (r *imageStore) Save() error {
 	if !r.lockfile.IsReadWrite() {
 		return fmt.Errorf("not allowed to modify the image store at %q: %w", r.imagespath(), ErrStoreIsReadOnly)
 	}
-	if !r.lockfile.Locked() {
-		return errors.New("image store is not locked for writing")
-	}
+	r.lockfile.AssertLockedForWriting()
 	rpath := r.imagespath()
 	if err := os.MkdirAll(filepath.Dir(rpath), 0700); err != nil {
 		return err
@@ -433,7 +436,7 @@ func newImageStore(dir string) (rwImageStore, error) {
 		return nil, err
 	}
 	defer istore.stopWriting()
-	if err := istore.Load(); err != nil {
+	if err := istore.load(true); err != nil {
 		return nil, err
 	}
 	return &istore, nil
@@ -456,7 +459,7 @@ func newROImageStore(dir string) (roImageStore, error) {
 		return nil, err
 	}
 	defer istore.stopReading()
-	if err := istore.Load(); err != nil {
+	if err := istore.load(false); err != nil {
 		return nil, err
 	}
 	return &istore, nil

diff --git a/layers.go b/layers.go
@@ -331,7 +331,7 @@ func (r *layerStore) startWritingWithReload(canReload bool) error {
 	}()
 
 	if canReload {
-		if err := r.ReloadIfChanged(); err != nil {
+		if err := r.reloadIfChanged(true); err != nil {
 			return err
 		}
 	}
@@ -366,7 +366,7 @@ func (r *layerStore) startReadingWithReload(canReload bool) error {
 	}()
 
 	if canReload {
-		if err := r.ReloadIfChanged(); err != nil {
+		if err := r.reloadIfChanged(false); err != nil {
 			return err
 		}
 	}
@@ -420,14 +420,17 @@ func (r *layerStore) Modified() (bool, error) {
 	return tmodified, nil
 }
 
-// ReloadIfChanged reloads the contents of the store from disk if it is changed.
-func (r *layerStore) ReloadIfChanged() error {
+// reloadIfChanged reloads the contents of the store from disk if it is changed.
+//
+// The caller must hold r.lockfile for reading _or_ writing; lockedForWriting is true
+// if it is held for writing.
+func (r *layerStore) reloadIfChanged(lockedForWriting bool) error {
 	r.loadMut.Lock()
 	defer r.loadMut.Unlock()
 
 	modified, err := r.Modified()
 	if err == nil && modified {
-		return r.Load()
+		return r.load(lockedForWriting)
 	}
 	return err
 }
@@ -448,9 +451,11 @@ func (r *layerStore) layerspath() string {
 	return filepath.Join(r.layerdir, "layers.json")
 }
 
-// Load reloads the contents of the store from disk.  It should be called
-// with the lock held.
-func (r *layerStore) Load() error {
+// load reloads the contents of the store from disk.
+//
+// The caller must hold r.lockfile for reading _or_ writing; lockedForWriting is true
+// if it is held for writing.
+func (r *layerStore) load(lockedForWriting bool) error {
 	shouldSave := false
 	rpath := r.layerspath()
 	info, err := os.Stat(rpath)
@@ -499,7 +504,8 @@ func (r *layerStore) Load() error {
 		}
 		err = nil
 	}
-	if shouldSave && (!r.lockfile.IsReadWrite() || !r.lockfile.Locked()) {
+	if shouldSave && (!r.lockfile.IsReadWrite() || !lockedForWriting) {
+		// Eventually, the callers should be modified to retry with a write lock if IsReadWrite && !lockedForWriting, instead.
 		return ErrDuplicateLayerNames
 	}
 	r.layers = layers
@@ -520,7 +526,7 @@ func (r *layerStore) Load() error {
 		// Last step: as we’re writable, try to remove anything that a previous
 		// user of this storage area marked for deletion but didn't manage to
 		// actually delete.
-		if r.lockfile.Locked() {
+		if lockedForWriting {
 			for _, layer := range r.layers {
 				if layer.Flags == nil {
 					layer.Flags = make(map[string]interface{})
@@ -581,7 +587,7 @@ func (r *layerStore) loadMounts() error {
 }
 
 // Save saves the contents of the store to disk.  It should be called with
-// the lock held.
+// the lock held, locked for writing.
 func (r *layerStore) Save() error {
 	r.mountsLockfile.Lock()
 	defer r.mountsLockfile.Unlock()
@@ -595,9 +601,7 @@ func (r *layerStore) saveLayers() error {
 	if !r.lockfile.IsReadWrite() {
 		return fmt.Errorf("not allowed to modify the layer store at %q: %w", r.layerspath(), ErrStoreIsReadOnly)
 	}
-	if !r.lockfile.Locked() {
-		return errors.New("layer store is not locked for writing")
-	}
+	r.lockfile.AssertLockedForWriting()
 	rpath := r.layerspath()
 	if err := os.MkdirAll(filepath.Dir(rpath), 0700); err != nil {
 		return err
@@ -616,9 +620,7 @@ func (r *layerStore) saveMounts() error {
 	if !r.lockfile.IsReadWrite() {
 		return fmt.Errorf("not allowed to modify the layer store at %q: %w", r.layerspath(), ErrStoreIsReadOnly)
 	}
-	if !r.mountsLockfile.Locked() {
-		return errors.New("layer store mount information is not locked for writing")
-	}
+	r.mountsLockfile.AssertLockedForWriting()
 	mpath := r.mountspath()
 	if err := os.MkdirAll(filepath.Dir(mpath), 0700); err != nil {
 		return err
@@ -675,7 +677,7 @@ func (s *store) newLayerStore(rundir string, layerdir string, driver drivers.Dri
 		return nil, err
 	}
 	defer rlstore.stopWriting()
-	if err := rlstore.Load(); err != nil {
+	if err := rlstore.load(true); err != nil {
 		return nil, err
 	}
 	return &rlstore, nil
@@ -700,7 +702,7 @@ func newROLayerStore(rundir string, layerdir string, driver drivers.Driver) (roL
 		return nil, err
 	}
 	defer rlstore.stopReading()
-	if err := rlstore.Load(); err != nil {
+	if err := rlstore.load(false); err != nil {
 		return nil, err
 	}
 	return &rlstore, nil

diff --git a/pkg/lockfile/lockfile.go b/pkg/lockfile/lockfile.go
@@ -40,8 +40,13 @@ type Locker interface {
 	// IsReadWrite() checks if the lock file is read-write
 	IsReadWrite() bool
 
-	// Locked() checks if lock is locked for writing by a thread in this process
-	Locked() bool
+	// AssertLocked() can be used by callers that _know_ that they hold the lock (for reading or writing), for sanity checking.
+	// It might do nothing at all, or it may panic if the caller is not the owner of this lock.
+	AssertLocked()
+
+	// AssertLocked() can be used by callers that _know_ that they hold the lock locked for writing, for sanity checking.
+	// It might do nothing at all, or it may panic if the caller is not the owner of this lock for writing.
+	AssertLockedForWriting()
 }
 
 var (