db: refactor directory locking

Refactor the acquisition of directory locks, moving it outside the logic of
Open and embedding the directory locks within the resolvedDirs struct. This
better encapsulates the locks and removes some subtle error handling within
Open.

Author: jbowens

db.go

@@ -1554,8 +1554,6 @@ func (d *DB) Close() error {
 	// is still valid for the checks below.
 	err = firstError(err, d.mu.versions.close())
 
-	err = firstError(err, d.dirs.Close())
-
 	d.readState.val.unrefLocked()
 
 	current := d.mu.versions.currentVersion()
@@ -1628,6 +1626,7 @@ func (d *DB) Close() error {
 	if v := d.mu.snapshots.count(); v > 0 {
 		err = firstError(err, errors.Errorf("leaked snapshots: %d open snapshots on DB %p", v, d))
 	}
+	err = firstError(err, d.dirs.Close())
 
 	if d.iterTracker != nil {
 		d.iterTracker.Close()

internal/base/directory_lock.go

@@ -5,8 +5,10 @@
 package base
 
 import (
+	"fmt"
 	"io"
 	"os"
+	"strings"
 	"sync/atomic"
 
 	"github.com/cockroachdb/errors"
@@ -19,6 +21,19 @@ type DirLockSet struct {
 	acquired []*DirLock
 }
 
+func (s *DirLockSet) String() string {
+	var buf strings.Builder
+	buf.WriteString("{")
+	for i, l := range s.acquired {
+		if i > 0 {
+			buf.WriteString(", ")
+		}
+		fmt.Fprint(&buf, l.dirname)
+	}
+	buf.WriteString("}")
+	return buf.String()
+}
+
 // Close releases all of the directory locks in the set.
 func (s *DirLockSet) Close() error {
 	var err error

open.go

@@ -103,14 +103,14 @@ func Open(dirname string, opts *Options) (db *DB, err error) {
 	}
 
 	// Open the database and WAL directories first.
-	dirs, err := prepareAndOpenDirs(dirname, opts)
+	dirs, err := prepareOpenAndLockDirs(dirname, opts)
 	if err != nil {
-		return nil, errors.Wrapf(err, "error opening database at %q", dirname)
+		err = errors.Wrapf(err, "error opening database at %q", dirname)
+		err = errors.CombineErrors(err, dirs.Close())
+		return nil, err
 	}
 	defer maybeCleanUp(dirs.Close)
 
-	var dirLocks base.DirLockSet
-
 	rs, err := recoverState(opts, dirname)
 	if err != nil {
 		return nil, err
@@ -294,8 +294,8 @@ func Open(dirname string, opts *Options) (db *DB, err error) {
 	})
 
 	walOpts := wal.Options{
-		Primary:              wal.Dir{FS: opts.FS, Dirname: dirs.WALPrimary.Dirname},
-		Secondary:            wal.Dir{},
+		Primary:              dirs.WALPrimary,
+		Secondary:            dirs.WALSecondary,
 		MinUnflushedWALNum:   wal.NumWAL(d.mu.versions.minUnflushedLogNum),
 		MaxNumRecyclableLogs: opts.MemTableStopWritesThreshold + 1,
 		NoSyncOnClose:        opts.NoSyncOnClose,
@@ -308,45 +308,7 @@ func Open(dirname string, opts *Options) (db *DB, err error) {
 		EventListener:        walEventListenerAdaptor{l: opts.EventListener},
 		WriteWALSyncOffsets:  func() bool { return d.FormatMajorVersion() >= FormatWALSyncChunks },
 	}
-	// Ensure we release the WAL directory locks if we fail to open the
-	// database. If we fail before initializing the WAL manager, this defer is
-	// responsible for releasing the locks. If we fail after initializing the
-	// WAL manager, closing the WAL manager will release the locks.
-	//
-	// TODO(jackson): Open's cleanup error handling logic is convoluted; can we
-	// simplify it?
-	defer maybeCleanUp(func() (err error) {
-		if d.mu.log.manager == nil {
-			if walOpts.Primary.Lock != nil {
-				err = errors.CombineErrors(err, walOpts.Primary.Lock.Close())
-			}
-			if walOpts.Secondary.Lock != nil {
-				err = errors.CombineErrors(err, walOpts.Secondary.Lock.Close())
-			}
-			return err
-		}
-		return nil
-	})
-
-	// Lock the dedicated WAL directory, if configured.
-	if dirs.WALPrimary.Dirname != dirname {
-		walOpts.Primary.Lock, err = dirLocks.AcquireOrValidate(opts.WALDirLock, dirs.WALPrimary.Dirname, opts.FS)
-		if err != nil {
-			return nil, err
-		}
-	}
 	if !opts.ReadOnly && opts.WALFailover != nil {
-		walOpts.Secondary = opts.WALFailover.Secondary
-		// Lock the secondary WAL directory, if distinct from the data directory
-		// and primary WAL directory.
-		if dirs.WALSecondary.Dirname != dirname && dirs.WALSecondary.Dirname != dirs.WALPrimary.Dirname {
-			walOpts.Secondary.Lock, err = dirLocks.AcquireOrValidate(
-				opts.WALFailover.Secondary.Lock, dirs.WALSecondary.Dirname, opts.WALFailover.Secondary.FS)
-			if err != nil {
-				return nil, err
-			}
-		}
-		walOpts.Secondary.Dirname = dirs.WALSecondary.Dirname
 		walOpts.FailoverOptions = opts.WALFailover.FailoverOptions
 		walOpts.FailoverWriteAndSyncLatency = prometheus.NewHistogram(prometheus.HistogramOpts{
 			Buckets: FsyncLatencyBuckets,
@@ -617,20 +579,23 @@ type resolvedDirs struct {
 
 // Close closes the data directory and the directory locks.
 func (d *resolvedDirs) Close() error {
-	err := errors.CombineErrors(
-		d.DataDir.Close(),
-		d.DirLocks.Close(),
-	)
+	var err error
+	if d.DataDir != nil {
+		err = errors.CombineErrors(err, d.DataDir.Close())
+	}
+	err = errors.CombineErrors(err, d.DirLocks.Close())
 	*d = resolvedDirs{}
 	return err
 }
 
-// prepareAndOpenDirs resolves the various directory paths indicated within
+// prepareOpenAndLockDirs resolves the various directory paths indicated within
 // Options (substituting {store_path} relative paths as necessary), creates the
-// directories if they don't exist, and locks the database directory.
+// directories if they don't exist, and acquires directory locks as necessary.
 //
-// Returns an error if ReadOnly is set and the directories don't exist.
-func prepareAndOpenDirs(dirname string, opts *Options) (dirs *resolvedDirs, err error) {
+// Returns an error if ReadOnly is set and the directories don't exist. Always
+// returns a non-nil resolvedDirs that may be closed to release all resources
+// acquired before any error was encountered.
+func prepareOpenAndLockDirs(dirname string, opts *Options) (dirs *resolvedDirs, err error) {
 	dirs = &resolvedDirs{}
 	dirs.WALPrimary.Dirname = dirname
 	dirs.WALPrimary.FS = opts.FS
@@ -676,7 +641,6 @@ func prepareAndOpenDirs(dirname string, opts *Options) (dirs *resolvedDirs, err
 		// Check that the wal dir exists.
 		walDir, err := opts.FS.OpenDir(dirs.WALPrimary.Dirname)
 		if err != nil {
-			dirs.DataDir.Close()
 			return dirs, err
 		}
 		walDir.Close()
@@ -685,10 +649,24 @@ func prepareAndOpenDirs(dirname string, opts *Options) (dirs *resolvedDirs, err
 	// Lock the database directory.
 	_, err = dirs.DirLocks.AcquireOrValidate(opts.Lock, dirname, opts.FS)
 	if err != nil {
-		dirs.DataDir.Close()
 		return dirs, err
 	}
-
+	// Lock the dedicated WAL directory, if configured.
+	if dirs.WALPrimary.Dirname != dirname {
+		dirs.WALPrimary.Lock, err = dirs.DirLocks.AcquireOrValidate(opts.WALDirLock, dirs.WALPrimary.Dirname, opts.FS)
+		if err != nil {
+			return dirs, err
+		}
+	}
+	// Lock the secondary WAL directory, if distinct from the data directory
+	// and primary WAL directory.
+	if opts.WALFailover != nil && dirs.WALSecondary.Dirname != dirname && dirs.WALSecondary.Dirname != dirs.WALPrimary.Dirname {
+		dirs.WALSecondary.Lock, err = dirs.DirLocks.AcquireOrValidate(
+			opts.WALFailover.Secondary.Lock, dirs.WALSecondary.Dirname, dirs.WALSecondary.FS)
+		if err != nil {
+			return dirs, err
+		}
+	}
 	return dirs, nil
 }
 

open_test.go

@@ -501,13 +501,15 @@ func TestOpenAlreadyLocked(t *testing.T) {
 	// Run tests for different filesystems.
 	t.Run("memfs", func(t *testing.T) {
 		for _, tc := range testCases {
-			mem := vfs.NewMem()
-			var tmpDirs [4]string
-			for i := range tmpDirs {
-				tmpDirs[i] = mem.PathJoin("dir", fmt.Sprintf("%d", i))
-				require.NoError(t, mem.MkdirAll(tmpDirs[i], 0755), "Failed to create temp dir %s", tmpDirs[i])
-			}
-			runTest(t, tmpDirs, tc.setupLocks, mem)
+			t.Run(tc.name, func(t *testing.T) {
+				tmpDirs := [4]string{"dir/0", "dir/1", "dir/2", "dir/3"}
+				mem := vfs.NewMem()
+				for i := range tmpDirs {
+					require.NoError(t, mem.MkdirAll(tmpDirs[i], 0755),
+						"Failed to create temp dir %s", tmpDirs[i])
+				}
+				runTest(t, tmpDirs, tc.setupLocks, mem)
+			})
 		}
 	})
 

testdata/cleaner

@@ -15,6 +15,7 @@ open-dir: db_wal
 close: db_wal
 open-dir: db
 lock: db/LOCK
+lock: db_wal/LOCK
 open-dir: db
 open-dir: db
 open-dir: db
@@ -24,7 +25,6 @@ create: db/marker.manifest.000001.MANIFEST-000001
 sync: db/marker.manifest.000001.MANIFEST-000001
 close: db/marker.manifest.000001.MANIFEST-000001
 sync: db
-lock: db_wal/LOCK
 open-dir: db_wal
 create: db/temporary.000002.dbtmp
 sync: db/temporary.000002.dbtmp
@@ -148,6 +148,7 @@ open-dir: db1_wal
 close: db1_wal
 open-dir: db1
 lock: db1/LOCK
+lock: db1_wal/LOCK
 open-dir: db1
 open-dir: db1
 open-dir: db1
@@ -157,7 +158,6 @@ create: db1/marker.manifest.000001.MANIFEST-000001
 sync: db1/marker.manifest.000001.MANIFEST-000001
 close: db1/marker.manifest.000001.MANIFEST-000001
 sync: db1
-lock: db1_wal/LOCK
 open-dir: db1_wal
 create: db1/temporary.000002.dbtmp
 sync: db1/temporary.000002.dbtmp
@@ -242,12 +242,12 @@ open-dir: db1_wal
 close: db1_wal
 open-dir: db1
 lock: db1/LOCK
+lock: db1_wal/LOCK
 open-dir: db1
 open-dir: db1
 open-dir: db1
 open: db1/MANIFEST-000001
 close: db1/MANIFEST-000001
-lock: db1_wal/LOCK
 remove: db1_wal/000003.log
 open-dir: db1_wal
 open: db1/OPTIONS-000002

testdata/event_listener

@@ -14,6 +14,7 @@ open-dir: wal
 close: wal
 open-dir: db
 lock: db/LOCK
+lock: wal/LOCK
 open-dir: db
 open-dir: db
 open-dir: db
@@ -24,7 +25,6 @@ sync: db/marker.manifest.000001.MANIFEST-000001
 close: db/marker.manifest.000001.MANIFEST-000001
 sync: db
 [JOB 1] MANIFEST created 000001
-lock: wal/LOCK
 open-dir: wal
 create: db/temporary.000002.dbtmp
 sync: db/temporary.000002.dbtmp

wal/failover_manager.go

@@ -746,12 +746,6 @@ func (wm *failoverManager) Close() error {
 	for _, f := range wm.dirHandles {
 		err = firstError(err, f.Close())
 	}
-	if wm.opts.Primary.Lock != nil {
-		err = firstError(err, wm.opts.Primary.Lock.Close())
-	}
-	if wm.opts.Secondary.Lock != nil {
-		err = firstError(err, wm.opts.Secondary.Lock.Close())
-	}
 	return err
 }
 

wal/standalone_manager.go

@@ -251,9 +251,6 @@ func (m *StandaloneManager) Close() error {
 		_, err = m.w.Close()
 	}
 	err = firstError(err, m.walDir.Close())
-	if m.o.Primary.Lock != nil {
-		err = firstError(err, m.o.Primary.Lock.Close())
-	}
 	return err
 }