From 5d4c93e4b736aa9d959130fb66c9fda5bdf54e4c Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Tue, 17 Nov 2020 15:27:25 -0800 Subject: [PATCH] android: make Channel always enterIdle upon network recover (#7611) 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. --- .../grpc/android/AndroidChannelBuilder.java | 23 +----- .../android/AndroidChannelBuilderTest.java | 73 +++++-------------- 2 files changed, 21 insertions(+), 75 deletions(-) diff --git a/android/src/main/java/io/grpc/android/AndroidChannelBuilder.java b/android/src/main/java/io/grpc/android/AndroidChannelBuilder.java index 792170df5a9d..117ce936d2ef 100644 --- a/android/src/main/java/io/grpc/android/AndroidChannelBuilder.java +++ b/android/src/main/java/io/grpc/android/AndroidChannelBuilder.java @@ -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 { @@ -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(); } } @@ -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(); } } } diff --git a/android/src/test/java/io/grpc/android/AndroidChannelBuilderTest.java b/android/src/test/java/io/grpc/android/AndroidChannelBuilderTest.java index e35dbf66aa22..41847537682e 100644 --- a/android/src/test/java/io/grpc/android/AndroidChannelBuilderTest.java +++ b/android/src/test/java/io/grpc/android/AndroidChannelBuilderTest.java @@ -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 @@ -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 @@ -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 @@ -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 @@ -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); } /** @@ -358,7 +327,6 @@ public void unregisterNetworkCallback(ConnectivityManager.NetworkCallback networ } private static class TestChannel extends ManagedChannel { - int resetCount; int enterIdleCount; @Override @@ -397,11 +365,6 @@ public String authority() { return null; } - @Override - public void resetConnectBackoff() { - resetCount++; - } - @Override public void enterIdle() { enterIdleCount++;