diff --git a/Firestore/Example/Tests/Local/FSTPersistenceTestHelpers.mm b/Firestore/Example/Tests/Local/FSTPersistenceTestHelpers.mm index 74059c33d61..dcbef0d9427 100644 --- a/Firestore/Example/Tests/Local/FSTPersistenceTestHelpers.mm +++ b/Firestore/Example/Tests/Local/FSTPersistenceTestHelpers.mm @@ -68,15 +68,14 @@ + (FSTLevelDB *)levelDBPersistenceWithDir:(Path)dir { + (FSTLevelDB *)levelDBPersistenceWithDir:(Path)dir lruParams:(LruParams)params { FSTLocalSerializer *serializer = [self localSerializer]; - FSTLevelDB *db = - [[FSTLevelDB alloc] initWithDirectory:std::move(dir) serializer:serializer lruParams:params]; - Status status = [db start]; + FSTLevelDB *ldb; + util::Status status = + [FSTLevelDB dbWithDirectory:std::move(dir) serializer:serializer lruParams:params ptr:&ldb]; if (!status.ok()) { [NSException raise:NSInternalInconsistencyException - format:@"Failed to start leveldb persistence: %s", status.ToString().c_str()]; + format:@"Failed to open DB: %s", status.ToString().c_str()]; } - - return db; + return ldb; } + (FSTLevelDB *)levelDBPersistenceWithLruParams:(LruParams)lruParams { @@ -88,14 +87,7 @@ + (FSTLevelDB *)levelDBPersistence { } + (FSTMemoryPersistence *)eagerGCMemoryPersistence { - FSTMemoryPersistence *persistence = [FSTMemoryPersistence persistenceWithEagerGC]; - Status status = [persistence start]; - if (!status.ok()) { - [NSException raise:NSInternalInconsistencyException - format:@"Failed to start memory persistence: %s", status.ToString().c_str()]; - } - - return persistence; + return [FSTMemoryPersistence persistenceWithEagerGC]; } + (FSTMemoryPersistence *)lruMemoryPersistence { @@ -104,15 +96,7 @@ + (FSTMemoryPersistence *)lruMemoryPersistence { + (FSTMemoryPersistence *)lruMemoryPersistenceWithLruParams:(LruParams)lruParams { FSTLocalSerializer *serializer = [self localSerializer]; - FSTMemoryPersistence *persistence = - [FSTMemoryPersistence persistenceWithLruParams:lruParams serializer:serializer]; - Status status = [persistence start]; - if (!status.ok()) { - [NSException raise:NSInternalInconsistencyException - format:@"Failed to start memory persistence: %s", status.ToString().c_str()]; - } - - return persistence; + return [FSTMemoryPersistence persistenceWithLruParams:lruParams serializer:serializer]; } @end diff --git a/Firestore/Source/Core/FSTFirestoreClient.mm b/Firestore/Source/Core/FSTFirestoreClient.mm index acb16e3644c..d0acd44514c 100644 --- a/Firestore/Source/Core/FSTFirestoreClient.mm +++ b/Firestore/Source/Core/FSTFirestoreClient.mm @@ -199,10 +199,20 @@ - (void)initializeWithUser:(const User &)user settings:(FIRFirestoreSettings *)s [[FSTSerializerBeta alloc] initWithDatabaseID:&self.databaseInfo->database_id()]; FSTLocalSerializer *serializer = [[FSTLocalSerializer alloc] initWithRemoteSerializer:remoteSerializer]; - FSTLevelDB *ldb = - [[FSTLevelDB alloc] initWithDirectory:std::move(dir) - serializer:serializer - lruParams:LruParams::WithCacheSize(settings.cacheSizeBytes)]; + FSTLevelDB *ldb; + Status levelDbStatus = + [FSTLevelDB dbWithDirectory:std::move(dir) + serializer:serializer + lruParams:LruParams::WithCacheSize(settings.cacheSizeBytes) + ptr:&ldb]; + if (!levelDbStatus.ok()) { + // If leveldb fails to start then just throw up our hands: the error is unrecoverable. + // There's nothing an end-user can do and nearly all failures indicate the developer is doing + // something grossly wrong so we should stop them cold in their tracks with a failure they + // can't ignore. + [NSException raise:NSInternalInconsistencyException + format:@"Failed to open DB: %s", levelDbStatus.ToString().c_str()]; + } _lruDelegate = ldb.referenceDelegate; _persistence = ldb; [self scheduleLruGarbageCollection]; @@ -210,16 +220,6 @@ - (void)initializeWithUser:(const User &)user settings:(FIRFirestoreSettings *)s _persistence = [FSTMemoryPersistence persistenceWithEagerGC]; } - Status status = [_persistence start]; - if (!status.ok()) { - // If local storage fails to start then just throw up our hands: the error is unrecoverable. - // There's nothing an end-user can do and nearly all failures indicate the developer is doing - // something grossly wrong so we should stop them cold in their tracks with a failure they - // can't ignore. - [NSException raise:NSInternalInconsistencyException - format:@"Failed to open DB: %s", status.ToString().c_str()]; - } - _localStore = [[FSTLocalStore alloc] initWithPersistence:_persistence initialUser:user]; FSTDatastore *datastore = [FSTDatastore datastoreWithDatabase:self.databaseInfo diff --git a/Firestore/Source/Local/FSTLevelDB.h b/Firestore/Source/Local/FSTLevelDB.h index 3128587a463..b629d4861f1 100644 --- a/Firestore/Source/Local/FSTLevelDB.h +++ b/Firestore/Source/Local/FSTLevelDB.h @@ -26,6 +26,7 @@ #include "Firestore/core/src/firebase/firestore/local/leveldb_transaction.h" #include "Firestore/core/src/firebase/firestore/util/path.h" #include "Firestore/core/src/firebase/firestore/util/status.h" +#include "Firestore/core/src/firebase/firestore/util/statusor.h" #include "leveldb/db.h" @class FSTLocalSerializer; @@ -40,13 +41,16 @@ NS_ASSUME_NONNULL_BEGIN @interface FSTLevelDB : NSObject /** - * Initializes the LevelDB in the given directory. Note that all expensive startup work including - * opening any database files is deferred until -[FSTPersistence start] is called. + * Creates a LevelDB in the given directory and sets `ptr` to point to it. Return value indicates + * success in creating the leveldb instance and must be checked before accessing `ptr`. C++ note: + * Once FSTLevelDB is ported to C++, this factory method should return StatusOr<>. It cannot + * currently do that because ObjC references are not allowed in StatusOr. */ -- (instancetype)initWithDirectory:(firebase::firestore::util::Path)directory - serializer:(FSTLocalSerializer *)serializer - lruParams:(firebase::firestore::local::LruParams)lruParams - NS_DESIGNATED_INITIALIZER; ++ (firebase::firestore::util::Status)dbWithDirectory:(firebase::firestore::util::Path)directory + serializer:(FSTLocalSerializer *)serializer + lruParams: + (firebase::firestore::local::LruParams)lruParams + ptr:(FSTLevelDB **)ptr; - (instancetype)init NS_UNAVAILABLE; @@ -65,15 +69,6 @@ NS_ASSUME_NONNULL_BEGIN storageDirectoryForDatabaseInfo:(const firebase::firestore::core::DatabaseInfo &)databaseInfo documentsDirectory:(const firebase::firestore::util::Path &)documentsDirectory; -/** - * Starts LevelDB-backed persistent storage by opening the database files, creating the DB if it - * does not exist. - * - * The leveldb directory is created relative to the appropriate document storage directory for the - * platform: NSDocumentDirectory on iOS or $HOME/.firestore on macOS. - */ -- (firebase::firestore::util::Status)start; - /** * @return A standard set of read options */ diff --git a/Firestore/Source/Local/FSTLevelDB.mm b/Firestore/Source/Local/FSTLevelDB.mm index 3087d244b49..116bb65b903 100644 --- a/Firestore/Source/Local/FSTLevelDB.mm +++ b/Firestore/Source/Local/FSTLevelDB.mm @@ -299,16 +299,50 @@ + (const ReadOptions)standardReadOptions { return users; } -- (instancetype)initWithDirectory:(firebase::firestore::util::Path)directory - serializer:(FSTLocalSerializer *)serializer - lruParams:(firebase::firestore::local::LruParams)lruParams { ++ (firebase::firestore::util::Status)dbWithDirectory:(firebase::firestore::util::Path)directory + serializer:(FSTLocalSerializer *)serializer + lruParams: + (firebase::firestore::local::LruParams)lruParams + ptr:(FSTLevelDB **)ptr { + Status status = [self ensureDirectory:directory]; + if (!status.ok()) return status; + + StatusOr> database = [self createDBWithDirectory:directory]; + if (!database.status().ok()) { + return database.status(); + } + + std::unique_ptr ldb = std::move(database.ValueOrDie()); + LevelDbMigrations::RunMigrations(ldb.get()); + LevelDbTransaction transaction(ldb.get(), "Start LevelDB"); + std::set users = [self collectUserSet:&transaction]; + transaction.Commit(); + FSTLevelDB *db = [[self alloc] initWithLevelDB:std::move(ldb) + users:users + directory:directory + serializer:serializer + lruParams:lruParams]; + *ptr = db; + return Status::OK(); +} + +- (instancetype)initWithLevelDB:(std::unique_ptr)db + users:(std::set)users + directory:(firebase::firestore::util::Path)directory + serializer:(FSTLocalSerializer *)serializer + lruParams:(firebase::firestore::local::LruParams)lruParams { if (self = [super init]) { + self.started = YES; + _ptr = std::move(db); _directory = std::move(directory); _serializer = serializer; _queryCache = [[FSTLevelDBQueryCache alloc] initWithDB:self serializer:self.serializer]; _referenceDelegate = [[FSTLevelDBLRUDelegate alloc] initWithPersistence:self lruParams:lruParams]; _transactionRunner.SetBackingPersistence(self); + _users = std::move(users); + [_queryCache start]; + [_referenceDelegate start]; } return self; } @@ -376,30 +410,8 @@ + (Path)storageDirectoryForDatabaseInfo:(const DatabaseInfo &)databaseInfo #pragma mark - Startup -- (Status)start { - HARD_ASSERT(!self.isStarted, "FSTLevelDB double-started!"); - self.started = YES; - - Status status = [self ensureDirectory:_directory]; - if (!status.ok()) return status; - - StatusOr> database = [self createDBWithDirectory:_directory]; - if (!database.status().ok()) { - return database.status(); - } - _ptr = std::move(database).ValueOrDie(); - - LevelDbMigrations::RunMigrations(_ptr.get()); - LevelDbTransaction transaction(_ptr.get(), "Start LevelDB"); - _users = [FSTLevelDB collectUserSet:&transaction]; - transaction.Commit(); - [_queryCache start]; - [_referenceDelegate start]; - return Status::OK(); -} - /** Creates the directory at @a directory and marks it as excluded from iCloud backup. */ -- (Status)ensureDirectory:(const Path &)directory { ++ (Status)ensureDirectory:(const Path &)directory { Status status = util::RecursivelyCreateDir(directory); if (!status.ok()) { return Status{FirestoreErrorCode::Internal, "Failed to create persistence directory"}.CausedBy( @@ -418,7 +430,7 @@ - (Status)ensureDirectory:(const Path &)directory { } /** Opens the database within the given directory. */ -- (StatusOr>)createDBWithDirectory:(const Path &)directory { ++ (StatusOr>)createDBWithDirectory:(const Path &)directory { Options options; options.create_if_missing = true; diff --git a/Firestore/Source/Local/FSTMemoryPersistence.mm b/Firestore/Source/Local/FSTMemoryPersistence.mm index e639ecc0de0..6aa8d6738e8 100644 --- a/Firestore/Source/Local/FSTMemoryPersistence.mm +++ b/Firestore/Source/Local/FSTMemoryPersistence.mm @@ -99,6 +99,7 @@ - (instancetype)init { if (self = [super init]) { _queryCache = [[FSTMemoryQueryCache alloc] initWithPersistence:self]; _remoteDocumentCache = [[FSTMemoryRemoteDocumentCache alloc] init]; + self.started = YES; } return self; } @@ -111,13 +112,6 @@ - (void)setReferenceDelegate:(id)referenceDelegate { } } -- (Status)start { - // No durable state to read on startup. - HARD_ASSERT(!self.isStarted, "FSTMemoryPersistence double-started!"); - self.started = YES; - return Status::OK(); -} - - (void)shutdown { // No durable state to ensure is closed on shutdown. HARD_ASSERT(self.isStarted, "FSTMemoryPersistence shutdown without start!"); diff --git a/Firestore/Source/Local/FSTPersistence.h b/Firestore/Source/Local/FSTPersistence.h index 4141d375dfc..4b106af5080 100644 --- a/Firestore/Source/Local/FSTPersistence.h +++ b/Firestore/Source/Local/FSTPersistence.h @@ -65,13 +65,6 @@ NS_ASSUME_NONNULL_BEGIN */ @protocol FSTPersistence -/** - * Starts persistent storage, opening the database or similar. - * - * @return A Status object that will be populated with an error message if startup fails. - */ -- (firebase::firestore::util::Status)start; - /** Releases any resources held during eager shutdown. */ - (void)shutdown;