Fix Windows flock/funlock race (etcd-io#122)

John Howard · xiang90 · commit 7ee3ded59d48 · 2018-09-12T13:56:54.000-07:00
Signed-off-by: John Howard &lt;jhoward@microsoft.com&gt;
diff --git a/bolt_unix.go b/bolt_unix.go
@@ -4,14 +4,13 @@ package bbolt
 
 import (
 	"fmt"
-	"os"
 	"syscall"
 	"time"
 	"unsafe"
 )
 
 // flock acquires an advisory lock on a file descriptor.
-func flock(db *DB, mode os.FileMode, exclusive bool, timeout time.Duration) error {
+func flock(db *DB, exclusive bool, timeout time.Duration) error {
 	var t time.Time
 	if timeout != 0 {
 		t = time.Now()
diff --git a/bolt_unix_solaris.go b/bolt_unix_solaris.go
@@ -2,7 +2,6 @@ package bbolt
 
 import (
 	"fmt"
-	"os"
 	"syscall"
 	"time"
 	"unsafe"
@@ -11,7 +10,7 @@ import (
 )
 
 // flock acquires an advisory lock on a file descriptor.
-func flock(db *DB, mode os.FileMode, exclusive bool, timeout time.Duration) error {
+func flock(db *DB, exclusive bool, timeout time.Duration) error {
 	var t time.Time
 	if timeout != 0 {
 		t = time.Now()
diff --git a/bolt_windows.go b/bolt_windows.go
@@ -16,8 +16,6 @@ var (
 )
 
 const (
-	lockExt = ".lock"
-
 	// see https://msdn.microsoft.com/en-us/library/windows/desktop/aa365203(v=vs.85).aspx
 	flagLockExclusive       = 2
 	flagLockFailImmediately = 1
@@ -48,28 +46,24 @@ func fdatasync(db *DB) error {
 }
 
 // flock acquires an advisory lock on a file descriptor.
-func flock(db *DB, mode os.FileMode, exclusive bool, timeout time.Duration) error {
-	// Create a separate lock file on windows because a process
-	// cannot share an exclusive lock on the same file. This is
-	// needed during Tx.WriteTo().
-	f, err := os.OpenFile(db.path+lockExt, os.O_CREATE, mode)
-	if err != nil {
-		return err
-	}
-	db.lockfile = f
-
+func flock(db *DB, exclusive bool, timeout time.Duration) error {
 	var t time.Time
 	if timeout != 0 {
 		t = time.Now()
 	}
-	fd := f.Fd()
 	var flag uint32 = flagLockFailImmediately
 	if exclusive {
 		flag |= flagLockExclusive
 	}
 	for {
-		// Attempt to obtain an exclusive lock.
-		err := lockFileEx(syscall.Handle(fd), flag, 0, 1, 0, &syscall.Overlapped{})
+		// Fix for https://github.com/etcd-io/bbolt/issues/121. Use byte-range
+		// -1..0 as the lock on the database file.
+		var m1 uint32 = (1 << 32) - 1 // -1 in a uint32
+		err := lockFileEx(syscall.Handle(db.file.Fd()), flag, 0, 1, 0, &syscall.Overlapped{
+			Offset:     m1,
+			OffsetHigh: m1,
+		})
+
 		if err == nil {
 			return nil
 		} else if err != errLockViolation {
@@ -88,9 +82,11 @@ func flock(db *DB, mode os.FileMode, exclusive bool, timeout time.Duration) erro
 
 // funlock releases an advisory lock on a file descriptor.
 func funlock(db *DB) error {
-	err := unlockFileEx(syscall.Handle(db.lockfile.Fd()), 0, 1, 0, &syscall.Overlapped{})
-	db.lockfile.Close()
-	os.Remove(db.path + lockExt)
+	var m1 uint32 = (1 << 32) - 1 // -1 in a uint32
+	err := unlockFileEx(syscall.Handle(db.file.Fd()), 0, 1, 0, &syscall.Overlapped{
+		Offset:     m1,
+		OffsetHigh: m1,
+	})
 	return err
 }
 
diff --git a/db.go b/db.go
@@ -105,8 +105,7 @@ type DB struct {
 
 	path     string
 	file     *os.File
-	lockfile *os.File // windows only
-	dataref  []byte   // mmap'ed readonly, write throws SEGV
+	dataref  []byte // mmap'ed readonly, write throws SEGV
 	data     *[maxMapSize]byte
 	datasz   int
 	filesz   int // current on disk file size
@@ -197,8 +196,7 @@ func Open(path string, mode os.FileMode, options *Options) (*DB, error) {
 	// if !options.ReadOnly.
 	// The database file is locked using the shared lock (more than one process may
 	// hold a lock at the same time) otherwise (options.ReadOnly is set).
-	if err := flock(db, mode, !db.readOnly, options.Timeout); err != nil {
-		db.lockfile = nil // make 'unused' happy. TODO: rework locks
+	if err := flock(db, !db.readOnly, options.Timeout); err != nil {
 		_ = db.close()
 		return nil, err
 	}
diff --git a/db_test.go b/db_test.go
@@ -63,6 +63,34 @@ func TestOpen(t *testing.T) {
 	}
 }
 
+// Regression validation for https://github.com/etcd-io/bbolt/pull/122.
+// Tests multiple goroutines simultaneously opening a database.
+func TestOpen_MultipleGoroutines(t *testing.T) {
+	const (
+		instances  = 30
+		iterations = 30
+	)
+	path := tempfile()
+	defer os.RemoveAll(path)
+	var wg sync.WaitGroup
+	for iteration := 0; iteration < iterations; iteration++ {
+		for instance := 0; instance < instances; instance++ {
+			wg.Add(1)
+			go func() {
+				defer wg.Done()
+				db, err := bolt.Open(path, 0600, nil)
+				if err != nil {
+					t.Fatal(err)
+				}
+				if err := db.Close(); err != nil {
+					t.Fatal(err)
+				}
+			}()
+		}
+		wg.Wait()
+	}
+}
+
 // Ensure that opening a database with a blank path returns an error.
 func TestOpen_ErrPathRequired(t *testing.T) {
 	_, err := bolt.Open("", 0666, nil)
