db: fix double Close of directory lock during failed Open

Previously if an error occurred during Open after the WAL manager had been
initialized, it was possible for a directory lock to be Closed multiple times.
This commit refactors the error handling to ensure we release the WALDir and
WALFailover.Secondary locks at most once.

Author: jbowens

internal/base/directory_lock.go

@@ -64,8 +64,8 @@ type DirLock struct {
 	//
 	// When acquired by the client and passed to Open, refs = 1 and the Open
 	// call increments it to 2. When the database is closed, it's decremented to
-	// 1. Finally when the original caller, calls Close on the Lock, it's
-	// drecemented to zero and the underlying file lock is released.
+	// 1. Finally when the original caller calls Close on the Lock, it's
+	// decremented to zero and the underlying file lock is released.
 	//
 	// When Open acquires the file lock, refs remains at 1 until the database is
 	// closed.
@@ -78,7 +78,7 @@ func (l *DirLock) refForOpen() error {
 	// it's 2, the lock is already in use by another database within the
 	// process.
 	if !l.refs.CompareAndSwap(1, 2) {
-		return errors.Errorf("pebble: unexpected Lock reference count; is the lock already in use?")
+		return errors.Errorf("pebble: unexpected %q DirLock reference count; is the lock already in use?", l.dirname)
 	}
 	return nil
 }
@@ -92,8 +92,11 @@ func (l *DirLock) Refs() int {
 // database. Close must not be called until after a database using the Lock has
 // been closed.
 func (l *DirLock) Close() error {
-	if l.refs.Add(-1) > 0 {
+	v := l.refs.Add(-1)
+	if v > 0 {
 		return nil
+	} else if v < 0 {
+		return errors.AssertionFailedf("pebble: unexpected %q DirLock reference count %d", l.dirname, v)
 	}
 	defer func() { l.fileLock = nil }()
 	return l.fileLock.Close()

open.go

@@ -101,52 +101,25 @@ func Open(dirname string, opts *Options) (db *DB, err error) {
 
 	// In all error cases, we return db = nil; this is used by various
 	// deferred cleanups.
+	maybeCleanUp := func(fn func() error) {
+		if db == nil {
+			err = errors.CombineErrors(err, fn())
+		}
+	}
 
 	// Open the database and WAL directories first.
 	walDirname, secondaryWalDirName, dataDir, err := prepareAndOpenDirs(dirname, opts)
 	if err != nil {
 		return nil, errors.Wrapf(err, "error opening database at %q", dirname)
 	}
-	defer func() {
-		if db == nil {
-			dataDir.Close()
-		}
-	}()
-
-	// Lock all directories that will be written to.
-	var dirLocks [3]*base.DirLock
-	defer func() {
-		for _, lock := range dirLocks {
-			if db == nil && lock != nil {
-				_ = lock.Close()
-			}
-		}
-	}()
+	defer maybeCleanUp(dataDir.Close)
 
 	// Lock the database directory.
 	fileLock, err := base.AcquireOrValidateDirectoryLock(opts.Lock, dirname, opts.FS)
 	if err != nil {
 		return nil, err
 	}
-	dirLocks[0] = fileLock
-
-	// Lock the WAL directories, if configured.
-	var walDirLock, walFailoverLock *base.DirLock
-	if walDirname != dirname {
-		walDirLock, err = base.AcquireOrValidateDirectoryLock(opts.WALDirLock, walDirname, opts.FS)
-		if err != nil {
-			return nil, err
-		}
-		dirLocks[1] = walDirLock
-	}
-	if opts.WALFailover != nil && secondaryWalDirName != dirname && secondaryWalDirName != walDirname {
-		walFailoverLock, err = base.AcquireOrValidateDirectoryLock(
-			opts.WALFailover.Secondary.Lock, secondaryWalDirName, opts.WALFailover.Secondary.FS)
-		if err != nil {
-			return nil, err
-		}
-		dirLocks[2] = walFailoverLock
-	}
+	defer maybeCleanUp(fileLock.Close)
 
 	// List the directory contents. This also happens to include WAL log files, if
 	// they are in the same dir, but we will ignore those below. The provider is
@@ -358,8 +331,9 @@ func Open(dirname string, opts *Options) (db *DB, err error) {
 	d.mu.log.metrics.fsyncLatency = prometheus.NewHistogram(prometheus.HistogramOpts{
 		Buckets: FsyncLatencyBuckets,
 	})
+
 	walOpts := wal.Options{
-		Primary:              wal.Dir{Lock: walDirLock, FS: opts.FS, Dirname: walDirname},
+		Primary:              wal.Dir{FS: opts.FS, Dirname: walDirname},
 		Secondary:            wal.Dir{},
 		MinUnflushedWALNum:   wal.NumWAL(d.mu.versions.minUnflushedLogNum),
 		MaxNumRecyclableLogs: opts.MemTableStopWritesThreshold + 1,
@@ -373,10 +347,45 @@ 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 walDirname != dirname {
+		walOpts.Primary.Lock, err = base.AcquireOrValidateDirectoryLock(opts.WALDirLock, walDirname, opts.FS)
+		if err != nil {
+			return nil, err
+		}
+	}
 	if opts.WALFailover != nil {
 		walOpts.Secondary = opts.WALFailover.Secondary
+		// Lock the secondary WAL directory, if distinct from the data directory
+		// and primary WAL directory.
+		if secondaryWalDirName != dirname && secondaryWalDirName != walDirname {
+			walOpts.Secondary.Lock, err = base.AcquireOrValidateDirectoryLock(
+				opts.WALFailover.Secondary.Lock, secondaryWalDirName, opts.WALFailover.Secondary.FS)
+			if err != nil {
+				return nil, err
+			}
+		}
 		walOpts.Secondary.Dirname = secondaryWalDirName
-		walOpts.Secondary.Lock = walFailoverLock
 		walOpts.FailoverOptions = opts.WALFailover.FailoverOptions
 		walOpts.FailoverWriteAndSyncLatency = prometheus.NewHistogram(prometheus.HistogramOpts{
 			Buckets: FsyncLatencyBuckets,
@@ -416,12 +425,7 @@ func Open(dirname string, opts *Options) (db *DB, err error) {
 	if err != nil {
 		return nil, err
 	}
-	defer func() {
-		if db == nil {
-			_ = walManager.Close()
-		}
-	}()
-
+	defer maybeCleanUp(walManager.Close)
 	d.mu.log.manager = walManager
 
 	d.cleanupManager = openCleanupManager(opts, d.objProvider, d.getDeletionPacerInfo)

testdata/cleaner

@@ -15,7 +15,6 @@ 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,6 +23,7 @@ sync: db/MANIFEST-000001
 create: 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_wal/000002.log
 sync: db_wal
@@ -146,7 +146,6 @@ 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
@@ -155,6 +154,7 @@ sync: db1/MANIFEST-000001
 create: 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_wal/000002.log
 sync: db1_wal
@@ -238,12 +238,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
 open-dir: db1_wal
 open: db1/OPTIONS-000003
 close: db1/OPTIONS-000003

testdata/event_listener

@@ -14,7 +14,6 @@ open-dir: wal
 close: wal
 open-dir: db
 lock: db/LOCK
-lock: wal/LOCK
 open-dir: db
 open-dir: db
 open-dir: db
@@ -24,6 +23,7 @@ create: 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: wal/000002.log
 sync: wal