android: initialize listener with isConnected=false (grpc#4606)

ericgribkoff · web-flow · commit 27a1c900a9dd · 2018-06-30T09:24:47.000-07:00
diff --git a/android/src/main/java/io/grpc/android/AndroidChannelBuilder.java b/android/src/main/java/io/grpc/android/AndroidChannelBuilder.java
@@ -187,37 +187,26 @@ static final class AndroidChannel extends ManagedChannel {
       if (context != null) {
         connectivityManager =
             (ConnectivityManager) context.getSystemService(Context.CONNECTIVITY_SERVICE);
-        configureNetworkMonitoring();
+        try {
+          configureNetworkMonitoring();
+        } catch (SecurityException e) {
+          Log.w(
+              LOG_TAG,
+              "Failed to configure network monitoring. Does app have ACCESS_NETWORK_STATE"
+                  + " permission?",
+              e);
+        }
       } else {
         connectivityManager = null;
       }
     }
 
     @GuardedBy("lock")
     private void configureNetworkMonitoring() {
-      // Eagerly check current network state to verify app has required permissions
-      NetworkInfo currentNetwork;
-      try {
-        currentNetwork = connectivityManager.getActiveNetworkInfo();
-      } catch (SecurityException e) {
-        Log.w(
-            LOG_TAG,
-            "Failed to configure network monitoring. Does app have ACCESS_NETWORK_STATE"
-                + " permission?",
-            e);
-        return;
-      }
-
       // Android N added the registerDefaultNetworkCallback API to listen to changes in the device's
       // default network. For earlier Android API levels, use the BroadcastReceiver API.
       if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N && connectivityManager != null) {
-        // The connection status may change before registration of the listener is complete, but
-        // this will at worst result in invoking resetConnectBackoff() instead of enterIdle() (or
-        // vice versa) on the first network change.
-        boolean isConnected = currentNetwork != null && currentNetwork.isConnected();
-
-        final DefaultNetworkCallback defaultNetworkCallback =
-            new DefaultNetworkCallback(isConnected);
+        final DefaultNetworkCallback defaultNetworkCallback = new DefaultNetworkCallback();
         connectivityManager.registerDefaultNetworkCallback(defaultNetworkCallback);
         unregisterRunnable =
             new Runnable() {
@@ -313,12 +302,13 @@ 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;
 
-      private DefaultNetworkCallback(boolean isConnected) {
-        this.isConnected = isConnected;
-      }
-
       @Override
       public void onAvailable(Network network) {
         if (isConnected) {
diff --git a/android/src/test/java/io/grpc/android/AndroidChannelBuilderTest.java b/android/src/test/java/io/grpc/android/AndroidChannelBuilderTest.java
@@ -256,15 +256,21 @@ public void resetConnectBackoffAndEnterIdle_api24() {
 
   @Test
   @Config(sdk = 24)
-  public void newChannelWithConnection_entersIdleOnConnectionChange_api24() {
+  public void newChannelWithConnection_entersIdleOnSecondConnectionChange_api24() {
     shadowConnectivityManager.setActiveNetworkInfo(MOBILE_CONNECTED);
     TestChannel delegateChannel = new TestChannel();
     ManagedChannel androidChannel =
         new AndroidChannelBuilder.AndroidChannel(
             delegateChannel, RuntimeEnvironment.application.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.
     shadowConnectivityManager.setActiveNetworkInfo(WIFI_CONNECTED);
-    assertThat(delegateChannel.resetCount).isEqualTo(0);
+    assertThat(delegateChannel.resetCount).isEqualTo(1);
+    assertThat(delegateChannel.enterIdleCount).isEqualTo(0);
+
+    shadowConnectivityManager.setActiveNetworkInfo(MOBILE_CONNECTED);
+    assertThat(delegateChannel.resetCount).isEqualTo(1);
     assertThat(delegateChannel.enterIdleCount).isEqualTo(1);
 
     androidChannel.shutdown();
