Skip to content

Use manual foreground detection to trigger network reconnect #2763

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jun 28, 2021
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions firebase-firestore/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
# Unreleased
- [changed] Increases the aggressiveness of network retires when an app's
foreground status changes.

# 23.0.1
- [changed] The SDK now tries to immediately establish a connection to the
backend when the app enters the foreground.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,16 +81,16 @@ public ImmutableSortedSet<DocumentKey> getRemoteKeysForTarget(int targetId) {
RemoteStore remoteStore =
new RemoteStore(callback, localStore, datastore, testQueue, connectivityMonitor);

waitFor(testQueue.enqueue(() -> remoteStore.forceEnableNetwork()));
waitFor(testQueue.enqueue(remoteStore::forceEnableNetwork));
drain(testQueue);
networkChangeSemaphore.drainPermits();

connectivityMonitor.goOffline();
waitFor(networkChangeSemaphore);
drain(testQueue);

waitFor(testQueue.enqueue(() -> remoteStore.forceEnableNetwork()));
networkChangeSemaphore.drainPermits();

waitFor(testQueue.enqueue(remoteStore::forceEnableNetwork));
connectivityMonitor.goOnline();
waitFor(networkChangeSemaphore);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,27 +17,35 @@
import static com.google.firebase.firestore.util.Assert.hardAssert;

import android.annotation.TargetApi;
import android.app.Activity;
import android.app.Application;
import android.content.BroadcastReceiver;
import android.content.ComponentCallbacks2;
import android.content.Context;
import android.content.Intent;
import android.content.IntentFilter;
import android.content.res.Configuration;
import android.net.ConnectivityManager;
import android.net.Network;
import android.os.Build;
import android.os.Bundle;
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import com.google.android.gms.common.api.internal.BackgroundDetector;
import com.google.firebase.firestore.util.Consumer;
import com.google.firebase.firestore.util.Logger;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.atomic.AtomicBoolean;

/**
* Android implementation of ConnectivityMonitor. Parallel implementations exist for N+ and pre-N.
*
* <p>Implementation note: Most of the code here was shamelessly stolen from
* https://github.com/grpc/grpc-java/blob/master/android/src/main/java/io/grpc/android/AndroidChannelBuilder.java
*/
public final class AndroidConnectivityMonitor
implements ConnectivityMonitor, BackgroundDetector.BackgroundStateChangeListener {
public final class AndroidConnectivityMonitor implements ConnectivityMonitor {

private static final String LOG_TAG = "AndroidConnectivityMonitor";

private final Context context;
@Nullable private final ConnectivityManager connectivityManager;
Expand Down Expand Up @@ -78,34 +86,72 @@ private void configureNetworkMonitoring() {
final DefaultNetworkCallback defaultNetworkCallback = new DefaultNetworkCallback();
connectivityManager.registerDefaultNetworkCallback(defaultNetworkCallback);
unregisterRunnable =
new Runnable() {
@Override
public void run() {
connectivityManager.unregisterNetworkCallback(defaultNetworkCallback);
}
};
() -> connectivityManager.unregisterNetworkCallback(defaultNetworkCallback);
} else {
NetworkReceiver networkReceiver = new NetworkReceiver();
@SuppressWarnings("deprecation")
IntentFilter networkIntentFilter = new IntentFilter(ConnectivityManager.CONNECTIVITY_ACTION);
context.registerReceiver(networkReceiver, networkIntentFilter);
unregisterRunnable =
new Runnable() {
@Override
public void run() {
context.unregisterReceiver(networkReceiver);
}
};
unregisterRunnable = () -> context.unregisterReceiver(networkReceiver);
}
}

private void configureBackgroundStateListener() {
BackgroundDetector.getInstance().addListener(this);
Application applicationContext = (Application) context.getApplicationContext();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

final AtomicBoolean inBackground = new AtomicBoolean();

// Manually register an ActivityLifecycleCallback. Android's BackgroundDetector only notifies
// when it is certain that the app transitioned from background to foreground. Instead, we
// want to be notified whenever there is a slight chance that this transition happened.
applicationContext.registerActivityLifecycleCallbacks(
new Application.ActivityLifecycleCallbacks() {
@Override
public void onActivityCreated(@NonNull Activity activity, Bundle savedInstanceState) {}

@Override
public void onActivityStarted(@NonNull Activity activity) {}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

raiseForegroundNotification();
}
}

@Override
public void onActivityPaused(@NonNull Activity activity) {}

@Override
public void onActivityStopped(@NonNull Activity activity) {}

@Override
public void onActivitySaveInstanceState(
@NonNull Activity activity, @NonNull Bundle outState) {}

@Override
public void onActivityDestroyed(@NonNull Activity activity) {}
});

applicationContext.registerComponentCallbacks(
new ComponentCallbacks2() {
@Override
public void onTrimMemory(int level) {
if (level == ComponentCallbacks2.TRIM_MEMORY_UI_HIDDEN) {
inBackground.set(true);
}
}

@Override
public void onConfigurationChanged(@NonNull Configuration newConfig) {}

@Override
public void onLowMemory() {}
});
}

@Override
public void onBackgroundStateChanged(boolean background) {
if (!background && isConnected()) {
public void raiseForegroundNotification() {
Logger.debug(LOG_TAG, "App has entered the foreground.");
if (isConnected()) {
raiseCallbacks(/* connected= */ true);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,11 @@ interface OnlineStateCallback {
shouldWarnClientIsOffline = true;
}

/** Returns the current online state. */
OnlineState getState() {
return state;
}

/**
* Called by RemoteStore when a watch stream is started (including on each backoff attempt).
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -214,14 +214,29 @@ public void onClose(Status status) {
// Porting Note: Unlike iOS, `restartNetwork()` is called even when the network
// becomes unreachable as we don't have any other way to tear down our streams.

// We only invoke restartNetwork() when the network status differs from our online
// state. This prevents frequent reconnects as the callback is invoked whenever
// the app reaches the foreground.
if (networkStatus.equals(NetworkStatus.REACHABLE)
&& onlineStateTracker.getState().equals(OnlineState.ONLINE)) {
return;
}

if (networkStatus.equals(NetworkStatus.UNREACHABLE)
&& onlineStateTracker.getState().equals(OnlineState.OFFLINE)) {
return;
}

// If the network has been explicitly disabled, make sure we don't accidentally
// re-enable it.
if (canUseNetwork()) {
// Tear down and re-create our network streams. This will ensure the backoffs are
// reset.
Logger.debug(LOG_TAG, "Restarting streams for network reachability change.");
restartNetwork();
if (!canUseNetwork()) {
return;
}

// Tear down and re-create our network streams. This will ensure the backoffs are
// reset.
Logger.debug(LOG_TAG, "Restarting streams for network reachability change.");
restartNetwork();
});
});
}
Expand Down