Use manual foreground detection to trigger network reconnect #2763

schmidt-sebastian · 2021-06-25T17:59:41Z

This is a follow up to #2693, which used Android's built-in BackgroundDetector to fast-forward network reconnects when an app entered the foreground. Unfortunately, the BackgroundDetector is pretty conservative and only fires if it knows for certain that the app was in the background and now is in the foreground. This leads to some suppressed notifications. This PR just hooks directly into the notification API and always raises the foreground notification.

Logs:

2021-06-25 13:30:40.601 4251-4665/com.google.firebase.example.fireeats I/Firestore: (23.0.2) [OnlineStateTracker]: Could not reach Cloud Firestore backend. Connection failed 1 times. Most recent error: Status{code=UNAVAILABLE, description=Unable to resolve host foo, cause=java.lang.RuntimeException: java.net.UnknownHostException: Unable to resolve host "foo": No address associated with hostname
...
    This typically indicates that your device does not have a healthy Internet connection at the moment. The client will operate in offline mode until it is able to successfully connect to the backend.
2021-06-25 13:30:40.605 4251-4665/com.google.firebase.example.fireeats I/Firestore: (23.0.2) [ExponentialBackoff]: Backing off for 8220 ms (base delay: 7593 ms, delay with jitter: 8372 ms, last attempt: 152 ms ago)
...
2021-06-25 13:30:46.836 4251-4251/com.google.firebase.example.fireeats I/Firestore: (23.0.2) [AndroidConnectivityMonitor]: App has entered the foreground.
2021-06-25 13:30:46.843 4251-4665/com.google.firebase.example.fireeats I/Firestore: (23.0.2) [RemoteStore]: Restarting streams for network reachability change.
2021-06-25 13:30:46.845 4251-4665/com.google.firebase.example.fireeats I/Firestore: (23.0.2) [WatchStream]: (f298b8) Performing stream teardown
2021-06-25 13:30:46.866 4251-4665/com.google.firebase.example.fireeats I/Firestore: (23.0.2) [WatchStream]: (f298b8) Stream is open

Fixes #2637

google-oss-bot · 2021-06-25T18:10:14Z

Coverage Report

Affected SDKs

`firebase-firestore`

SDK overall coverage changed from 47.23% (563781c) to 47.09% (e0e64817) by -0.14%.

Filename	Base (`563781c`)	Head (e0e64817)	Diff
AndroidConnectivityMonitor.java	46.77%	39.51%	-7.27%
AsyncQueue.java	78.39%	76.88%	-1.51%
FirestoreClient.java	34.96%	30.08%	-4.88%
LruGarbageCollector.java	93.46%	84.11%	-9.35%
OnlineStateTracker.java	100.00%	98.11%	-1.89%
RemoteStore.java	92.15%	89.56%	-2.59%

Test Logs

Notes

HTML coverage reports can be produced locally with ./gradlew <product>:checkCoverage.
Report files are located at <product-build-dir>/reports/jacoco/.

Head commit (e0e64817) is created by Prow via merging commits: 563781c d9051b6.

google-oss-bot · 2021-06-25T18:10:50Z

Binary Size Report

Affected SDKs

firebase-firestore

Type Base (563781c) Head (e0e64817) Diff

aar 1.03 MB 1.03 MB +1.02 kB (+0.1%)

apk (release) 3.19 MB 3.19 MB +76 B (+0.0%)

Test Logs

Notes

Head commit (e0e64817) is created by Prow via merging commits: 563781c d9051b6.

schmidt-sebastian · 2021-06-25T20:34:12Z

Looks like this broke a test.

schmidt-sebastian · 2021-06-25T21:06:47Z

...firestore/src/main/java/com/google/firebase/firestore/remote/AndroidConnectivityMonitor.java

+                // Only raise the foreground notification if the same activity when to the
+                // background before. This prevents notifications when an app switches between
+                // activities.
+                if (activity == lastActivity) {


@samtstern Does this code make sense? I would like to detect when an app goes into the background and then back into the foreground. I assume that in this case, we will get a notification with the same activity, whereas in the normal app lifecycle we will receive this notification with a different activity.

Do you also happen to know if the memory address here will always be the same?

I have a feeling activity == lastActivity is going to be too simple, as Android destroys activities all the time. That's why you need to do all the savedInstanceState stuff to handle device rotation.

But I don't know the more robust way to handle Activity comparison.

Moreover, holding a strong reference to an Activity leaks the Activity instance, causing a memory leak. As Sam mentions, the Activity instances would compare unequal even if it was the same activity but some "configuration change" caused the Activity to be destroyed and re-created, such as a screen rotation.

What if you instead compared the activity's Intent for equality? Holding a strong reference to an Intent is not a memory leak and it more-or-less describes the Activity. It's not perfect, because, in theory, you can start a new Activity with the exact same Intent overtop of itself. But I think that situation is fairly rare.

I changed the implementation to also hook into the component callbacks, which offer a notification when an app's UI becomes hidden. This is also used here: cs/piper///depot/google3/java/com/google/android/gmscore/integ/client/common/src/com/google/android/gms/common/api/internal/BackgroundDetector.java;l=140

The code is now very similar to BackgroundDetector, but it also fires if the state transition is not 100% certain. In my testing, it works to detect the transitions, whereas BackgroundDetector sometimes misses a transition. It is likely better to fire more often then to miss a transition.

schmidt-sebastian · 2021-06-25T21:26:57Z

/test smoke-tests

dconeybe · 2021-06-28T13:24:20Z

firebase-firestore/CHANGELOG.md

@@ -1,4 +1,8 @@
-# Unreleased
+# Unreelased


typo: "Unreelased"

dconeybe · 2021-06-28T13:24:54Z

firebase-firestore/CHANGELOG.md

-# Unreleased
+# Unreelased
+- [changed] Increases the aggressiveness of network retires when an app's
+  visibility status changes.


Is "visibility" the right term here? Would it make more sense to just say "foreground status"?

That might be better. Updated.

dconeybe · 2021-06-28T13:29:53Z

...firestore/src/main/java/com/google/firebase/firestore/remote/AndroidConnectivityMonitor.java

+                // Only raise the foreground notification if the same activity when to the
+                // background before. This prevents notifications when an app switches between
+                // activities.
+                if (activity == lastActivity) {


Moreover, holding a strong reference to an Activity leaks the Activity instance, causing a memory leak. As Sam mentions, the Activity instances would compare unequal even if it was the same activity but some "configuration change" caused the Activity to be destroyed and re-created, such as a screen rotation.

What if you instead compared the activity's Intent for equality? Holding a strong reference to an Intent is not a memory leak and it more-or-less describes the Activity. It's not perfect, because, in theory, you can start a new Activity with the exact same Intent overtop of itself. But I think that situation is fairly rare.

dconeybe · 2021-06-28T17:18:12Z

...firestore/src/main/java/com/google/firebase/firestore/remote/AndroidConnectivityMonitor.java

    }
  }

  private void configureBackgroundStateListener() {
-    BackgroundDetector.getInstance().addListener(this);
+    Application applicationContext = (Application) context.getApplicationContext();


nit: applicationContext can be renamed to application since it is being casted to an instance of Application.

dconeybe · 2021-06-28T17:27:29Z

...firestore/src/main/java/com/google/firebase/firestore/remote/AndroidConnectivityMonitor.java

+
+          @Override
+          public void onActivityResumed(@NonNull Activity activity) {
+            if (inBackground.compareAndSet(true, false)) {


Consider putting the body of onActivityResumed() into onActivityCreated() as well, so that the "offline" state is updated as early as possible (this is what BackgroundDetector does). Even copy it into onActivityStarted() too.

That's a good idea. It might not make a huge difference in practice since bringing up the network is a pretty heavy operation on its own, but it certainly should not hut (at least I would hope so).

google-cla bot added the cla: yes Override cla label Jun 25, 2021

google-oss-bot added the size/M label Jun 25, 2021

schmidt-sebastian force-pushed the mrschmidt/custombackgrounddetector branch from 46cbc04 to cbdd29f Compare June 25, 2021 18:00

Use manual foreground detection to trigger network reconnect

f3a5738

schmidt-sebastian force-pushed the mrschmidt/custombackgrounddetector branch from cbdd29f to f3a5738 Compare June 25, 2021 18:01

Simplify

98a7ba7

schmidt-sebastian requested a review from dconeybe June 25, 2021 19:37

schmidt-sebastian assigned dconeybe Jun 25, 2021

schmidt-sebastian added the api: firestore label Jun 25, 2021

schmidt-sebastian unassigned dconeybe Jun 25, 2021

Change handling of online state changes

63cf21a

google-oss-bot added size/L and removed size/M labels Jun 25, 2021

schmidt-sebastian commented Jun 25, 2021

View reviewed changes

schmidt-sebastian assigned dconeybe Jun 25, 2021

Changelog

3907fed

dconeybe requested changes Jun 28, 2021

View reviewed changes

Also use ComponentCallbacks2

9b0cebd

schmidt-sebastian force-pushed the mrschmidt/custombackgrounddetector branch from 73d64eb to 9b0cebd Compare June 28, 2021 16:17

dconeybe requested changes Jun 28, 2021

View reviewed changes

dconeybe assigned schmidt-sebastian and unassigned dconeybe Jun 28, 2021

Call listener more often

d9051b6

schmidt-sebastian assigned dconeybe and unassigned schmidt-sebastian Jun 28, 2021

dconeybe approved these changes Jun 28, 2021

View reviewed changes

dconeybe assigned schmidt-sebastian and unassigned dconeybe Jun 28, 2021

schmidt-sebastian merged commit f65c333 into master Jun 28, 2021

schmidt-sebastian deleted the mrschmidt/custombackgrounddetector branch June 28, 2021 20:21

Ehesp mentioned this pull request Jul 2, 2021

🐛 [cloud_firestore] Streambuilder very slow to update from Cloud Firestore when Android app resumes after long period of inactivity firebase/flutterfire#4305

Closed

firebase locked and limited conversation to collaborators Jul 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use manual foreground detection to trigger network reconnect #2763

Use manual foreground detection to trigger network reconnect #2763

schmidt-sebastian commented Jun 25, 2021 •

edited

Loading

google-oss-bot commented Jun 25, 2021 •

edited

Loading

`firebase-firestore`

google-oss-bot commented Jun 25, 2021 •

edited

Loading

`firebase-firestore`

schmidt-sebastian commented Jun 25, 2021

schmidt-sebastian Jun 25, 2021

samtstern Jun 28, 2021

dconeybe Jun 28, 2021

schmidt-sebastian Jun 28, 2021

schmidt-sebastian commented Jun 25, 2021

dconeybe Jun 28, 2021

schmidt-sebastian Jun 28, 2021

dconeybe Jun 28, 2021

schmidt-sebastian Jun 28, 2021

dconeybe Jun 28, 2021

dconeybe Jun 28, 2021

schmidt-sebastian Jun 28, 2021

dconeybe Jun 28, 2021

schmidt-sebastian Jun 28, 2021

Use manual foreground detection to trigger network reconnect #2763

Use manual foreground detection to trigger network reconnect #2763

Conversation

schmidt-sebastian commented Jun 25, 2021 • edited Loading

google-oss-bot commented Jun 25, 2021 • edited Loading

Coverage Report

Affected SDKs

firebase-firestore

Test Logs

Notes

google-oss-bot commented Jun 25, 2021 • edited Loading

Binary Size Report

Affected SDKs

firebase-firestore

Test Logs

Notes

schmidt-sebastian commented Jun 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian commented Jun 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian commented Jun 25, 2021 •

edited

Loading

google-oss-bot commented Jun 25, 2021 •

edited

Loading

`firebase-firestore`

google-oss-bot commented Jun 25, 2021 •

edited

Loading

`firebase-firestore`