Skip to content

Commit

Permalink
android: make Channel always enterIdle upon network recover (#7611)
Browse files Browse the repository at this point in the history 
On Android, there is a race between making new RPCs and reconnecting after network comes back. If the former happens first, RPCs fail immediately. This is because resetConnectBackoff() does not update the picker before trying to reconnect and new RPCs are sent with the old picker, which fails RPCs immediately.

In this change, we move to use enterIdle(), which updates the channel picker to cause new RPCs being buffered (while subchannels are in reconnecting), at the moment network recovers. Hopefully, this can avoid RPCs being dropped prematurely in network recovery.
  • Loading branch information
@voidzcy
voidzcy committed Nov 17, 2020
1 parent 729175c commit b3429ec
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 75 deletions.
23 changes: 3 additions & 20 deletions android/src/main/java/io/grpc/android/AndroidChannelBuilder.java
View file
Expand Up @@ -151,7 +151,7 @@ public ManagedChannel build() {

/**
* Wraps an OkHttp channel and handles invoking the appropriate methods (e.g., {@link
* ManagedChannel#resetConnectBackoff}) when the device network state changes.
* ManagedChannel#enterIdle) when the device network state changes.
*/
@VisibleForTesting
static final class AndroidChannel extends ManagedChannel {
Expand Down Expand Up @@ -290,26 +290,9 @@ public void enterIdle() {
/** Respond to changes in the default network. Only used on API levels 24+. */
@TargetApi(Build.VERSION_CODES.N)
private class DefaultNetworkCallback extends ConnectivityManager.NetworkCallback {
// Registering a listener may immediate invoke onAvailable/onLost: the API docs do not specify
// if the methods are always invoked once, then again on any change, or only on change. When
// onAvailable() is invoked immediately without an actual network change, it's preferable to
// (spuriously) resetConnectBackoff() rather than enterIdle(), as the former is a no-op if the
// channel has already moved to CONNECTING.
private boolean isConnected = false;

@Override
public void onAvailable(Network network) {
if (isConnected) {
delegate.enterIdle();
} else {
delegate.resetConnectBackoff();
}
isConnected = true;
}

@Override
public void onLost(Network network) {
isConnected = false;
delegate.enterIdle();
}
}

Expand All @@ -325,7 +308,7 @@ public void onReceive(Context context, Intent intent) {
boolean wasConnected = isConnected;
isConnected = networkInfo != null && networkInfo.isConnected();
if (isConnected && !wasConnected) {
delegate.resetConnectBackoff();
delegate.enterIdle();
}
}
}
Expand Down
73 changes: 18 additions & 55 deletions android/src/test/java/io/grpc/android/AndroidChannelBuilderTest.java
View file
Expand Up @@ -119,7 +119,7 @@ public void nullContextDoesNotThrow_api23() {
.sendBroadcast(new Intent(ConnectivityManager.CONNECTIVITY_ACTION));
androidChannel.shutdown();

assertThat(delegateChannel.resetCount).isEqualTo(0);
assertThat(delegateChannel.enterIdleCount).isEqualTo(0);
}

@Test
Expand All @@ -133,53 +133,52 @@ public void nullContextDoesNotThrow_api24() {
shadowOf(connectivityManager).setActiveNetworkInfo(MOBILE_CONNECTED);
androidChannel.shutdown();

assertThat(delegateChannel.resetCount).isEqualTo(0);
assertThat(delegateChannel.enterIdleCount).isEqualTo(0);
}

@Test
@Config(sdk = 23)
public void resetConnectBackoff_api23() {
public void networkChanges_api23() {
TestChannel delegateChannel = new TestChannel();
ManagedChannel androidChannel =
new AndroidChannelBuilder.AndroidChannel(
delegateChannel, ApplicationProvider.getApplicationContext());
assertThat(delegateChannel.resetCount).isEqualTo(0);
assertThat(delegateChannel.enterIdleCount).isEqualTo(0);

// On API levels < 24, the broadcast receiver will invoke resetConnectBackoff() on the first
// On API levels < 24, the broadcast receiver will invoke enterIdle() on the first
// connectivity action broadcast regardless of previous connection status
shadowOf(connectivityManager).setActiveNetworkInfo(WIFI_CONNECTED);
ApplicationProvider
.getApplicationContext()
.sendBroadcast(new Intent(ConnectivityManager.CONNECTIVITY_ACTION));
assertThat(delegateChannel.resetCount).isEqualTo(1);
assertThat(delegateChannel.enterIdleCount).isEqualTo(1);

// The broadcast receiver may fire when the active network status has not actually changed
ApplicationProvider
.getApplicationContext()
.sendBroadcast(new Intent(ConnectivityManager.CONNECTIVITY_ACTION));
assertThat(delegateChannel.resetCount).isEqualTo(1);
assertThat(delegateChannel.enterIdleCount).isEqualTo(1);

// Drop the connection
shadowOf(connectivityManager).setActiveNetworkInfo(null);
ApplicationProvider
.getApplicationContext()
.sendBroadcast(new Intent(ConnectivityManager.CONNECTIVITY_ACTION));
assertThat(delegateChannel.resetCount).isEqualTo(1);
assertThat(delegateChannel.enterIdleCount).isEqualTo(1);

// Notify that a new but not connected network is available
shadowOf(connectivityManager).setActiveNetworkInfo(MOBILE_DISCONNECTED);
ApplicationProvider
.getApplicationContext()
.sendBroadcast(new Intent(ConnectivityManager.CONNECTIVITY_ACTION));
assertThat(delegateChannel.resetCount).isEqualTo(1);
assertThat(delegateChannel.enterIdleCount).isEqualTo(1);

// Establish a connection
shadowOf(connectivityManager).setActiveNetworkInfo(MOBILE_CONNECTED);
ApplicationProvider
.getApplicationContext()
.sendBroadcast(new Intent(ConnectivityManager.CONNECTIVITY_ACTION));
assertThat(delegateChannel.resetCount).isEqualTo(2);
assertThat(delegateChannel.enterIdleCount).isEqualTo(2);

// Disconnect, then shutdown the channel and verify that the broadcast receiver has been
// unregistered
Expand All @@ -193,72 +192,42 @@ public void resetConnectBackoff_api23() {
.getApplicationContext()
.sendBroadcast(new Intent(ConnectivityManager.CONNECTIVITY_ACTION));

assertThat(delegateChannel.resetCount).isEqualTo(2);
// enterIdle is not called on API levels < 24
assertThat(delegateChannel.enterIdleCount).isEqualTo(0);
assertThat(delegateChannel.enterIdleCount).isEqualTo(2);
}

@Test
@Config(sdk = 24)
public void resetConnectBackoffAndEnterIdle_api24() {
public void networkChanges_api24() {
shadowOf(connectivityManager).setActiveNetworkInfo(MOBILE_DISCONNECTED);
TestChannel delegateChannel = new TestChannel();
ManagedChannel androidChannel =
new AndroidChannelBuilder.AndroidChannel(
delegateChannel, ApplicationProvider.getApplicationContext());
assertThat(delegateChannel.resetCount).isEqualTo(0);
assertThat(delegateChannel.enterIdleCount).isEqualTo(0);

// Establish an initial network connection
shadowOf(connectivityManager).setActiveNetworkInfo(MOBILE_CONNECTED);
assertThat(delegateChannel.resetCount).isEqualTo(1);
assertThat(delegateChannel.enterIdleCount).isEqualTo(0);
assertThat(delegateChannel.enterIdleCount).isEqualTo(1);

// Switch to another network to trigger enterIdle()
shadowOf(connectivityManager).setActiveNetworkInfo(WIFI_CONNECTED);
assertThat(delegateChannel.resetCount).isEqualTo(1);
assertThat(delegateChannel.enterIdleCount).isEqualTo(1);
assertThat(delegateChannel.enterIdleCount).isEqualTo(2);

// Switch to an offline network and then to null
shadowOf(connectivityManager).setActiveNetworkInfo(WIFI_DISCONNECTED);
shadowOf(connectivityManager).setActiveNetworkInfo(null);
assertThat(delegateChannel.resetCount).isEqualTo(1);
assertThat(delegateChannel.enterIdleCount).isEqualTo(1);
assertThat(delegateChannel.enterIdleCount).isEqualTo(2);

// Establish a connection
shadowOf(connectivityManager).setActiveNetworkInfo(MOBILE_CONNECTED);
assertThat(delegateChannel.resetCount).isEqualTo(2);
assertThat(delegateChannel.enterIdleCount).isEqualTo(1);
assertThat(delegateChannel.enterIdleCount).isEqualTo(3);

// Disconnect, then shutdown the channel and verify that the callback has been unregistered
shadowOf(connectivityManager).setActiveNetworkInfo(null);
androidChannel.shutdown();
shadowOf(connectivityManager).setActiveNetworkInfo(MOBILE_CONNECTED);

assertThat(delegateChannel.resetCount).isEqualTo(2);
assertThat(delegateChannel.enterIdleCount).isEqualTo(1);
}

@Test
@Config(sdk = 24)
public void newChannelWithConnection_entersIdleOnSecondConnectionChange_api24() {
shadowOf(connectivityManager).setActiveNetworkInfo(MOBILE_CONNECTED);
TestChannel delegateChannel = new TestChannel();
ManagedChannel androidChannel =
new AndroidChannelBuilder.AndroidChannel(
delegateChannel, ApplicationProvider.getApplicationContext());

// The first onAvailable() may just signal that the device was connected when the callback is
// registered, rather than indicating a changed network, so we do not enter idle.
shadowOf(connectivityManager).setActiveNetworkInfo(WIFI_CONNECTED);
assertThat(delegateChannel.resetCount).isEqualTo(1);
assertThat(delegateChannel.enterIdleCount).isEqualTo(0);

shadowOf(connectivityManager).setActiveNetworkInfo(MOBILE_CONNECTED);
assertThat(delegateChannel.resetCount).isEqualTo(1);
assertThat(delegateChannel.enterIdleCount).isEqualTo(1);

androidChannel.shutdown();
assertThat(delegateChannel.enterIdleCount).isEqualTo(3);
}

@Test
Expand All @@ -279,7 +248,7 @@ public void shutdownNowUnregistersBroadcastReceiver_api23() {
.getApplicationContext()
.sendBroadcast(new Intent(ConnectivityManager.CONNECTIVITY_ACTION));

assertThat(delegateChannel.resetCount).isEqualTo(0);
assertThat(delegateChannel.enterIdleCount).isEqualTo(0);
}

@Test
Expand All @@ -294,7 +263,7 @@ public void shutdownNowUnregistersNetworkCallback_api24() {
androidChannel.shutdownNow();
shadowOf(connectivityManager).setActiveNetworkInfo(WIFI_CONNECTED);

assertThat(delegateChannel.resetCount).isEqualTo(0);
assertThat(delegateChannel.enterIdleCount).isEqualTo(0);
}

/**
Expand Down Expand Up @@ -358,7 +327,6 @@ public void unregisterNetworkCallback(ConnectivityManager.NetworkCallback networ
}

private static class TestChannel extends ManagedChannel {
int resetCount;
int enterIdleCount;

@Override
Expand Down Expand Up @@ -397,11 +365,6 @@ public String authority() {
return null;
}

@Override
public void resetConnectBackoff() {
resetCount++;
}

@Override
public void enterIdle() {
enterIdleCount++;
Expand Down

0 comments on commit b3429ec

Please sign in to comment.