addressing comments #3

Author: wu-hui

Firestore/Example/Tests/Integration/API/FIRDatabaseTests.mm

@@ -1331,13 +1331,7 @@ - (void)testRestartFirestoreLeadsToNewInstance {
       @{@"owner" : @{@"name" : @"Jonny", @"email" : @"[email protected]"}};
   [self writeDocumentRef:[firestore documentWithPath:@"abc/123"] data:data];
 
-  // Shutdown `firestore`.
-  XCTestExpectation *expectation = [self expectationWithDescription:@"shudown"];
-  [firestore shutdownWithCompletion:^(NSError *_Nullable error) {
-    XCTAssertNil(error);
-    [expectation fulfill];
-  }];
-  [self awaitExpectations];
+  [self shutdownFirestore:firestore];
 
   // Create a new instance, check it's a different instance.
   FIRFirestore *newInstance = [FIRFirestore firestoreForApp:app];

Firestore/Source/API/FIRFirestore+Internal.h

@@ -37,8 +37,8 @@ NS_ASSUME_NONNULL_BEGIN
 /** Provides a registry management interface for FIRFirestore instances. */
 @protocol FSTFirestoreInstanceRegistry
 
-/** Removes the FIRFirestore instance with given name from registry. */
-- (void)removeInstance:(NSString *)database;
+/** Removes the FIRFirestore instance with given database name from registry. */
+- (void)removeInstanceWithDatabase:(NSString *)database;
 
 @end
 
@@ -53,7 +53,7 @@ NS_ASSUME_NONNULL_BEGIN
                credentialsProvider:(std::shared_ptr<auth::CredentialsProvider>)credentialsProvider
                        workerQueue:(std::shared_ptr<util::AsyncQueue>)workerQueue
                        firebaseApp:(FIRApp *)app
-                  instanceRegistry:(id<FSTFirestoreInstanceRegistry>)registry;
+                  instanceRegistry:(nullable id<FSTFirestoreInstanceRegistry>)registry;
 @end
 
 /** Internal FIRFirestore API we don't want exposed in our public header files. */

Firestore/Source/API/FIRFirestore.mm

@@ -72,7 +72,7 @@ @interface FIRFirestore ()
 @implementation FIRFirestore {
   std::shared_ptr<Firestore> _firestore;
   FIRFirestoreSettings *_settings;
-  id<FSTFirestoreInstanceRegistry> _registry;
+  __weak id<FSTFirestoreInstanceRegistry> _registry;
 }
 
 + (instancetype)firestore {
@@ -110,7 +110,7 @@ - (instancetype)initWithDatabaseID:(model::DatabaseId)databaseID
                credentialsProvider:(std::shared_ptr<CredentialsProvider>)credentialsProvider
                        workerQueue:(std::shared_ptr<AsyncQueue>)workerQueue
                        firebaseApp:(FIRApp *)app
-                  instanceRegistry:(id<FSTFirestoreInstanceRegistry>)registry {
+                  instanceRegistry:(nullable id<FSTFirestoreInstanceRegistry>)registry {
   if (self = [super init]) {
     _firestore = std::make_shared<Firestore>(std::move(databaseID), std::move(persistenceKey),
                                              std::move(credentialsProvider), std::move(workerQueue),
@@ -306,8 +306,10 @@ - (void)shutdownInternalWithCompletion:(nullable void (^)(NSError *_Nullable err
 }
 
 - (void)shutdownWithCompletion:(nullable void (^)(NSError *_Nullable error))completion {
-  if (_registry) {
-    [_registry removeInstance:util::MakeNSString(_firestore->database_id().database_id())];
+  id<FSTFirestoreInstanceRegistry> strongRegistry = _registry;
+  if (strongRegistry) {
+    [strongRegistry
+        removeInstanceWithDatabase:util::MakeNSString(_firestore->database_id().database_id())];
   }
   [self shutdownInternalWithCompletion:completion];
 }

Firestore/Source/API/FSTFirestoreComponent.h

@@ -49,7 +49,7 @@ NS_ASSUME_NONNULL_BEGIN
 /// Default method for retrieving a Firestore instance, or creating one if it doesn't exist.
 - (FIRFirestore *)firestoreForDatabase:(NSString *)database;
 
-- (void)removeInstance:(NSString *)database;
+- (void)removeInstanceWithDatabase:(NSString *)database;
 
 /// Default initializer.
 - (instancetype)initWithApp:(FIRApp *)app NS_DESIGNATED_INITIALIZER;

Firestore/Source/API/FSTFirestoreComponent.mm

@@ -114,8 +114,8 @@ - (FIRFirestore *)firestoreForDatabase:(NSString *)database {
   }
 }
 
-- (void)removeInstance:(NSString *)database {
-  @synchronized(self.instances) {
+- (void)removeInstanceWithDatabase:(NSString *)database {
+  @synchronized(_instances) {
     NSString *key = [self keyForDatabase:database];
     [_instances removeObjectForKey:key];
   }
@@ -125,7 +125,7 @@ - (void)removeInstance:(NSString *)database {
 
 - (void)appWillBeDeleted:(FIRApp *)app {
   NSDictionary<NSString *, FIRFirestore *> *instances;
-  @synchronized(self.instances) {
+  @synchronized(_instances) {
     instances = [_instances copy];
     [_instances removeAllObjects];
   }

Firestore/Source/Core/FSTFirestoreClient.mm

@@ -305,7 +305,7 @@ - (void)enableNetworkWithCallback:(util::StatusCallback)callback {
 }
 
 - (void)shutdownWithCallback:(util::StatusCallback)callback {
-  _workerQueue->EnqueueAndInitializeShutdown([self, callback] {
+  _workerQueue->EnqueueAndInitiateShutdown([self, callback] {
     self->_credentialsProvider->SetCredentialChangeListener(nullptr);
 
     // If we've scheduled LRU garbage collection, cancel it.

Firestore/core/src/firebase/firestore/remote/connectivity_monitor_apple.mm

@@ -100,9 +100,9 @@ explicit ConnectivityMonitorApple(
     }
 
     // It's okay to use the main queue for reachability events because they are
-    // fairly infrequent,
-    // and there's no good way to get the underlying dispatch queue out of the
-    // worker queue. The callback itself is still executed on the worker queue.
+    // fairly infrequent, and there's no good way to get the underlying dispatch
+    // queue out of the worker queue. The callback itself is still executed on
+    // the worker queue.
     success = SCNetworkReachabilitySetDispatchQueue(reachability_,
                                                     dispatch_get_main_queue());
     if (!success) {

Firestore/core/src/firebase/firestore/util/async_queue.cc

@@ -68,7 +68,7 @@ void AsyncQueue::Enqueue(const Operation& operation) {
   EnqueueRelaxed(operation);
 }
 
-void AsyncQueue::EnqueueAndInitializeShutdown(const Operation& operation) {
+void AsyncQueue::EnqueueAndInitiateShutdown(const Operation& operation) {
   std::lock_guard<std::mutex> lock{shut_down_mutex_};
   VerifySequentialOrder();
   if (is_shutting_down_) {

Firestore/core/src/firebase/firestore/util/async_queue.h

@@ -106,7 +106,7 @@ class AsyncQueue {
   //
   // The exception is `EnqueueEvenAfterShutdown`, operations requsted via
   // this will still be scheduled.
-  void EnqueueAndInitializeShutdown(const Operation& operation);
+  void EnqueueAndInitiateShutdown(const Operation& operation);
 
   // Like `Enqueue`, but it will proceed scheduling the requested operation
   // regardless of whether the queue is shut down or not.

Firestore/core/test/firebase/firestore/util/async_queue_test.cc

@@ -187,7 +187,7 @@ TEST_P(AsyncQueueTest, CanScheduleOprationsWithRespectsToShutdownState) {
   std::string steps;
 
   queue.Enqueue([&] { steps += '1'; });
-  queue.EnqueueAndInitializeShutdown([&] { steps += '2'; });
+  queue.EnqueueAndInitiateShutdown([&] { steps += '2'; });
   queue.Enqueue([&] { steps += '3'; });
   queue.EnqueueEvenAfterShutdown([&] { steps += '4'; });
   queue.EnqueueEvenAfterShutdown([&] { signal_finished(); });