Synchronise access to _databases dictionary (#430)

* Synchronise access to _databases dictionary * Add test - disabled by default * Synchronise `close` * Copyrights and CHANGELOG
cloudant · May 17, 2018 · b10098e · b10098e
1 parent ce9d891
commit b10098e
Show file tree

Hide file tree

Showing 4 changed files with 81 additions and 61 deletions.
diff --git a/CDTDatastore/touchdb/TD_DatabaseManager.h b/CDTDatastore/touchdb/TD_DatabaseManager.h
@@ -3,6 +3,7 @@
 //  TouchDB
 //
 //  Created by Jens Alfke on 3/22/12.
+//  Copyright © 2018 IBM Corporation. All rights reserved.
 //  Copyright (c) 2012 Couchbase, Inc. All rights reserved.
 //
 //  Modifications for this distribution by Cloudant, Inc., Copyright(c) 2014 Cloudant, Inc.
@@ -55,12 +56,6 @@ extern NSUInteger const kTD_DatabaseManagerErrorCodeInvalidName;
  */
 - (TD_Database*)databaseNamed:(NSString*)name;
 
-/**
- * Returns a database that was previously allocated with 'databaseNamed:'.
- * Be aware that the database may or may not be open.
- */
-- (TD_Database*)cachedDatabaseNamed:(NSString*)name;
-
 - (BOOL)deleteDatabaseNamed:(NSString*)name error:(NSError *__autoreleasing *)error;
 
 @property (readonly) NSArray* allDatabaseNames;

diff --git a/CDTDatastore/touchdb/TD_DatabaseManager.m b/CDTDatastore/touchdb/TD_DatabaseManager.m
@@ -3,6 +3,7 @@
 //  TouchDB
 //
 //  Created by Jens Alfke on 3/22/12.
+//  Copyright © 2018 IBM Corporation. All rights reserved.
 //  Copyright (c) 2012 Couchbase, Inc. All rights reserved.
 //
 //  Modifications for this distribution by Cloudant, Inc., Copyright (c) 2014 Cloudant, Inc.
@@ -123,66 +124,63 @@ - (NSString*)pathForName:(NSString*)name
 
 - (TD_Database*)databaseNamed:(NSString*)name
 {
-    TD_Database* db = [self cachedDatabaseNamed:name];
-    if (!db) {
-        NSString* path = [self pathForName:name];
-        if (!path) {
-            CDTLogError(CDTDATASTORE_LOG_CONTEXT, @"Database name not valid");
-        } else {
-            db = [[TD_Database alloc] initWithPath:path];
-            if (_options.readOnly && !db.exists) {
-                CDTLogDebug(CDTDATASTORE_LOG_CONTEXT, @"Read-only db does not exist. Return nil");
-
-                db = nil;
+    @synchronized(self) {
+        TD_Database* db = _databases[name];
+        if (!db) {
+            NSString* path = [self pathForName:name];
+            if (!path) {
+                CDTLogError(CDTDATASTORE_LOG_CONTEXT, @"Database name not valid");
             } else {
-                db.name = name;
-                db.readOnly = _options.readOnly;
-
-                _databases[name] = db;
+                db = [[TD_Database alloc] initWithPath:path];
+                if (_options.readOnly && !db.exists) {
+                    CDTLogDebug(CDTDATASTORE_LOG_CONTEXT, @"Read-only db does not exist. Return nil");
+
+                    db = nil;
+                } else {
+                    db.name = name;
+                    db.readOnly = _options.readOnly;
+
+                    _databases[name] = db;
+                }
             }
         }
+
+        return db;
     }
-
-    return db;
-}
-
-- (TD_Database*)cachedDatabaseNamed:(NSString*)name
-{
-    TD_Database* db = _databases[name];
-
-    return db;
 }
 
 - (BOOL)deleteDatabaseNamed:(NSString *)name error:(NSError *__autoreleasing *)error
 {
-    BOOL success = NO;
-
-    TD_Database *db = [self cachedDatabaseNamed:name];
-    if (db) {
-        // Do not simply delete the files, use instance method
-        success = [db deleteDatabase:error];
-        if (success) {
-            // Release cache
-            [_databases removeObjectForKey:name];
-        }
-    } else {
-        // Database not loaded in memory. Delete the files
-        NSString *path = [self pathForName:name];
-        if (path) {
-            success = [TD_Database deleteClosedDatabaseAtPath:path error:error];
-        } else if (error) {
-            NSDictionary *userInfo = @{
-                NSLocalizedDescriptionKey : NSLocalizedString(@"Couldn't delete database.", nil),
-                NSLocalizedFailureReasonErrorKey : NSLocalizedString(@"Invalid name?", nil),
-                NSLocalizedRecoverySuggestionErrorKey : NSLocalizedString(@"Invalid name?", nil)
-            };
-            *error = [NSError errorWithDomain:kTD_DatabaseManagerErrorDomain
-                                         code:kTD_DatabaseManagerErrorCodeInvalidName
-                                     userInfo:userInfo];
+    @synchronized(self) {
+        BOOL success = NO;
+
+        TD_Database *db = _databases[name];
+        if (db) {
+            // Do not simply delete the files, use instance method
+            success = [db deleteDatabase:error];
+            if (success) {
+                // Release cache
+                [_databases removeObjectForKey:name];
+            }
+        } else {
+            // Database not loaded in memory. Delete the files
+            NSString *path = [self pathForName:name];
+            if (path) {
+                success = [TD_Database deleteClosedDatabaseAtPath:path error:error];
+            } else if (error) {
+                NSDictionary *userInfo = @{
+                                           NSLocalizedDescriptionKey : NSLocalizedString(@"Couldn't delete database.", nil),
+                                           NSLocalizedFailureReasonErrorKey : NSLocalizedString(@"Invalid name?", nil),
+                                           NSLocalizedRecoverySuggestionErrorKey : NSLocalizedString(@"Invalid name?", nil)
+                                           };
+                *error = [NSError errorWithDomain:kTD_DatabaseManagerErrorDomain
+                                             code:kTD_DatabaseManagerErrorCodeInvalidName
+                                         userInfo:userInfo];
+            }
         }
+
+        return success;
     }
-
-    return success;
 }
 
 - (NSArray*)allDatabaseNames
@@ -200,12 +198,14 @@ - (NSArray*) allOpenDatabases {
 }
 
 - (void) close {
-    CDTLogInfo(CDTDATASTORE_LOG_CONTEXT, @"CLOSING %@ ...", self);
-    for (TD_Database* db in _databases.allValues) {
-        [db close];
+    @synchronized(self) {
+        CDTLogInfo(CDTDATASTORE_LOG_CONTEXT, @"CLOSING %@ ...", self);
+        for (TD_Database* db in _databases.allValues) {
+            [db close];
+        }
+        [_databases removeAllObjects];
+        CDTLogInfo(CDTDATASTORE_LOG_CONTEXT, @"CLOSED %@", self);
     }
-    [_databases removeAllObjects];
-    CDTLogInfo(CDTDATASTORE_LOG_CONTEXT, @"CLOSED %@", self);
 }
 
 @end
diff --git a/CDTDatastoreTests/DatastoreManagerTests.m b/CDTDatastoreTests/DatastoreManagerTests.m
@@ -145,6 +145,29 @@ - (void) testDatastoreClosesFilehandles {
 }
 #endif
 
+// test disabled because it takes a few minutes to run
+// re-enable to check for regressions in synchronisation of _databases dictionary in TD_DatabaseManager
+- (void) xxxTestDatastoreGetThreaded {
+    // store the TD_Database pointers, to ensure we always get the same one
+    NSMutableSet *dss = [NSMutableSet set];
+    int n = 200000;
+    // spawn `n` threads to simultaneously retrieve the same datastore
+    dispatch_group_t group = dispatch_group_create();
+    for (int i=0; i<n; i++) {
+        dispatch_group_async(group, dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_HIGH, 0),
+                             ^(void){
+                                 NSError *err;
+                                 CDTDatastore *ds = [self.factory datastoreNamed:@"test" error:&err];
+                                 // add the TD_Database to the set
+                                 [dss addObject:[ds database]];
+                                 XCTAssertNil(err);
+                             });
+    }
+    dispatch_group_wait(group, DISPATCH_TIME_FOREVER);
+    // we should only ever get one TD_Database pointer
+    XCTAssertEqual([dss count], 1);
+}
+
 @end
 
 
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3,6 +3,8 @@
 ## Unreleased
 - [FIXED] Bug which prevented CDTDatastore instances from being closed after calling index/query
   methods.
+- [FIXED] Race condition which under some circumstances could return different `CDTDatastore`
+  instances for the same open database.
 
 ## 2.0.2 (2018-04-30)
 - [FIXED] Bug which prevented `find` queries from executing