Reset backoff when connectivity status changes #5857

schmidt-sebastian · 2020-06-19T17:59:12Z

This is a follow up to #4985 and addresses an additional problem that I saw in the test app. If the SDK was already in the background and we were in a backoff phase, the SDK waited for the backoff to finish before reconnecting, which could take up to a minute. This PR plumbs the connectivity handler through to RemoteStore and resets the backoff when a connectivity change occurred.

cc @wu-hui

Fixes #5783

var-const

Is it possible to test the new behavior? (I think all existing tests that emulate connectivity changes are in test/unit/remote/grpc_connection_test.cc, see if it's possible to write similar tests for this)

var-const · 2020-06-19T20:31:41Z

Firestore/CHANGELOG.md

@@ -1,3 +1,8 @@
+# Unreleased
+- [fixed] Removed a delay that may have prevented Firestore from immediately
+  establishing a network connections if a connectivity change occurred while


Nit: either "a network connection" or "network connections".

var-const · 2020-06-19T20:34:30Z

Firestore/core/src/remote/datastore.cc

 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)},


Nit: this move is now extraneous (moving a built-in, such as a pointer, is the same as copying it).

var-const · 2020-06-19T20:39:25Z

Firestore/core/src/remote/remote_store.cc

@@ -59,13 +59,15 @@ using util::Status;
 constexpr int kMaxPendingWrites = 10;

 RemoteStore::RemoteStore(
-    LocalStore* local_store,
+    local::LocalStore* local_store,


Nit: qualifying LocalStore with its namespace shouldn't be necessary given the using directive above.

var-const · 2020-06-19T20:49:07Z

Firestore/core/src/remote/remote_store.cc

+        if (CanUseNetwork()) {
+          LOG_DEBUG("RemoteStore %s restarting streams as connectivity changed",
+                    this);
+          RestartNetwork();


This doesn't create any undesirable interactions with the existing logic on connectivity change, right? (I don't think it should, just checking)

The existing logic invalidates the GRPC channel, which speeds up reconnects already and is the foundation this PR is based on.

var-const · 2020-06-19T20:49:52Z

Firestore/core/src/remote/remote_store.cc

@@ -77,6 +79,15 @@ 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 /*ignored*/) {


Is it necessary/desirable to run this logic when the network status is "unavailable"?

Probably not - but note that

firebase-ios-sdk/Firestore/core/src/remote/grpc_connection.cc

Line 296 in 8633f5f

grpc_channel_.reset();

kills the GRPC channel no matter what.

var-const · 2020-06-19T20:50:59Z

Firestore/core/src/remote/remote_store.cc

-    DisableNetworkInternal();
-    online_state_tracker_.UpdateState(OnlineState::Unknown);
-    EnableNetwork();
+    RestartNetwork();


Looks like this PR could also make reconnection faster upon credentials' change? If correct, perhaps it's worth mentioning in the PR's description?

Only if the credential change happened while offline, which is probably uncommon (since Auth needs a network connection as well).

schmidt-sebastian

I addressed the easy comments.

I don't know how to unit test this - let's talk about this on Monday. This code affects the RemoteStore, so we should test it there. The only tests for the RemoteStore, however, are the Spec tests, which right now don't handle connectivity. I think writing a unit test would entail porting this PR to Web and then adding the credential change listener to the Spec tests suite. That's something we can do, but maybe we can come up with an easier and more direct way to test.

In the meantime, I will verify via test app that my recent change to ignore Unavailable didn't affect the functionality.

schmidt-sebastian · 2020-06-19T21:25:43Z

Firestore/CHANGELOG.md

@@ -1,3 +1,8 @@
+# Unreleased
+- [fixed] Removed a delay that may have prevented Firestore from immediately
+  establishing a network connections if a connectivity change occurred while


schmidt-sebastian · 2020-06-19T21:25:59Z

Firestore/core/src/remote/datastore.cc

 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)},


schmidt-sebastian · 2020-06-19T21:26:04Z

Firestore/core/src/remote/remote_store.cc

@@ -59,13 +59,15 @@ using util::Status;
 constexpr int kMaxPendingWrites = 10;

 RemoteStore::RemoteStore(
-    LocalStore* local_store,
+    local::LocalStore* local_store,


schmidt-sebastian · 2020-06-19T21:30:18Z

Firestore/core/src/remote/remote_store.cc

@@ -77,6 +79,15 @@ 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 /*ignored*/) {


Probably not - but note that

firebase-ios-sdk/Firestore/core/src/remote/grpc_connection.cc

Line 296 in 8633f5f

grpc_channel_.reset();

kills the GRPC channel no matter what.

schmidt-sebastian · 2020-06-19T21:31:04Z

Firestore/core/src/remote/remote_store.cc

+        if (CanUseNetwork()) {
+          LOG_DEBUG("RemoteStore %s restarting streams as connectivity changed",
+                    this);
+          RestartNetwork();


The existing logic invalidates the GRPC channel, which speeds up reconnects already and is the foundation this PR is based on.

schmidt-sebastian · 2020-06-19T21:31:40Z

Firestore/core/src/remote/remote_store.cc

-    DisableNetworkInternal();
-    online_state_tracker_.UpdateState(OnlineState::Unknown);
-    EnableNetwork();
+    RestartNetwork();


Only if the credential change happened while offline, which is probably uncommon (since Auth needs a network connection as well).

var-const

I agree that testing this is a hard problem, so I don't want to block this PR on that (it can be done in a follow-up, and for now doing manual testing seems sufficient).

schmidt-sebastian · 2020-06-22T22:26:25Z

Verified manually.

Reset backoff if connectivity status changed

4d65af6

googlebot added the cla: yes label Jun 19, 2020

schmidt-sebastian changed the title ~~Reset backoff if connectivity status changed~~ WIP Reset backoff if connectivity status changed Jun 19, 2020

google-oss-bot added the api: firestore label Jun 19, 2020

schmidt-sebastian added 2 commits June 19, 2020 11:04

WIP

e54fc59

Changelog

846cf50

schmidt-sebastian changed the title ~~WIP Reset backoff if connectivity status changed~~ Reset backoff when connectivity status changes Jun 19, 2020

Update CHANGELOG.md

fa76da6

schmidt-sebastian requested a review from var-const June 19, 2020 20:21

schmidt-sebastian assigned var-const Jun 19, 2020

var-const reviewed Jun 19, 2020

View reviewed changes

var-const assigned schmidt-sebastian and unassigned var-const Jun 19, 2020

schmidt-sebastian added 3 commits June 19, 2020 14:16

Address code style

40fabe9

Merge

192e422

Style

656d333

schmidt-sebastian commented Jun 19, 2020

View reviewed changes

var-const approved these changes Jun 19, 2020

View reviewed changes

schmidt-sebastian merged commit 296a1ea into master Jun 22, 2020

schmidt-sebastian deleted the mrschmidt/backoff branch June 22, 2020 22:49

schmidt-sebastian mentioned this pull request Jun 23, 2020

Reset backoff when connectivity status changes firebase/firebase-android-sdk#1706

Merged

granluo pushed a commit that referenced this pull request Jul 7, 2020

Reset backoff when connectivity status changes (#5857)

d534766

firebase locked and limited conversation to collaborators Jul 23, 2020

Reset backoff when connectivity status changes #5857

Reset backoff when connectivity status changes #5857

Uh oh!

Conversation

schmidt-sebastian commented Jun 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

var-const left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian Jun 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian Jun 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

var-const left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian commented Jun 22, 2020

Uh oh!

Uh oh!

schmidt-sebastian commented Jun 19, 2020 •

edited

Loading

schmidt-sebastian Jun 19, 2020 •

edited

Loading

schmidt-sebastian Jun 19, 2020 •

edited

Loading

var-const left a comment •

edited

Loading