From b10098e61c30021e3201895ff8abd6497e8cf7db Mon Sep 17 00:00:00 2001 From: Tom Blench Date: Thu, 17 May 2018 14:23:15 +0100 Subject: [PATCH] Synchronise access to _databases dictionary (#430) * Synchronise access to _databases dictionary * Add test - disabled by default * Synchronise `close` * Copyrights and CHANGELOG --- CDTDatastore/touchdb/TD_DatabaseManager.h | 7 +- CDTDatastore/touchdb/TD_DatabaseManager.m | 110 +++++++++++----------- CDTDatastoreTests/DatastoreManagerTests.m | 23 +++++ CHANGELOG.md | 2 + 4 files changed, 81 insertions(+), 61 deletions(-) diff --git a/CDTDatastore/touchdb/TD_DatabaseManager.h b/CDTDatastore/touchdb/TD_DatabaseManager.h index e74d68c8d..af46c19b7 100644 --- 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 index 6155966f4..cb938d2df 100644 --- 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 index e5f7cb6e8..b87354bcc 100644 --- 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