Remove start from persistence interface (#2173)

* Remove start from persistence interface, switch FSTLevelDB to use a factory method that returns Status

Author: Greg Soltis

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

Firestore/Source/Core/FSTFirestoreClient.mm

@@ -199,27 +199,27 @@ - (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];
   } else {
     _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

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 <FSTPersistence, FSTTransactional>
 
 /**
- * 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
  */

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<std::unique_ptr<DB>> database = [self createDBWithDirectory:directory];
+  if (!database.status().ok()) {
+    return database.status();
+  }
+
+  std::unique_ptr<DB> ldb = std::move(database.ValueOrDie());
+  LevelDbMigrations::RunMigrations(ldb.get());
+  LevelDbTransaction transaction(ldb.get(), "Start LevelDB");
+  std::set<std::string> 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<leveldb::DB>)db
+                          users:(std::set<std::string>)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<std::unique_ptr<DB>> 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<std::unique_ptr<DB>>)createDBWithDirectory:(const Path &)directory {
++ (StatusOr<std::unique_ptr<DB>>)createDBWithDirectory:(const Path &)directory {
   Options options;
   options.create_if_missing = true;
 

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<FSTReferenceDelegate>)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!");

Firestore/Source/Local/FSTPersistence.h

@@ -65,13 +65,6 @@ NS_ASSUME_NONNULL_BEGIN
  */
 @protocol FSTPersistence <NSObject>
 
-/**
- * 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;