addressing comments #1

Firestore/Example/Tests/Integration/FSTDatastoreTests.mm

@@ -160,7 +160,6 @@ @interface FSTDatastoreTests : XCTestCase
 @implementation FSTDatastoreTests {
   std::shared_ptr<AsyncQueue> _testWorkerQueue;
   FSTLocalStore *_localStore;
-  EmptyCredentialsProvider _credentials;
 
   DatabaseInfo _databaseInfo;
   std::shared_ptr<Datastore> _datastore;
@@ -184,8 +183,8 @@ - (void)setUp {
   dispatch_queue_t queue = dispatch_queue_create(
       "com.google.firestore.FSTDatastoreTestsWorkerQueue", DISPATCH_QUEUE_SERIAL);
   _testWorkerQueue = std::make_shared<AsyncQueue>(absl::make_unique<ExecutorLibdispatch>(queue));
-  _datastore = std::make_shared<Datastore>(
-      _databaseInfo, _testWorkerQueue, std::shared_ptr<EmptyCredentialsProvider>(&_credentials));
+  _datastore = std::make_shared<Datastore>(_databaseInfo, _testWorkerQueue,
+                                           std::make_shared<EmptyCredentialsProvider>());
 
   _remoteStore =
       absl::make_unique<RemoteStore>(_localStore, _datastore, _testWorkerQueue, [](OnlineState) {});

Firestore/Example/Tests/SpecTests/FSTMockDatastore.h

@@ -38,7 +38,7 @@ class MockDatastore : public Datastore {
  public:
   MockDatastore(const core::DatabaseInfo& database_info,
                 const std::shared_ptr<util::AsyncQueue>& worker_queue,
-                auth::CredentialsProvider* credentials);
+                std::shared_ptr<auth::CredentialsProvider> credentials);
 
   std::shared_ptr<WatchStream> CreateWatchStream(WatchStreamCallback* callback) override;
   std::shared_ptr<WriteStream> CreateWriteStream(WriteStreamCallback* callback) override;
@@ -93,7 +93,7 @@ class MockDatastore : public Datastore {
   // reduces the number of test-only methods in `Datastore`.
   const core::DatabaseInfo* database_info_ = nullptr;
   std::shared_ptr<util::AsyncQueue> worker_queue_;
-  auth::CredentialsProvider* credentials_ = nullptr;
+  std::shared_ptr<auth::CredentialsProvider> credentials_;
 
   std::shared_ptr<MockWatchStream> watch_stream_;
   std::shared_ptr<MockWriteStream> write_stream_;

Firestore/Example/Tests/SpecTests/FSTMockDatastore.mm

@@ -66,13 +66,12 @@
 class MockWatchStream : public WatchStream {
  public:
   MockWatchStream(const std::shared_ptr<AsyncQueue>& worker_queue,
-                  CredentialsProvider* credentials_provider,
+                  std::shared_ptr<CredentialsProvider> credentials_provider,
                   FSTSerializerBeta* serializer,
                   GrpcConnection* grpc_connection,
                   WatchStreamCallback* callback,
                   MockDatastore* datastore)
-      : WatchStream{worker_queue, std::shared_ptr<CredentialsProvider>(credentials_provider),
-                    serializer, grpc_connection, callback},
+      : WatchStream{worker_queue, credentials_provider, serializer, grpc_connection, callback},
         datastore_{datastore},
         callback_{callback} {
   }
@@ -158,13 +157,12 @@ void WriteWatchChange(const WatchChange& change, SnapshotVersion snap) {
 class MockWriteStream : public WriteStream {
  public:
   MockWriteStream(const std::shared_ptr<AsyncQueue>& worker_queue,
-                  CredentialsProvider* credentials_provider,
+                  std::shared_ptr<CredentialsProvider> credentials_provider,
                   FSTSerializerBeta* serializer,
                   GrpcConnection* grpc_connection,
                   WriteStreamCallback* callback,
                   MockDatastore* datastore)
-      : WriteStream{worker_queue, std::shared_ptr<CredentialsProvider>(credentials_provider),
-                    serializer, grpc_connection, callback},
+      : WriteStream{worker_queue, credentials_provider, serializer, grpc_connection, callback},
         datastore_{datastore},
         callback_{callback} {
   }
@@ -242,7 +240,7 @@ int sent_mutations_count() const {
 
 MockDatastore::MockDatastore(const core::DatabaseInfo& database_info,
                              const std::shared_ptr<util::AsyncQueue>& worker_queue,
-                             auth::CredentialsProvider* credentials)
+                             std::shared_ptr<auth::CredentialsProvider> credentials)
     : Datastore{database_info, worker_queue, std::shared_ptr<CredentialsProvider>(credentials),
                 CreateNoOpConnectivityMonitor()},
       database_info_{&database_info},

Firestore/Example/Tests/SpecTests/FSTSyncEngineTestDriver.mm

@@ -149,7 +149,6 @@ @implementation FSTSyncEngineTestDriver {
 
   DatabaseInfo _databaseInfo;
   User _currentUser;
-  EmptyCredentialsProvider _credentialProvider;
 
   std::shared_ptr<MockDatastore> _datastore;
 }
@@ -180,7 +179,8 @@ - (instancetype)initWithPersistence:(id<FSTPersistence>)persistence
     _persistence = persistence;
     _localStore = [[FSTLocalStore alloc] initWithPersistence:persistence initialUser:initialUser];
 
-    _datastore = std::make_shared<MockDatastore>(_databaseInfo, _workerQueue, &_credentialProvider);
+    _datastore = std::make_shared<MockDatastore>(_databaseInfo, _workerQueue,
+                                                 std::make_shared<EmptyCredentialsProvider>());
     _remoteStore = absl::make_unique<RemoteStore>(
         _localStore, _datastore, _workerQueue, [self](OnlineState onlineState) {
           [self.syncEngine applyChangedOnlineState:onlineState];

Firestore/Source/API/FIRFirestore+Internal.h

@@ -65,7 +65,7 @@ NS_ASSUME_NONNULL_BEGIN
 + (FIRFirestore *)recoverFromFirestore:(std::shared_ptr<api::Firestore>)firestore;
 
 /**
- * Shuts down this FirebaseFirestore instance.
+ * Shuts down this `FIRFirestore` instance.
  *
  * After shutdown only the `clearPersistence` method may be used. Any other method
  * will throw an error.

Firestore/Source/API/FSTFirestoreComponent.mm

@@ -124,10 +124,14 @@ - (void)removeInstance:(NSString *)database {
 #pragma mark - FIRComponentLifecycleMaintainer
 
 - (void)appWillBeDeleted:(FIRApp *)app {
-  for (NSString *key in _instances) {
-    [_instances[key] shutdownInternalWithCompletion:nil];
+  NSDictionary<NSString *, FIRFirestore *> *instances;
+  @synchronized(self.instances) {
+    instances = [_instances copy];
+    [_instances removeAllObjects];
+  }
+  for (NSString *key in instances) {
+    [instances[key] shutdownInternalWithCompletion:nil];
   }
-  [_instances removeAllObjects];
 }
 
 #pragma mark - Object Lifecycle

Firestore/Source/Core/FSTFirestoreClient.h

@@ -68,8 +68,8 @@ NS_ASSUME_NONNULL_BEGIN
  */
 + (instancetype)clientWithDatabaseInfo:(const core::DatabaseInfo &)databaseInfo
                               settings:(const api::Settings &)settings
-                   credentialsProvider:(std::shared_ptr<auth::CredentialsProvider>)
-                                           credentialsProvider  // no passing ownership
+                   credentialsProvider:
+                       (std::shared_ptr<auth::CredentialsProvider>)credentialsProvider
                           userExecutor:(std::shared_ptr<util::Executor>)userExecutor
                            workerQueue:(std::shared_ptr<util::AsyncQueue>)workerQueue;
 

Firestore/Source/Core/FSTFirestoreClient.mm

@@ -99,8 +99,7 @@ @interface FSTFirestoreClient () {
 
 - (instancetype)initWithDatabaseInfo:(const DatabaseInfo &)databaseInfo
                             settings:(const Settings &)settings
-                 credentialsProvider:(std::shared_ptr<CredentialsProvider>)
-                                         credentialsProvider  // no passing ownership
+                 credentialsProvider:(std::shared_ptr<CredentialsProvider>)credentialsProvider
                         userExecutor:(std::shared_ptr<Executor>)userExecutor
                          workerQueue:(std::shared_ptr<AsyncQueue>)queue NS_DESIGNATED_INITIALIZER;
 
@@ -148,8 +147,7 @@ - (bool)isShutdown {
 
 + (instancetype)clientWithDatabaseInfo:(const DatabaseInfo &)databaseInfo
                               settings:(const Settings &)settings
-                   credentialsProvider:(std::shared_ptr<CredentialsProvider>)
-                                           credentialsProvider  // no passing ownership
+                   credentialsProvider:(std::shared_ptr<CredentialsProvider>)credentialsProvider
                           userExecutor:(std::shared_ptr<Executor>)userExecutor
                            workerQueue:(std::shared_ptr<AsyncQueue>)workerQueue {
   return [[FSTFirestoreClient alloc] initWithDatabaseInfo:databaseInfo
@@ -161,8 +159,7 @@ + (instancetype)clientWithDatabaseInfo:(const DatabaseInfo &)databaseInfo
 
 - (instancetype)initWithDatabaseInfo:(const DatabaseInfo &)databaseInfo
                             settings:(const Settings &)settings
-                 credentialsProvider:(std::shared_ptr<CredentialsProvider>)
-                                         credentialsProvider  // no passing ownership
+                 credentialsProvider:(std::shared_ptr<CredentialsProvider>)credentialsProvider
                         userExecutor:(std::shared_ptr<Executor>)userExecutor
                          workerQueue:(std::shared_ptr<AsyncQueue>)workerQueue {
   if (self = [super init]) {
@@ -318,8 +315,9 @@ - (void)shutdownWithCallback:(util::StatusCallback)callback {
     _remoteStore->Shutdown();
     [self.persistence shutdown];
   });
+
   // This separate enqueue ensures if shutdown is called multiple times
-  // every time the callback is triggered. If it is in the previous
+  // every time the callback is triggered. If it is in the above
   // enqueue, it might not get executed because after first shutdown
   // all operations are not executed.
   _workerQueue->EnqueueEvenAfterShutdown([self, callback] {

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

@@ -99,6 +99,10 @@ explicit ConnectivityMonitorApple(
       return;
     }
 
+    // Reachability events are fairly infrequent, we avoid using dispatch queue
+    // from `work_queue` and use main queue here to avoid leaking implemetation
+    // details of `work_queue`. The callback itself is still executed on
+    // `work_queue`.
     success = SCNetworkReachabilitySetDispatchQueue(reachability_,
                                                     dispatch_get_main_queue());
     if (!success) {

Firestore/core/src/firebase/firestore/remote/datastore.h

@@ -76,7 +76,7 @@ class Datastore : public std::enable_shared_from_this<Datastore> {
 
   Datastore(const core::DatabaseInfo& database_info,
             const std::shared_ptr<util::AsyncQueue>& worker_queue,
-            const std::shared_ptr<auth::CredentialsProvider> credentials);
+            std::shared_ptr<auth::CredentialsProvider> credentials);
 
   virtual ~Datastore() {
   }
@@ -142,7 +142,7 @@ class Datastore : public std::enable_shared_from_this<Datastore> {
   /** Test-only constructor */
   Datastore(const core::DatabaseInfo& database_info,
             const std::shared_ptr<util::AsyncQueue>& worker_queue,
-            const std::shared_ptr<auth::CredentialsProvider> credentials,
+            std::shared_ptr<auth::CredentialsProvider> credentials,
             std::unique_ptr<ConnectivityMonitor> connectivity_monitor);
 
   /** Test-only method */

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

@@ -91,17 +91,17 @@ void LogGrpcCallFinished(absl::string_view rpc_name,
 
 Datastore::Datastore(const DatabaseInfo& database_info,
                      const std::shared_ptr<AsyncQueue>& worker_queue,
-                     const std::shared_ptr<CredentialsProvider> credentials)
+                     std::shared_ptr<CredentialsProvider> credentials)
     : Datastore{database_info, worker_queue, credentials,
                 ConnectivityMonitor::Create(worker_queue)} {
 }
 
 Datastore::Datastore(const DatabaseInfo& database_info,
                      const std::shared_ptr<AsyncQueue>& worker_queue,
-                     const std::shared_ptr<CredentialsProvider> credentials,
+                     std::shared_ptr<CredentialsProvider> credentials,
                      std::unique_ptr<ConnectivityMonitor> connectivity_monitor)
     : worker_queue_{NOT_NULL(worker_queue)},
-      credentials_{credentials},
+      credentials_{std::move(credentials)},
       rpc_executor_{CreateExecutor()},
       connectivity_monitor_{std::move(connectivity_monitor)},
       grpc_connection_{database_info, worker_queue, &grpc_queue_,

Firestore/core/src/firebase/firestore/remote/stream.h

@@ -120,7 +120,7 @@ class Stream : public GrpcStreamObserver,
   };
 
   Stream(const std::shared_ptr<util::AsyncQueue>& worker_queue,
-         const std::shared_ptr<auth::CredentialsProvider> credentials_provider,
+         std::shared_ptr<auth::CredentialsProvider> credentials_provider,
          GrpcConnection* grpc_connection,
          util::TimerId backoff_timer_id,
          util::TimerId idle_timer_id);

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

@@ -53,13 +53,13 @@
 }  // namespace
 
 Stream::Stream(const std::shared_ptr<AsyncQueue>& worker_queue,
-               const std::shared_ptr<CredentialsProvider> credentials_provider,
+               std::shared_ptr<CredentialsProvider> credentials_provider,
                GrpcConnection* grpc_connection,
                TimerId backoff_timer_id,
                TimerId idle_timer_id)
     : backoff_{worker_queue, backoff_timer_id, kBackoffFactor,
                kBackoffInitialDelay, kBackoffMaxDelay},
-      credentials_provider_{credentials_provider},
+      credentials_provider_{std::move(credentials_provider)},
       worker_queue_{worker_queue},
       grpc_connection_{grpc_connection},
       idle_timer_id_{idle_timer_id} {

Firestore/core/src/firebase/firestore/remote/watch_stream.h

@@ -81,12 +81,11 @@ class WatchStreamCallback {
  */
 class WatchStream : public Stream {
  public:
-  WatchStream(
-      const std::shared_ptr<util::AsyncQueue>& async_queue,
-      const std::shared_ptr<auth::CredentialsProvider> credentials_provider,
-      FSTSerializerBeta* serializer,
-      GrpcConnection* grpc_connection,
-      WatchStreamCallback* callback);
+  WatchStream(const std::shared_ptr<util::AsyncQueue>& async_queue,
+              std::shared_ptr<auth::CredentialsProvider> credentials_provider,
+              FSTSerializerBeta* serializer,
+              GrpcConnection* grpc_connection,
+              WatchStreamCallback* callback);
 
   /**
    * Registers interest in the results of the given query. If the query includes

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

@@ -37,7 +37,7 @@
 
 WatchStream::WatchStream(
     const std::shared_ptr<AsyncQueue>& async_queue,
-    const std::shared_ptr<CredentialsProvider> credentials_provider,
+    std::shared_ptr<CredentialsProvider> credentials_provider,
     FSTSerializerBeta* serializer,
     GrpcConnection* grpc_connection,
     WatchStreamCallback* callback)

Firestore/core/src/firebase/firestore/remote/write_stream.h

@@ -99,12 +99,11 @@ class WriteStreamCallback {
  */
 class WriteStream : public Stream {
  public:
-  WriteStream(
-      const std::shared_ptr<util::AsyncQueue>& async_queue,
-      const std::shared_ptr<auth::CredentialsProvider> credentials_provider,
-      FSTSerializerBeta* serializer,
-      GrpcConnection* grpc_connection,
-      WriteStreamCallback* callback);
+  WriteStream(const std::shared_ptr<util::AsyncQueue>& async_queue,
+              std::shared_ptr<auth::CredentialsProvider> credentials_provider,
+              FSTSerializerBeta* serializer,
+              GrpcConnection* grpc_connection,
+              WriteStreamCallback* callback);
 
   void SetLastStreamToken(NSData* token);
   /**

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

@@ -36,7 +36,7 @@
 
 WriteStream::WriteStream(
     const std::shared_ptr<AsyncQueue>& async_queue,
-    const std::shared_ptr<CredentialsProvider> credentials_provider,
+    std::shared_ptr<CredentialsProvider> credentials_provider,
     FSTSerializerBeta* serializer,
     GrpcConnection* grpc_connection,
     WriteStreamCallback* callback)

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

@@ -28,7 +28,6 @@ namespace util {
 AsyncQueue::AsyncQueue(std::unique_ptr<Executor> executor)
     : executor_{std::move(executor)} {
   is_operation_in_progress_ = false;
-  is_shutting_down_ = false;
 }
 
 // TODO(varconst): assert in destructor that the queue is empty.
@@ -50,10 +49,10 @@ void AsyncQueue::VerifyIsCurrentQueue() const {
 }
 
 void AsyncQueue::ExecuteBlocking(const Operation& operation) {
-  // This is not guarded by `shut_down_mutex_` and `is_shutting_down_`
-  // because it is the execution of the operation, not scheduling. Checking
-  // `is_shutting_down_` here would mean *all* operations will not run after
-  // shutdown, which is not expected.
+  // This is not guarded by `is_shutting_down_` because it is the execution
+  // of the operation, not scheduling. Checking `is_shutting_down_` here
+  // would mean *all* operations will not run after shutdown, which is not
+  // intended.
   VerifyIsCurrentExecutor();
   HARD_ASSERT(!is_operation_in_progress_,
               "ExecuteBlocking may not be called "
@@ -84,7 +83,7 @@ void AsyncQueue::EnqueueEvenAfterShutdown(const Operation& operation) {
   executor_->Execute(Wrap(operation));
 }
 
-bool AsyncQueue::is_shutting_down() {
+bool AsyncQueue::is_shutting_down() const {
   return is_shutting_down_;
 }
 

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

@@ -98,27 +98,25 @@ class AsyncQueue {
   // destroyed may invoke `Enqueue`).
   //
   // After the shutdown process has initiated (`is_shutting_down()` is true),
-  // calling `Enqueue` will return, but the operation will *NOT* be run.
+  // calling `Enqueue` is a no-op.
   void Enqueue(const Operation& operation);
 
   // Like `Enqueue`, but also starts the shutdown process. Once the shutdown
-  // process has started, this queue will not run any operations requested
-  // via `Enqueue*` methods. They will not report error, they will simply
-  // return with the requested operations not scheduled.
+  // process has started, calling any Enqueue* methods becomes a no-op
   //
   // The exception is `EnqueueEvenAfterShutdown`, operations requsted via
   // this will still be scheduled.
   void EnqueueAndInitializeShutdown(const Operation& operation);
 
   // Like `Enqueue`, but it will proceed scheduling the requested operation
-  // regardless if the queue is shut down or not.
+  // regardless of whether the queue is shut down or not.
   void EnqueueEvenAfterShutdown(const Operation& operation);
 
   // Like `Enqueue`, but without applying any prerequisite checks.
   void EnqueueRelaxed(const Operation& operation);
 
   // Whether the queue has initiated its shutdown process.
-  bool is_shutting_down();
+  bool is_shutting_down() const;
 
   // Puts the `operation` on the queue to be executed `delay` milliseconds from
   // now, and returns a handle that allows to cancel the operation (provided it
@@ -185,7 +183,7 @@ class AsyncQueue {
   std::atomic<bool> is_operation_in_progress_;
   std::unique_ptr<Executor> executor_;
 
-  bool is_shutting_down_;
+  bool is_shutting_down_ = false;
   mutable std::mutex shut_down_mutex_;
 };