Browse Source

Block datastore closing until open transactions complete

- Avoids the following race condition detected in bolt db:

=== RUN   TestObfuscatedRemoteServerLists

WARNING: DATA RACE
Read at 0x00c00015c240 by goroutine 58:
  github.com/Psiphon-Labs/psiphon-tunnel-core/vendor/github.com/Psiphon-Labs/bolt.(*Bucket).pageNode()
      /gopath/src/github.com/Psiphon-Labs/psiphon-tunnel-core/vendor/github.com/Psiphon-Labs/bolt/db.go:796 +0x224
  github.com/Psiphon-Labs/psiphon-tunnel-core/vendor/github.com/Psiphon-Labs/bolt.(*Cursor).search()
      /gopath/src/github.com/Psiphon-Labs/psiphon-tunnel-core/vendor/github.com/Psiphon-Labs/bolt/cursor.go:254 +0x73
  [...]
      /gopath/src/github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon/dataStore_bolt.go:146 +0x77
  github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon.datastoreView()
      /gopath/src/github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon/dataStore.go:115 +0x87
  github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon.CountUnreportedPersistentStats()
  [...]

Previous write at 0x00c00015c240 by goroutine 74:
  github.com/Psiphon-Labs/psiphon-tunnel-core/vendor/github.com/Psiphon-Labs/bolt.munmap()
      /home/travis/gopath/src/github.com/Psiphon-Labs/psiphon-tunnel-core/vendor/github.com/Psiphon-Labs/bolt/bolt_unix.go:77 +0xde
  github.com/Psiphon-Labs/psiphon-tunnel-core/vendor/github.com/Psiphon-Labs/bolt.(*DB).munmap()
      /home/travis/gopath/src/github.com/Psiphon-Labs/psiphon-tunnel-core/vendor/github.com/Psiphon-Labs/bolt/db.go:299 +0x3c
  [...]
  github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon.(*datastoreDB).close()
      /home/travis/gopath/src/github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon/dataStore_bolt.go:142 +0x4b
  github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon.CloseDataStore()
      /home/travis/gopath/src/github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon/dataStore.go:97 +0xe4
  github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon.testObfuscatedRemoteServerLists()
      /home/travis/gopath/src/github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon/remoteServerList_test.go:462 +0x2946
  [...]
Rod Hynes 7 years ago
parent
commit
f9a29c97e1
1 changed files with 17 additions and 24 deletions
  1. 17 24
      psiphon/dataStore.go

+ 17 - 24
psiphon/dataStore.go

@@ -48,33 +48,31 @@ var (
 	datastorePersistentStatTypeRemoteServerList = string(datastoreRemoteServerListStatsBucket)
 	datastoreServerEntryFetchGCThreshold        = 20
 
-	datastoreInitalizeMutex sync.Mutex
-	datastoreReferenceMutex sync.Mutex
-	activeDatastoreDB       *datastoreDB
+	datastoreMutex    sync.RWMutex
+	activeDatastoreDB *datastoreDB
 )
 
 // OpenDataStore opens and initializes the singleton data store instance.
 func OpenDataStore(config *Config) error {
 
-	datastoreInitalizeMutex.Lock()
-	defer datastoreInitalizeMutex.Unlock()
+	datastoreMutex.Lock()
 
-	datastoreReferenceMutex.Lock()
 	existingDB := activeDatastoreDB
-	datastoreReferenceMutex.Unlock()
 
 	if existingDB != nil {
+		datastoreMutex.Unlock()
 		return common.ContextError(errors.New("db already open"))
 	}
 
 	newDB, err := datastoreOpenDB(config.DataStoreDirectory)
 	if err != nil {
+		datastoreMutex.Unlock()
 		return common.ContextError(err)
 	}
 
-	datastoreReferenceMutex.Lock()
 	activeDatastoreDB = newDB
-	datastoreReferenceMutex.Unlock()
+
+	datastoreMutex.Unlock()
 
 	_ = resetAllPersistentStatsToUnreported()
 
@@ -84,11 +82,8 @@ func OpenDataStore(config *Config) error {
 // CloseDataStore closes the singleton data store instance, if open.
 func CloseDataStore() {
 
-	datastoreInitalizeMutex.Lock()
-	defer datastoreInitalizeMutex.Unlock()
-
-	datastoreReferenceMutex.Lock()
-	defer datastoreReferenceMutex.Unlock()
+	datastoreMutex.Lock()
+	defer datastoreMutex.Unlock()
 
 	if activeDatastoreDB == nil {
 		return
@@ -104,15 +99,14 @@ func CloseDataStore() {
 
 func datastoreView(fn func(tx *datastoreTx) error) error {
 
-	datastoreReferenceMutex.Lock()
-	db := activeDatastoreDB
-	datastoreReferenceMutex.Unlock()
+	datastoreMutex.RLock()
+	defer datastoreMutex.RUnlock()
 
-	if db == nil {
+	if activeDatastoreDB == nil {
 		return common.ContextError(errors.New("database not open"))
 	}
 
-	err := db.view(fn)
+	err := activeDatastoreDB.view(fn)
 	if err != nil {
 		err = common.ContextError(err)
 	}
@@ -121,15 +115,14 @@ func datastoreView(fn func(tx *datastoreTx) error) error {
 
 func datastoreUpdate(fn func(tx *datastoreTx) error) error {
 
-	datastoreReferenceMutex.Lock()
-	db := activeDatastoreDB
-	datastoreReferenceMutex.Unlock()
+	datastoreMutex.RLock()
+	defer datastoreMutex.RUnlock()
 
-	if db == nil {
+	if activeDatastoreDB == nil {
 		return common.ContextError(errors.New("database not open"))
 	}
 
-	err := db.update(fn)
+	err := activeDatastoreDB.update(fn)
 	if err != nil {
 		err = common.ContextError(err)
 	}