Suggested fixes for cpp/port_auth (#846)

* Get rid of MockDatastore factory This avoids the need to statically allocate (and leak) a credentials provider * Use absl::make_unique std::make_unique technically does not exist until C++14. * #include <utility> for std::move * Use std::future for the initial user
firebase · Feb 23, 2018 · 8df9ab4 · 8df9ab4
1 parent c4a8336
commit 8df9ab4
Show file tree

Hide file tree

Showing 6 changed files with 41 additions and 43 deletions.
diff --git a/Firestore/Example/Tests/SpecTests/FSTMockDatastore.h b/Firestore/Example/Tests/SpecTests/FSTMockDatastore.h
@@ -34,8 +34,6 @@ NS_ASSUME_NONNULL_BEGIN
  */
 @property(nonatomic) int writeStreamRequestCount;
 
-+ (instancetype)mockDatastoreWithWorkerDispatchQueue:(FSTDispatchQueue *)workerDispatchQueue;
-
 #pragma mark - Watch Stream manipulation.
 
 /** Injects an Added WatchChange containing the given targetIDs. */

diff --git a/Firestore/Example/Tests/SpecTests/FSTMockDatastore.mm b/Firestore/Example/Tests/SpecTests/FSTMockDatastore.mm
@@ -16,8 +16,6 @@
 
 #import "Firestore/Example/Tests/SpecTests/FSTMockDatastore.h"
 
-#include <list>
-
 #import "Firestore/Source/Core/FSTSnapshotVersion.h"
 #import "Firestore/Source/Local/FSTQueryData.h"
 #import "Firestore/Source/Model/FSTMutation.h"
@@ -290,20 +288,6 @@ @interface FSTMockDatastore ()
 
 @implementation FSTMockDatastore
 
-+ (instancetype)mockDatastoreWithWorkerDispatchQueue:(FSTDispatchQueue *)workerDispatchQueue {
-  // This owns the DatabaseInfos since we do not have FirestoreClient instance to own them.
-  static DatabaseInfo database_info{DatabaseId{"project", "database"}, "persistence", "host",
-                                    false};
-
-  // Note that we purposely don't bother to cleanup the EmptyCredentialsProvider instances.
-  static std::list<EmptyCredentialsProvider> credentials_providers;
-  credentials_providers.emplace_back();
-
-  return [[FSTMockDatastore alloc] initWithDatabaseInfo:&database_info
-                                    workerDispatchQueue:workerDispatchQueue
-                                            credentials:&credentials_providers.back()];
-}
-
 #pragma mark - Overridden FSTDatastore methods.
 
 - (FSTWatchStream *)createWatchStream {

diff --git a/Firestore/Example/Tests/SpecTests/FSTSyncEngineTestDriver.mm b/Firestore/Example/Tests/SpecTests/FSTSyncEngineTestDriver.mm
@@ -37,10 +37,16 @@
 #import "Firestore/Example/Tests/Core/FSTSyncEngine+Testing.h"
 #import "Firestore/Example/Tests/SpecTests/FSTMockDatastore.h"
 
+#include "Firestore/core/src/firebase/firestore/auth/empty_credentials_provider.h"
 #include "Firestore/core/src/firebase/firestore/auth/user.h"
+#include "Firestore/core/src/firebase/firestore/core/database_info.h"
+#include "Firestore/core/src/firebase/firestore/model/database_id.h"
 
+using firebase::firestore::auth::EmptyCredentialsProvider;
 using firebase::firestore::auth::HashUser;
 using firebase::firestore::auth::User;
+using firebase::firestore::core::DatabaseInfo;
+using firebase::firestore::model::DatabaseId;
 
 NS_ASSUME_NONNULL_BEGIN
 
@@ -83,7 +89,9 @@ @implementation FSTSyncEngineTestDriver {
   // ivar is declared as mutable.
   std::unordered_map<User, NSMutableArray<FSTOutstandingWrite *> *, HashUser> _outstandingWrites;
 
+  DatabaseInfo _databaseInfo;
   User _currentUser;
+  EmptyCredentialsProvider _credentialProvider;
 }
 
 - (instancetype)initWithPersistence:(id<FSTPersistence>)persistence
@@ -106,13 +114,17 @@ - (instancetype)initWithPersistence:(id<FSTPersistence>)persistence
 
     _events = [NSMutableArray array];
 
+    _databaseInfo = {DatabaseId{"project", "database"}, "persistence", "host", false};
+
     // Set up the sync engine and various stores.
     dispatch_queue_t mainQueue = dispatch_get_main_queue();
     FSTDispatchQueue *dispatchQueue = [FSTDispatchQueue queueWith:mainQueue];
     _localStore = [[FSTLocalStore alloc] initWithPersistence:persistence
                                             garbageCollector:garbageCollector
                                                  initialUser:initialUser];
-    _datastore = [FSTMockDatastore mockDatastoreWithWorkerDispatchQueue:dispatchQueue];
+    _datastore = [[FSTMockDatastore alloc] initWithDatabaseInfo:&_databaseInfo
+                                            workerDispatchQueue:dispatchQueue
+                                                    credentials:&_credentialProvider];
 
     _remoteStore = [FSTRemoteStore remoteStoreWithLocalStore:_localStore datastore:_datastore];
 

diff --git a/Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm b/Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm
@@ -17,6 +17,7 @@
 #import "Firestore/Example/Tests/Util/FSTIntegrationTestCase.h"
 
 #include <memory>
+#include <utility>
 
 #import <FirebaseCore/FIRLogger.h>
 #import <FirebaseFirestore/FirebaseFirestore-umbrella.h>
@@ -27,6 +28,7 @@
 #include "Firestore/core/src/firebase/firestore/model/database_id.h"
 #include "Firestore/core/src/firebase/firestore/util/autoid.h"
 #include "Firestore/core/src/firebase/firestore/util/string_apple.h"
+#include "absl/memory/memory.h"
 
 #import "Firestore/Source/API/FIRFirestore+Internal.h"
 #import "Firestore/Source/Core/FSTFirestoreClient.h"
@@ -141,7 +143,9 @@ - (FIRFirestore *)firestoreWithProjectID:(NSString *)projectID {
   FIRSetLoggerLevel(FIRLoggerLevelDebug);
   // HACK: FIRFirestore expects a non-nil app, but for tests we cheat.
   FIRApp *app = nil;
-  std::unique_ptr<CredentialsProvider> credentials_provider(new EmptyCredentialsProvider());
+  std::unique_ptr<CredentialsProvider> credentials_provider =
+      absl::make_unique<firebase::firestore::auth::EmptyCredentialsProvider>();
+
   FIRFirestore *firestore = [[FIRFirestore alloc] initWithProjectID:util::MakeStringView(projectID)
                                                            database:DatabaseId::kDefault
                                                      persistenceKey:persistenceKey

diff --git a/Firestore/Source/API/FIRFirestore.mm b/Firestore/Source/API/FIRFirestore.mm
@@ -17,6 +17,7 @@
 #import "FIRFirestore.h"
 
 #include <memory>
+#include <utility>
 
 #import <FirebaseCore/FIRApp.h>
 #import <FirebaseCore/FIRAppInternal.h>
@@ -44,6 +45,7 @@
 #include "Firestore/core/src/firebase/firestore/core/database_info.h"
 #include "Firestore/core/src/firebase/firestore/model/database_id.h"
 #include "Firestore/core/src/firebase/firestore/util/string_apple.h"
+#include "absl/memory/memory.h"
 
 namespace util = firebase::firestore::util;
 using firebase::firestore::auth::CredentialsProvider;
@@ -159,7 +161,7 @@ + (instancetype)firestoreForApp:(FIRApp *)app database:(NSString *)database {
           queueWith:dispatch_queue_create("com.google.firebase.firestore", DISPATCH_QUEUE_SERIAL)];
 
       std::unique_ptr<CredentialsProvider> credentials_provider =
-          std::make_unique<FirebaseCredentialsProvider>(app);
+          absl::make_unique<FirebaseCredentialsProvider>(app);
 
       NSString *persistenceKey = app.name;
 

diff --git a/Firestore/Source/Core/FSTFirestoreClient.mm b/Firestore/Source/Core/FSTFirestoreClient.mm
@@ -16,6 +16,8 @@
 
 #import "Firestore/Source/Core/FSTFirestoreClient.h"
 
+#import <future>
+
 #import "Firestore/Source/Core/FSTEventManager.h"
 #import "Firestore/Source/Core/FSTSyncEngine.h"
 #import "Firestore/Source/Core/FSTTransaction.h"
@@ -104,35 +106,31 @@ - (instancetype)initWithDatabaseInfo:(const DatabaseInfo &)databaseInfo
     _userDispatchQueue = userDispatchQueue;
     _workerDispatchQueue = workerDispatchQueue;
 
-    dispatch_semaphore_t initialUserAvailable = dispatch_semaphore_create(0);
-    __block bool initialized = false;
-    __block User initialUser;
-    FSTWeakify(self);
-    auto userChangeListener = ^(User user) {
-      FSTStrongify(self);
-      if (self) {
-        if (!initialized) {
-          initialUser = user;
-          initialized = true;
-          dispatch_semaphore_signal(initialUserAvailable);
-        } else {
-          [workerDispatchQueue dispatchAsync:^{
-            [self userDidChange:user];
-          }];
-        }
+    auto userPromise = std::make_shared<std::promise<User>>();
+
+    __weak typeof(self) weakSelf = self;
+    auto userChangeListener = [initialized = false, userPromise, weakSelf, workerDispatchQueue](User user) mutable {
+      typeof(self) strongSelf = weakSelf;
+      if (!strongSelf) return;
+
+      if (!initialized) {
+        initialized = true;
+        userPromise->set_value(user);
+      } else {
+        [workerDispatchQueue dispatchAsync:^{
+          [strongSelf userDidChange:user];
+        }];
       }
     };
 
-    _credentialsProvider->SetUserChangeListener(
-        [userChangeListener](const User &user) { userChangeListener(user); });
+    _credentialsProvider->SetUserChangeListener(userChangeListener);
 
     // Defer initialization until we get the current user from the userChangeListener. This is
     // guaranteed to be synchronously dispatched onto our worker queue, so we will be initialized
     // before any subsequently queued work runs.
     [_workerDispatchQueue dispatchAsync:^{
-      dispatch_semaphore_wait(initialUserAvailable, DISPATCH_TIME_FOREVER);
-
-      [self initializeWithUser:initialUser usePersistence:usePersistence];
+      User user = userPromise->get_future().get();
+      [self initializeWithUser:user usePersistence:usePersistence];
     }];
   }
   return self;
@@ -237,7 +235,7 @@ - (void)enableNetworkWithCompletion:(nullable FSTVoidErrorBlock)completion {
 
 - (void)shutdownWithCompletion:(nullable FSTVoidErrorBlock)completion {
   [self.workerDispatchQueue dispatchAsync:^{
-    _credentialsProvider->SetUserChangeListener(nullptr);
+    self->_credentialsProvider->SetUserChangeListener(nullptr);
 
     [self.remoteStore shutdown];
     [self.localStore shutdown];