Reset backoff when connectivity status changes (#5857)

Firestore/CHANGELOG.md

@@ -1,3 +1,8 @@
+# Unreleased
+- [fixed] Removed a delay that may have prevented Firestore from immediately
+  establishing a network connection if a connectivity change occurred while
+  the app was in the background.
+
 # v1.16.0
 - [fixed] Fixed an issue that may have prevented the client from connecting
   to the backend immediately after a user signed in.

Firestore/Example/Tests/Integration/FSTDatastoreTests.mm

@@ -39,6 +39,7 @@
 #include "Firestore/core/src/model/mutation_batch_result.h"
 #include "Firestore/core/src/model/precondition.h"
 #include "Firestore/core/src/model/set_mutation.h"
+#include "Firestore/core/src/remote/connectivity_monitor.h"
 #include "Firestore/core/src/remote/datastore.h"
 #include "Firestore/core/src/remote/remote_event.h"
 #include "Firestore/core/src/remote/remote_store.h"
@@ -46,6 +47,7 @@
 #include "Firestore/core/src/util/hard_assert.h"
 #include "Firestore/core/src/util/status.h"
 #include "Firestore/core/src/util/string_apple.h"
+#include "Firestore/core/test/unit/remote/create_noop_connectivity_monitor.h"
 #include "Firestore/core/test/unit/testutil/async_testing.h"
 #include "Firestore/core/test/unit/testutil/testutil.h"
 #include "absl/memory/memory.h"
@@ -72,6 +74,8 @@ using firebase::firestore::model::MutationBatchResult;
 using firebase::firestore::model::Precondition;
 using firebase::firestore::model::OnlineState;
 using firebase::firestore::model::TargetId;
+using firebase::firestore::remote::ConnectivityMonitor;
+using firebase::firestore::remote::CreateNoOpConnectivityMonitor;
 using firebase::firestore::remote::Datastore;
 using firebase::firestore::remote::GrpcConnection;
 using firebase::firestore::remote::RemoteEvent;
@@ -219,6 +223,8 @@ class RemoteStoreEventCapture : public RemoteStoreCallback {
 
   DatabaseInfo _databaseInfo;
   SimpleQueryEngine _queryEngine;
+
+  std::unique_ptr<ConnectivityMonitor> _connectivityMonitor;
   std::shared_ptr<Datastore> _datastore;
   std::unique_ptr<RemoteStore> _remoteStore;
 }
@@ -238,15 +244,17 @@ class RemoteStoreEventCapture : public RemoteStoreCallback {
       DatabaseInfo(database_id, "test-key", util::MakeString(settings.host), settings.sslEnabled);
 
   _testWorkerQueue = testutil::AsyncQueueForTesting();
+  _connectivityMonitor = CreateNoOpConnectivityMonitor();
   _datastore = std::make_shared<Datastore>(_databaseInfo, _testWorkerQueue,
-                                           std::make_shared<EmptyCredentialsProvider>());
+                                           std::make_shared<EmptyCredentialsProvider>(),
+                                           _connectivityMonitor.get());
 
   _persistence = MemoryPersistence::WithEagerGarbageCollector();
   _localStore =
       absl::make_unique<LocalStore>(_persistence.get(), &_queryEngine, User::Unauthenticated());
 
   _remoteStore = absl::make_unique<RemoteStore>(_localStore.get(), _datastore, _testWorkerQueue,
-                                                [](OnlineState) {});
+                                                _connectivityMonitor.get(), [](OnlineState) {});
 
   _testWorkerQueue->Enqueue([=] { _remoteStore->Start(); });
 }

Firestore/Example/Tests/SpecTests/FSTMockDatastore.h

@@ -30,6 +30,7 @@ namespace firebase {
 namespace firestore {
 namespace remote {
 
+class ConnectivityMonitor;
 class MockWatchStream;
 class MockWriteStream;
 
@@ -37,7 +38,8 @@ class MockDatastore : public Datastore {
  public:
   MockDatastore(const core::DatabaseInfo& database_info,
                 const std::shared_ptr<util::AsyncQueue>& worker_queue,
-                std::shared_ptr<auth::CredentialsProvider> credentials);
+                std::shared_ptr<auth::CredentialsProvider> credentials,
+                ConnectivityMonitor* connectivity_monitor);
 
   std::shared_ptr<WatchStream> CreateWatchStream(WatchStreamCallback* callback) override;
   std::shared_ptr<WriteStream> CreateWriteStream(WriteStreamCallback* callback) override;

Firestore/Example/Tests/SpecTests/FSTMockDatastore.mm

@@ -51,7 +51,6 @@ using firebase::firestore::model::MutationResult;
 using firebase::firestore::model::SnapshotVersion;
 using firebase::firestore::model::TargetId;
 using firebase::firestore::remote::ConnectivityMonitor;
-using firebase::firestore::remote::CreateNoOpConnectivityMonitor;
 using firebase::firestore::remote::GrpcConnection;
 using firebase::firestore::remote::WatchChange;
 using firebase::firestore::remote::WatchStream;
@@ -243,8 +242,9 @@ class MockWriteStream : public WriteStream {
 
 MockDatastore::MockDatastore(const core::DatabaseInfo& database_info,
                              const std::shared_ptr<util::AsyncQueue>& worker_queue,
-                             std::shared_ptr<auth::CredentialsProvider> credentials)
-    : Datastore{database_info, worker_queue, credentials, CreateNoOpConnectivityMonitor()},
+                             std::shared_ptr<auth::CredentialsProvider> credentials,
+                             ConnectivityMonitor* connectivity_monitor)
+    : Datastore{database_info, worker_queue, credentials, connectivity_monitor},
       database_info_{&database_info},
       worker_queue_{worker_queue},
       credentials_{credentials} {

Firestore/Example/Tests/SpecTests/FSTSyncEngineTestDriver.mm

@@ -52,6 +52,7 @@
 #include "Firestore/core/src/util/statusor.h"
 #include "Firestore/core/src/util/string_format.h"
 #include "Firestore/core/src/util/to_string.h"
+#include "Firestore/core/test/unit/remote/create_noop_connectivity_monitor.h"
 #include "Firestore/core/test/unit/testutil/async_testing.h"
 #include "absl/memory/memory.h"
 
@@ -81,6 +82,8 @@ using firebase::firestore::model::MutationResult;
 using firebase::firestore::model::OnlineState;
 using firebase::firestore::model::SnapshotVersion;
 using firebase::firestore::model::TargetId;
+using firebase::firestore::remote::CreateNoOpConnectivityMonitor;
+using firebase::firestore::remote::ConnectivityMonitor;
 using firebase::firestore::remote::MockDatastore;
 using firebase::firestore::remote::RemoteStore;
 using firebase::firestore::remote::WatchChange;
@@ -167,6 +170,8 @@ NS_ASSUME_NONNULL_BEGIN
 
   std::unique_ptr<RemoteStore> _remoteStore;
 
+  std::unique_ptr<ConnectivityMonitor> _connectivityMonitor;
+
   DelayedConstructor<EventManager> _eventManager;
 
   // Set of active targets, keyed by target Id, mapped to corresponding resume token,
@@ -212,11 +217,13 @@ NS_ASSUME_NONNULL_BEGIN
     _workerQueue = testutil::AsyncQueueForTesting();
     _persistence = std::move(persistence);
     _localStore = absl::make_unique<LocalStore>(_persistence.get(), &_queryEngine, initialUser);
+    _connectivityMonitor = CreateNoOpConnectivityMonitor();
 
     _datastore = std::make_shared<MockDatastore>(_databaseInfo, _workerQueue,
-                                                 std::make_shared<EmptyCredentialsProvider>());
+                                                 std::make_shared<EmptyCredentialsProvider>(),
+                                                 _connectivityMonitor.get());
     _remoteStore = absl::make_unique<RemoteStore>(
-        _localStore.get(), _datastore, _workerQueue,
+        _localStore.get(), _datastore, _workerQueue, _connectivityMonitor.get(),
         [self](OnlineState onlineState) { _syncEngine->HandleOnlineStateChange(onlineState); });
     ;
 

Firestore/core/src/core/firestore_client.cc

@@ -42,6 +42,7 @@
 #include "Firestore/core/src/model/database_id.h"
 #include "Firestore/core/src/model/document_set.h"
 #include "Firestore/core/src/model/mutation.h"
+#include "Firestore/core/src/remote/connectivity_monitor.h"
 #include "Firestore/core/src/remote/datastore.h"
 #include "Firestore/core/src/remote/remote_store.h"
 #include "Firestore/core/src/remote/serializer.h"
@@ -84,6 +85,7 @@ using model::DocumentMap;
 using model::MaybeDocument;
 using model::Mutation;
 using model::OnlineState;
+using remote::ConnectivityMonitor;
 using remote::Datastore;
 using remote::RemoteStore;
 using remote::Serializer;
@@ -193,13 +195,14 @@ void FirestoreClient::Initialize(const User& user, const Settings& settings) {
   query_engine_ = absl::make_unique<IndexFreeQueryEngine>();
   local_store_ = absl::make_unique<LocalStore>(persistence_.get(),
                                                query_engine_.get(), user);
-
+  connectivity_monitor_ = ConnectivityMonitor::Create(worker_queue_);
   auto datastore = std::make_shared<Datastore>(database_info_, worker_queue_,
-                                               credentials_provider_);
+                                               credentials_provider_,
+                                               connectivity_monitor_.get());
 
   remote_store_ = absl::make_unique<RemoteStore>(
       local_store_.get(), std::move(datastore), worker_queue_,
-      [this](OnlineState online_state) {
+      connectivity_monitor_.get(), [this](OnlineState online_state) {
         sync_engine_->HandleOnlineStateChange(online_state);
       });
 

Firestore/core/src/core/firestore_client.h

@@ -51,6 +51,7 @@ class Mutation;
 }  // namespace model
 
 namespace remote {
+class ConnectivityMonitor;
 class RemoteStore;
 }  // namespace remote
 
@@ -204,6 +205,7 @@ class FirestoreClient : public std::enable_shared_from_this<FirestoreClient> {
   std::unique_ptr<local::Persistence> persistence_;
   std::unique_ptr<local::LocalStore> local_store_;
   std::unique_ptr<local::QueryEngine> query_engine_;
+  std::unique_ptr<remote::ConnectivityMonitor> connectivity_monitor_;
   std::unique_ptr<remote::RemoteStore> remote_store_;
   std::unique_ptr<SyncEngine> sync_engine_;
   std::unique_ptr<EventManager> event_manager_;

Firestore/core/src/remote/datastore.cc

@@ -88,23 +88,16 @@ void LogGrpcCallFinished(absl::string_view rpc_name,
 
 }  // namespace
 
-Datastore::Datastore(const DatabaseInfo& database_info,
-                     const std::shared_ptr<AsyncQueue>& worker_queue,
-                     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,
                      std::shared_ptr<CredentialsProvider> credentials,
-                     std::unique_ptr<ConnectivityMonitor> connectivity_monitor)
+                     ConnectivityMonitor* connectivity_monitor)
     : worker_queue_{NOT_NULL(worker_queue)},
       credentials_{std::move(credentials)},
       rpc_executor_{CreateExecutor()},
-      connectivity_monitor_{std::move(connectivity_monitor)},
+      connectivity_monitor_{connectivity_monitor},
       grpc_connection_{database_info, worker_queue, &grpc_queue_,
-                       connectivity_monitor_.get()},
+                       connectivity_monitor_},
       datastore_serializer_{database_info} {
   if (!database_info.ssl_enabled()) {
     GrpcConnection::UseInsecureChannel(database_info.host());

Firestore/core/src/remote/datastore.h

@@ -42,6 +42,8 @@ namespace firebase {
 namespace firestore {
 namespace remote {
 
+class ConnectivityMonitor;
+
 /**
  * `Datastore` represents a proxy for the remote server, hiding details of the
  * RPC layer. It:
@@ -65,7 +67,8 @@ class Datastore : public std::enable_shared_from_this<Datastore> {
 
   Datastore(const core::DatabaseInfo& database_info,
             const std::shared_ptr<util::AsyncQueue>& worker_queue,
-            std::shared_ptr<auth::CredentialsProvider> credentials);
+            std::shared_ptr<auth::CredentialsProvider> credentials,
+            ConnectivityMonitor* connectivity_monitor);
 
   virtual ~Datastore() = default;
 
@@ -127,12 +130,6 @@ class Datastore : public std::enable_shared_from_this<Datastore> {
   Datastore& operator=(Datastore&& other) = delete;
 
  protected:
-  /** Test-only constructor */
-  Datastore(const core::DatabaseInfo& database_info,
-            const std::shared_ptr<util::AsyncQueue>& worker_queue,
-            std::shared_ptr<auth::CredentialsProvider> credentials,
-            std::unique_ptr<ConnectivityMonitor> connectivity_monitor);
-
   /** Test-only method */
   grpc::CompletionQueue* grpc_queue() {
     return &grpc_queue_;
@@ -184,8 +181,7 @@ class Datastore : public std::enable_shared_from_this<Datastore> {
   // shared for all spawned gRPC streams and calls).
   std::unique_ptr<util::Executor> rpc_executor_;
   grpc::CompletionQueue grpc_queue_;
-  // TODO(varconst): move `ConnectivityMonitor` to `FirestoreClient`.
-  std::unique_ptr<ConnectivityMonitor> connectivity_monitor_;
+  ConnectivityMonitor* connectivity_monitor_ = nullptr;
   GrpcConnection grpc_connection_;
 
   std::vector<std::unique_ptr<GrpcCall>> active_calls_;

Firestore/core/src/remote/remote_store.cc

@@ -61,11 +61,13 @@ constexpr int kMaxPendingWrites = 10;
 RemoteStore::RemoteStore(
     LocalStore* local_store,
     std::shared_ptr<Datastore> datastore,
-    const std::shared_ptr<AsyncQueue>& worker_queue,
+    const std::shared_ptr<util::AsyncQueue>& worker_queue,
+    ConnectivityMonitor* connectivity_monitor,
     std::function<void(model::OnlineState)> online_state_handler)
     : local_store_{local_store},
       datastore_{std::move(datastore)},
-      online_state_tracker_{worker_queue, std::move(online_state_handler)} {
+      online_state_tracker_{worker_queue, std::move(online_state_handler)},
+      connectivity_monitor_{NOT_NULL(connectivity_monitor)} {
   datastore_->Start();
 
   // Create streams (but note they're not started yet)
@@ -77,6 +79,23 @@ void RemoteStore::Start() {
   // For now, all setup is handled by `EnableNetwork`. We might expand on this
   // in the future.
   EnableNetwork();
+
+  connectivity_monitor_->AddCallback(
+      [this](ConnectivityMonitor::NetworkStatus network_status) {
+        if (network_status == ConnectivityMonitor::NetworkStatus::Unavailable) {
+          LOG_DEBUG(
+              "RemoteStore %s ignoring connectivity callback for unavailable "
+              "network",
+              this);
+          return;
+        }
+
+        if (CanUseNetwork()) {
+          LOG_DEBUG("RemoteStore %s restarting streams as connectivity changed",
+                    this);
+          RestartNetwork();
+        }
+      });
 }
 
 void RemoteStore::EnableNetwork() {
@@ -528,16 +547,22 @@ absl::optional<TargetData> RemoteStore::GetTargetDataForTarget(
                                         : absl::optional<TargetData>{};
 }
 
+void RemoteStore::RestartNetwork() {
+  is_network_enabled_ = false;
+  DisableNetworkInternal();
+  online_state_tracker_.UpdateState(OnlineState::Unknown);
+  write_stream_->InhibitBackoff();
+  watch_stream_->InhibitBackoff();
+  EnableNetwork();
+}
+
 void RemoteStore::HandleCredentialChange() {
   if (CanUseNetwork()) {
     // Tear down and re-create our network streams. This will ensure we get a
     // fresh auth token for the new user and re-fill the write pipeline with new
     // mutations from the `LocalStore` (since mutations are per-user).
     LOG_DEBUG("RemoteStore %s restarting streams for new credential", this);
-    is_network_enabled_ = false;
-    DisableNetworkInternal();
-    online_state_tracker_.UpdateState(OnlineState::Unknown);
-    EnableNetwork();
+    RestartNetwork();
   }
 }
 

Firestore/core/src/remote/remote_store.h

@@ -44,6 +44,8 @@ class LocalStore;
 
 namespace remote {
 
+class ConnectivityMonitor;
+
 /**
  * A callback interface for events from remote store.
  */
@@ -109,6 +111,7 @@ class RemoteStore : public TargetMetadataProvider,
   RemoteStore(local::LocalStore* local_store,
               std::shared_ptr<Datastore> datastore,
               const std::shared_ptr<util::AsyncQueue>& worker_queue,
+              ConnectivityMonitor* connectivity_monitor,
               std::function<void(model::OnlineState)> online_state_handler);
 
   void set_sync_engine(RemoteStoreCallback* sync_engine) {
@@ -205,6 +208,7 @@ class RemoteStore : public TargetMetadataProvider,
       std::vector<model::MutationResult> mutation_results) override;
 
  private:
+  void RestartNetwork();
   void DisableNetworkInternal();
 
   void SendWatchRequest(const local::TargetData& target_data);
@@ -270,6 +274,8 @@ class RemoteStore : public TargetMetadataProvider,
 
   OnlineStateTracker online_state_tracker_;
 
+  ConnectivityMonitor* connectivity_monitor_ = nullptr;
+
   /**
    * Set to true by `EnableNetwork` and false by `DisableNetwork` and indicates
    * the user-preferred network state.

Firestore/core/test/unit/remote/datastore_test.cc

@@ -32,6 +32,7 @@
 #include "Firestore/core/src/util/status.h"
 #include "Firestore/core/src/util/statusor.h"
 #include "Firestore/core/src/util/string_apple.h"
+#include "Firestore/core/test/unit/remote/create_noop_connectivity_monitor.h"
 #include "Firestore/core/test/unit/remote/fake_credentials_provider.h"
 #include "Firestore/core/test/unit/remote/grpc_stream_tester.h"
 #include "Firestore/core/test/unit/testutil/async_testing.h"
@@ -102,9 +103,10 @@ class FakeDatastore : public Datastore {
 std::shared_ptr<FakeDatastore> CreateDatastore(
     const DatabaseInfo& database_info,
     const std::shared_ptr<AsyncQueue>& worker_queue,
-    std::shared_ptr<CredentialsProvider> credentials) {
+    std::shared_ptr<CredentialsProvider> credentials,
+    ConnectivityMonitor* connectivity_monitor) {
   return std::make_shared<FakeDatastore>(database_info, worker_queue,
-                                         credentials);
+                                         credentials, connectivity_monitor);
 }
 
 }  // namespace
@@ -114,7 +116,11 @@ class DatastoreTest : public testing::Test {
   DatastoreTest()
       : database_info{DatabaseId{"p", "d"}, "", "localhost", false},
         worker_queue{testutil::AsyncQueueForTesting()},
-        datastore{CreateDatastore(database_info, worker_queue, credentials)},
+        connectivity_monitor{CreateNoOpConnectivityMonitor()},
+        datastore{CreateDatastore(database_info,
+                                  worker_queue,
+                                  credentials,
+                                  connectivity_monitor.get())},
         fake_grpc_queue{datastore->queue()} {
     // Deliberately don't `Start` the `Datastore` to prevent normal gRPC
     // completion queue polling; the test is using `FakeGrpcQueue`.
@@ -153,9 +159,9 @@ class DatastoreTest : public testing::Test {
       std::make_shared<FakeCredentialsProvider>();
 
   std::shared_ptr<AsyncQueue> worker_queue;
+  std::unique_ptr<ConnectivityMonitor> connectivity_monitor;
   std::shared_ptr<FakeDatastore> datastore;
 
-  std::unique_ptr<ConnectivityMonitor> connectivity_monitor;
   FakeGrpcQueue fake_grpc_queue;
 };