Merge branch 'locking-responsibility' into HEAD

containers.go

@@ -68,11 +68,24 @@ type Container struct {
 
 // rwContainerStore provides bookkeeping for information about Containers.
 type rwContainerStore interface {
-	fileBasedStore
 	metadataStore
 	containerBigDataStore
 	flaggableStore
 
+	// startWriting makes sure the store is fresh, and locks it for writing.
+	// If this succeeds, the caller MUST call stopWriting().
+	startWriting() error
+
+	// stopWriting releases locks obtained by startWriting.
+	stopWriting()
+
+	// startReading makes sure the store is fresh, and locks it for reading.
+	// If this succeeds, the caller MUST call stopReading().
+	startReading() error
+
+	// stopReading releases locks obtained by startReading.
+	stopReading()
+
 	// Create creates a container that has a specified ID (or generates a
 	// random one if an empty value is supplied) and optional names,
 	// based on the specified image, using the specified layer as its
@@ -173,6 +186,77 @@ func (c *Container) MountOpts() []string {
 	}
 }
 
+// startWritingWithReload makes sure the store is fresh if canReload, and locks it for writing.
+// If this succeeds, the caller MUST call stopWriting().
+//
+// This is an internal implementation detail of containerStore construction, every other caller
+// should use startWriting() instead.
+func (r *containerStore) startWritingWithReload(canReload bool) error {
+	r.lockfile.Lock()
+	succeeded := false
+	defer func() {
+		if !succeeded {
+			r.lockfile.Unlock()
+		}
+	}()
+
+	if canReload {
+		if err := r.reloadIfChanged(); err != nil {
+			return err
+		}
+	}
+
+	succeeded = true
+	return nil
+}
+
+// startWriting makes sure the store is fresh, and locks it for writing.
+// If this succeeds, the caller MUST call stopWriting().
+func (r *containerStore) startWriting() error {
+	return r.startWritingWithReload(true)
+}
+
+// stopWriting releases locks obtained by startWriting.
+func (r *containerStore) stopWriting() {
+	r.lockfile.Unlock()
+}
+
+// startReading makes sure the store is fresh, and locks it for reading.
+// If this succeeds, the caller MUST call stopReading().
+func (r *containerStore) startReading() error {
+	r.lockfile.RLock()
+	succeeded := false
+	defer func() {
+		if !succeeded {
+			r.lockfile.Unlock()
+		}
+	}()
+
+	if err := r.reloadIfChanged(); err != nil {
+		return err
+	}
+
+	succeeded = true
+	return nil
+}
+
+// stopReading releases locks obtained by startReading.
+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 {
+	r.loadMut.Lock()
+	defer r.loadMut.Unlock()
+
+	modified, err := r.lockfile.Modified()
+	if err == nil && modified {
+		return r.load()
+	}
+	return err
+}
+
 func (r *containerStore) Containers() ([]Container, error) {
 	containers := make([]Container, len(r.containers))
 	for i := range r.containers {
@@ -193,7 +277,9 @@ func (r *containerStore) datapath(id, key string) string {
 	return filepath.Join(r.datadir(id), makeBigDataBaseName(key))
 }
 
-func (r *containerStore) Load() error {
+// load reloads the contents of the store from disk.  It should be called
+// with the lock held.
+func (r *containerStore) load() error {
 	needSave := false
 	rpath := r.containerspath()
 	data, err := os.ReadFile(rpath)
@@ -226,13 +312,15 @@ func (r *containerStore) Load() error {
 	r.bylayer = layers
 	r.byname = names
 	if needSave {
-		return r.Save()
+		return r.save()
 	}
 	return nil
 }
 
-func (r *containerStore) Save() error {
-	if !r.Locked() {
+// save saves the contents of the store to disk.  It should be called with
+// the lock held.
+func (r *containerStore) save() error {
+	if !r.lockfile.Locked() {
 		return errors.New("container store is not locked")
 	}
 	rpath := r.containerspath()
@@ -246,7 +334,7 @@ func (r *containerStore) Save() error {
 	if err := ioutils.AtomicWriteFile(rpath, jdata, 0600); err != nil {
 		return err
 	}
-	return r.Touch()
+	return r.lockfile.Touch()
 }
 
 func newContainerStore(dir string) (rwContainerStore, error) {
@@ -265,9 +353,11 @@ func newContainerStore(dir string) (rwContainerStore, error) {
 		bylayer:    make(map[string]*Container),
 		byname:     make(map[string]*Container),
 	}
-	cstore.Lock()
-	defer cstore.Unlock()
-	if err := cstore.Load(); err != nil {
+	if err := cstore.startWritingWithReload(false); err != nil {
+		return nil, err
+	}
+	defer cstore.stopWriting()
+	if err := cstore.load(); err != nil {
 		return nil, err
 	}
 	return &cstore, nil
@@ -294,7 +384,7 @@ func (r *containerStore) ClearFlag(id string, flag string) error {
 		return ErrContainerUnknown
 	}
 	delete(container.Flags, flag)
-	return r.Save()
+	return r.save()
 }
 
 func (r *containerStore) SetFlag(id string, flag string, value interface{}) error {
@@ -306,7 +396,7 @@ func (r *containerStore) SetFlag(id string, flag string, value interface{}) erro
 		container.Flags = make(map[string]interface{})
 	}
 	container.Flags[flag] = value
-	return r.Save()
+	return r.save()
 }
 
 func (r *containerStore) Create(id string, names []string, image, layer, metadata string, options *ContainerOptions) (container *Container, err error) {
@@ -363,7 +453,7 @@ func (r *containerStore) Create(id string, names []string, image, layer, metadat
 		for _, name := range names {
 			r.byname[name] = container
 		}
-		err = r.Save()
+		err = r.save()
 		container = copyContainer(container)
 	}
 	return container, err
@@ -379,7 +469,7 @@ func (r *containerStore) Metadata(id string) (string, error) {
 func (r *containerStore) SetMetadata(id, metadata string) error {
 	if container, ok := r.lookup(id); ok {
 		container.Metadata = metadata
-		return r.Save()
+		return r.save()
 	}
 	return ErrContainerUnknown
 }
@@ -421,7 +511,7 @@ func (r *containerStore) updateNames(id string, names []string, op updateNameOpe
 		r.byname[name] = container
 	}
 	container.Names = names
-	return r.Save()
+	return r.save()
 }
 
 func (r *containerStore) Delete(id string) error {
@@ -453,7 +543,7 @@ func (r *containerStore) Delete(id string) error {
 			r.containers = append(r.containers[:toDeleteIndex], r.containers[toDeleteIndex+1:]...)
 		}
 	}
-	if err := r.Save(); err != nil {
+	if err := r.save(); err != nil {
 		return err
 	}
 	if err := os.RemoveAll(r.datadir(id)); err != nil {
@@ -600,7 +690,7 @@ func (r *containerStore) SetBigData(id, key string, data []byte) error {
 			save = true
 		}
 		if save {
-			err = r.Save()
+			err = r.save()
 		}
 	}
 	return err
@@ -618,46 +708,3 @@ func (r *containerStore) Wipe() error {
 	}
 	return nil
 }
-
-func (r *containerStore) Lock() {
-	r.lockfile.Lock()
-}
-
-func (r *containerStore) RLock() {
-	r.lockfile.RLock()
-}
-
-func (r *containerStore) Unlock() {
-	r.lockfile.Unlock()
-}
-
-func (r *containerStore) Touch() error {
-	return r.lockfile.Touch()
-}
-
-func (r *containerStore) Modified() (bool, error) {
-	return r.lockfile.Modified()
-}
-
-func (r *containerStore) IsReadWrite() bool {
-	return r.lockfile.IsReadWrite()
-}
-
-func (r *containerStore) TouchedSince(when time.Time) bool {
-	return r.lockfile.TouchedSince(when)
-}
-
-func (r *containerStore) Locked() bool {
-	return r.lockfile.Locked()
-}
-
-func (r *containerStore) ReloadIfChanged() error {
-	r.loadMut.Lock()
-	defer r.loadMut.Unlock()
-
-	modified, err := r.Modified()
-	if err == nil && modified {
-		return r.Load()
-	}
-	return err
-}