store: simplify db locking and functions

Instead of having a file lock to handle inter process locking and a sync.Mutex
to handle locking between multiple goroutines, just create, lock and close a
new file lock at every db.Do function.

Additionally, saving a sqldb instance in the db struct between open and close
doesn't makes great sense (and will create data races accessing sqldb, and that
was one of the reasons for the added sync.Mutex) since sqldb will be opened and
closed for every db.Do.

store/db.go

@@ -16,14 +16,10 @@ package store
 
 import (
 	"database/sql"
-	"errors"
-	"fmt"
 	"os"
 	"path/filepath"
-	"sync"
 
 	"github.com/coreos/rkt/pkg/lock"
-	"github.com/hashicorp/errwrap"
 
 	_ "github.com/cznic/ql/driver"
 )
@@ -32,133 +28,38 @@ const (
 	DbFilename = "ql.db"
 )
 
-// dbLock is used to guarantee both thread-safety and process-safety
-// for db access.
-type dbLock struct {
-	// This lock is to make that access to the ql db file being blocking
-	// since ql use an internal locking that will not block and return an
-	// error when a lock is already held.
-	fl *lock.FileLock
-	// This lock is to avoid concurrent access from multiple goroutines.
-	sync.Mutex
-}
-
-func newDBLock(dirPath string) (*dbLock, error) {
-	l, err := lock.NewLock(dirPath, lock.Dir)
-	if err != nil {
-		return nil, err
-	}
-	return &dbLock{fl: l}, nil
-}
-
-func (dl *dbLock) lock() error {
-	dl.Lock()
-	if err := dl.fl.ExclusiveLock(); err != nil {
-		dl.Unlock()
-		return err
-	}
-	return nil
-}
-
-func (dl *dbLock) unlock() {
-	if err := dl.fl.Unlock(); err != nil {
-		// TODO(sgotti) what'll happen when df.fl.Unlock fails? From
-		// man 2 flock looks like it'll happen only when the underlying
-		// fd has been closed (in this case the lock has been released
-		// when the fd has been closed, assuming no dup fd due to
-		// forking etc...).
-
-		// If there're other cases where it fails without unlocking,
-		// there's no simple way to handle them.
-		// Possible solutions:
-		// * panic (done here)
-		// * try to close the lock (and related fd), panic if close
-		// fails and create a new lock.
-		//
-		// Passing a specific error to the caller and let it recover
-		// creating a new store instance is tricky because we
-		// don't know the lock state and cannot be sure on how to clean
-		// this store instance
-
-		panic(fmt.Errorf("failed to unlock the db flock: %v", err))
-	}
-	dl.Unlock()
-}
-
-func (dl *dbLock) close() error {
-	return dl.fl.Close()
-}
-
 type DB struct {
 	dbdir string
-	dl    *dbLock
-	sqldb *sql.DB
 }
 
 func NewDB(dbdir string) (*DB, error) {
 	if err := os.MkdirAll(dbdir, defaultPathPerm); err != nil {
 		return nil, err
 	}
-
-	dl, err := newDBLock(dbdir)
-	if err != nil {
-		return nil, err
-	}
-
-	return &DB{dbdir: dbdir, dl: dl}, nil
-}
-
-func (db *DB) Open() error {
-	if err := db.dl.lock(); err != nil {
-		return err
-	}
-
-	sqldb, err := sql.Open("ql", filepath.Join(db.dbdir, DbFilename))
-	if err != nil {
-		db.dl.unlock()
-		return err
-	}
-	db.sqldb = sqldb
-
-	return nil
-}
-
-func (db *DB) Close() error {
-	if db.sqldb == nil {
-		panic("cas db, Close called without an open sqldb")
-	}
-
-	if err := db.sqldb.Close(); err != nil {
-		return errwrap.Wrap(errors.New("cas db close failed"), err)
-	}
-	db.sqldb = nil
-
-	// Don't close the flock as it will be reused.
-	db.dl.unlock()
-	return nil
-}
-
-func (db *DB) Begin() (*sql.Tx, error) {
-	return db.sqldb.Begin()
+	return &DB{dbdir: dbdir}, nil
 }
 
 type txfunc func(*sql.Tx) error
 
 // Do Opens the db, executes DoTx and then Closes the DB
+// To support a multiprocess and multigoroutine model on a single file access
+// database like ql there's the need to exlusively lock, open, close, unlock the
+// db for every transaction. For this reason every db transaction should be
+// fast to not block other processes/goroutines.
 func (db *DB) Do(fns ...txfunc) error {
-	err := db.Open()
+	l, err := lock.ExclusiveLock(db.dbdir, lock.Dir)
 	if err != nil {
 		return err
 	}
-	defer db.Close()
+	defer l.Close()
 
-	return db.DoTx(fns...)
-}
+	sqldb, err := sql.Open("ql", filepath.Join(db.dbdir, DbFilename))
+	if err != nil {
+		return err
+	}
+	defer sqldb.Close()
 
-// DoTx executes the provided txfuncs inside a unique transaction.
-// If one of the functions returns an error the whole transaction is rolled back.
-func (db *DB) DoTx(fns ...txfunc) error {
-	tx, err := db.Begin()
+	tx, err := sqldb.Begin()
 	if err != nil {
 		return err
 	}

store/db_test.go

@@ -78,10 +78,6 @@ func createTable(db *DB, t *testing.T) {
 }
 
 func TestDBRace(t *testing.T) {
-	// TODO(sgotti) this will not find concurrent accesses to ql db from
-	// multiple processes using multiple goroutines. A test that spawns at
-	// least two processes using multiple goroutines is needed.
-	// See https://github.com/coreos/rkt/pull/2391
 	oldGoMaxProcs := runtime.GOMAXPROCS(runtime.NumCPU())
 	defer runtime.GOMAXPROCS(oldGoMaxProcs)
 

store/store.go

@@ -310,13 +310,7 @@ func NewStore(baseDir string) (*Store, error) {
 
 // Close closes a Store opened with NewStore().
 func (s *Store) Close() error {
-	if err := s.storeLock.Close(); err != nil {
-		return err
-	}
-	if err := s.db.dl.close(); err != nil {
-		return err
-	}
-	return nil
+	return s.storeLock.Close()
 }
 
 // backupDB backs up current database.