android: add AndroidChannelBuilder #4172

ericgribkoff · 2018-03-05T23:30:04Z

Introduces a wrapper around an OkHttp channel that accepts an Android Contextand registers as a network listener to automatically invoke the necessary methods on ManagedChannel (enterIdle and resetConnectBackoff) when the device connection state changes.

This will have to be a new artifact with a direct Android dependency: interacting with the Android network monitoring APIs in the existing OkHttp package via reflection could work, but we need to provide the connectivity manager with implementations of Android callback interfaces, which would require using the java.lang.reflect.Proxy API to construct these reflectively. It seems much cleaner to just have (yet one more) new artifact for Android-specific functionality.

Manually tested with a local app. Robolectric tests to follow. This reduces the burden on users to take advantage of the gRPC channel API methods: this can all be accomplished with gRPC's public API, but doing so requires users to (a) keep track of every channel reference and (b) understand the Android network monitoring APIs, which have changed over time, and how to plumb these changes to the respective gRPC channel methods.

cc @srini100

ericgribkoff · 2018-03-16T07:52:18Z

I revised the AndroidChannel implementation somewhat and added Robolectric tests.

The tests cover both API levels before and after Android 24, which added the ConnectivityManager#registerDefaultNetworkCallback API. The registerDefaultNetworkCallback API is used to determine both when to invoke enterIdle for graceful network transitions and resetConnectBackoff when the device re-establishes a connection.

Devices with a pre-24 OS fallback to the older BroadcastReceiver API, which supports invoking resetConnectBackoff when the device comes back online but doesn't give the necessary signal to know when to invoke enterIdle. If it proves to be needed, we may be able to extract enough information out of pre-24 APIs to reliably determine when to invoke enterIdle, but the implementation of this would get messy compared to using the nicely suited new default network APIs. And, for now, resetConnectBackoff alone is enough for pre-24 devices to capture the primary benefit of having a connection-aware channel, avoiding downtime after the network comes back.

ericgribkoff · 2018-04-18T18:15:26Z

@ejona86 Friendly ping (branch cut for v1.12 is in five days, looks like this is definitely at risk for making the cut now?)

ejona86 · 2018-04-20T22:48:32Z

android/build.gradle

+    mavenCentral()
+    mavenLocal()
+    maven {
+        url "https://oss.sonatype.org/content/repositories/snapshots"


I don't think we want to pull in the snapshots repo.

ejona86 · 2018-04-20T22:49:46Z

android/build.gradle

+
+dependencies {
+    implementation 'io.grpc:grpc-core:1.12.0-SNAPSHOT' // CURRENT_GRPC_VERSION
+    implementation 'io.grpc:grpc-okhttp:1.12.0-SNAPSHOT' // CURRENT_GRPC_VERSION


I think I'd prefer to leave off the grpc-okhttp dependency for now, as it would be hard to remove to support mixing with cronet.

ejona86 · 2018-04-20T22:51:24Z

android/src/main/java/io/grpc/android/AndroidChannelBuilder.java

+
+/**
+ * Builds a {@link ManagedChannel} that automatically monitors the Android device's network state.
+ * Network changes are propagated to the underlying OkHttp-backed {@ManagedChannel} to smoothly


I don't think we need to specify implementation details like okhttp and "backed" and some delegate managedchannel.

ejona86 · 2018-04-20T22:53:30Z

android/src/main/java/io/grpc/android/AndroidChannelBuilder.java

+
+  /** Always fails. Call {@link #forAddress(String, int, Context)} instead. */
+  public static AndroidChannelBuilder forTarget(String target) {
+    throw new UnsupportedOperationException("call forTarget(String, Context) instead");


As discussed in the API review meeting, these could be supported and we'd remove the Context from the forTarget call.

Done. Context is now optional and set via the context() builder method.

ejona86 · 2018-04-20T22:58:28Z

android/src/main/java/io/grpc/android/AndroidChannelBuilder.java

+
+      // 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) {


Does this work for libraries? I thought many of these fields only work for applications, as when the library is compiled the final value isn't known and nothing rebuilds the library.

These work for libraries - they are statically available constants about the device, not compile-time build constants. Javadoc here, and these are used in Cronet and inAndroidPlatform in okhttp3.

ejona86 · 2018-04-20T23:04:43Z

android/src/main/java/io/grpc/android/AndroidChannelBuilder.java

+
+    private void unregisterNetworkListener() {
+      if (needToUnregisterListener) {
+        synchronized (lock) {


Do you need to check needToUnregisterListener again within the lock?

Yes, good catch - fixed by switch to guardedby

ejona86 · 2018-04-20T23:05:36Z

android/src/main/java/io/grpc/android/AndroidChannelBuilder.java

+    private final Object lock = new Object();
+
+    // May only go from true to false, and lock must be held when assigning this
+    private volatile boolean needToUnregisterListener = true;


I'd suggest making this just @GuardedBy("lock"). It's only going to be rarely used, because shutdown isn't called frequently. Little benefit in optimizing the avoidance of the lock.

ejona86 · 2018-04-20T23:06:25Z

android/src/main/java/io/grpc/android/AndroidChannelBuilder.java

+    private void unregisterNetworkListener() {
+      if (needToUnregisterListener) {
+        synchronized (lock) {
+          if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N && connectivityManager != null) {


Does repeating the Build check here help dead code elimination? If not, is it necessary?

Refactored to use runnable instead of this conditional

ejona86 · 2018-04-20T23:08:05Z

android/src/main/java/io/grpc/android/AndroidChannelBuilder.java

+    private final Context context;
+    private final ConnectivityManager connectivityManager;
+
+    private DefaultNetworkCallback defaultNetworkCallback;


Since these are only used for shut down, you could consider just storing a Runnable for clean-up. It's fine as-is though.

(I'm mainly considering it because of the Build.VERSION.SDK_INT >= Build.VERSION_CODES.N check during shut down)

ericgribkoff

Thanks for the review! Comments addressed.

ericgribkoff · 2018-04-21T04:24:56Z

android/build.gradle

+    mavenCentral()
+    mavenLocal()
+    maven {
+        url "https://oss.sonatype.org/content/repositories/snapshots"


ericgribkoff · 2018-04-21T04:57:13Z

android/src/main/java/io/grpc/android/AndroidChannelBuilder.java

+
+/**
+ * Builds a {@link ManagedChannel} that automatically monitors the Android device's network state.
+ * Network changes are propagated to the underlying OkHttp-backed {@ManagedChannel} to smoothly


ericgribkoff · 2018-04-21T04:57:17Z

android/build.gradle

+
+dependencies {
+    implementation 'io.grpc:grpc-core:1.12.0-SNAPSHOT' // CURRENT_GRPC_VERSION
+    implementation 'io.grpc:grpc-okhttp:1.12.0-SNAPSHOT' // CURRENT_GRPC_VERSION


ericgribkoff · 2018-04-21T05:28:00Z

android/src/main/java/io/grpc/android/AndroidChannelBuilder.java

+
+  /** Always fails. Call {@link #forAddress(String, int, Context)} instead. */
+  public static AndroidChannelBuilder forTarget(String target) {
+    throw new UnsupportedOperationException("call forTarget(String, Context) instead");


Done. Context is now optional and set via the context() builder method.

ericgribkoff · 2018-04-21T05:37:27Z

android/src/main/java/io/grpc/android/AndroidChannelBuilder.java

+
+      // 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) {


These work for libraries - they are statically available constants about the device, not compile-time build constants. Javadoc here, and these are used in Cronet and inAndroidPlatform in okhttp3.

ericgribkoff · 2018-04-21T05:38:02Z

android/src/main/java/io/grpc/android/AndroidChannelBuilder.java

+    private final Object lock = new Object();
+
+    // May only go from true to false, and lock must be held when assigning this
+    private volatile boolean needToUnregisterListener = true;


ericgribkoff · 2018-04-21T05:54:10Z

android/src/main/java/io/grpc/android/AndroidChannelBuilder.java

+    private final Context context;
+    private final ConnectivityManager connectivityManager;
+
+    private DefaultNetworkCallback defaultNetworkCallback;


ericgribkoff · 2018-04-21T05:55:47Z

android/src/main/java/io/grpc/android/AndroidChannelBuilder.java

+
+    private void unregisterNetworkListener() {
+      if (needToUnregisterListener) {
+        synchronized (lock) {


Yes, good catch - fixed by switch to guardedby

ericgribkoff · 2018-04-21T05:56:11Z

android/src/main/java/io/grpc/android/AndroidChannelBuilder.java

+    private void unregisterNetworkListener() {
+      if (needToUnregisterListener) {
+        synchronized (lock) {
+          if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N && connectivityManager != null) {


Refactored to use runnable instead of this conditional

…ne plugin

ejona86 · 2018-04-21T06:53:28Z

android/src/main/java/io/grpc/android/AndroidChannelBuilder.java

+
+  private static final Class<?> findClass(String className) {
+    try {
+      return Class.forName(className);


You need the literal Class.forName("io.grpc.okhttp.OkHttpChannelBuilder"), otherwise ProGuard won't detect the reference.

ejona86 · 2018-04-21T06:58:04Z

android/src/main/java/io/grpc/android/AndroidChannelBuilder.java

+        connectivityManager.registerDefaultNetworkCallback(defaultNetworkCallback);
+        unregisterRunnable =
+            new Runnable() {
+              @TargetApi(Build.VERSION_CODES.LOLLIPOP)


I'd like to understand these annotations at some point.

Analogous to SuppressWarnings. Without it there are linter errors about unregisterNetworkCallback requiring Android L in the runnable.

ejona86 · 2018-04-21T07:00:16Z

android/src/main/java/io/grpc/android/AndroidChannelBuilder.java

+    @Nullable private final Context context;
+    @Nullable private final ConnectivityManager connectivityManager;
+
+    @Nullable private DefaultNetworkCallback defaultNetworkCallback;


I had sort of imagined these two fields would just be local variables, since the Runnables could just get a closure of them. This is fine too.

ericgribkoff · 2018-04-23T16:56:55Z

@ejona86 Made a few small changes to address comments and catch the security exception thrown for apps that don't have the ACCESS_NETWORK_STATE permission. This should be ready to merge now, PTAL

ericgribkoff force-pushed the android_channel branch from 54a580d to d668b24 Compare March 6, 2018 02:29

ericgribkoff added the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Mar 9, 2018

kokoro-team removed the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Mar 9, 2018

ericgribkoff added the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Mar 9, 2018

kokoro-team removed the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Mar 9, 2018

android: add AndroidChannelBuilder

64a8fce

ericgribkoff force-pushed the android_channel branch from d668b24 to 64a8fce Compare March 16, 2018 07:22

tests

e0a9dd1

ericgribkoff requested review from zpencer and removed request for zpencer March 16, 2018 07:52

ericgribkoff assigned ejona86 Mar 20, 2018

ejona86 reviewed Apr 20, 2018

View reviewed changes

ericgribkoff commented Apr 21, 2018

View reviewed changes

ericgribkoff added 3 commits April 20, 2018 23:07

remove snapshot repo and direct okhttp dependency

7067773

make context optional via context() builder

03daad5

replace unregister boolean with Runnable, use @GuardedBy and errorpro…

4638d70

…ne plugin

ericgribkoff force-pushed the android_channel branch from 69b49bf to 4638d70 Compare April 21, 2018 06:08

ejona86 approved these changes Apr 21, 2018

View reviewed changes

ericgribkoff added 2 commits April 22, 2018 13:16

proguard, remove local vars

289c8c4

Catch and log warning if ACCESS_NETWORK_STATE permission is not granted

5ded152

ericgribkoff force-pushed the android_channel branch from 4f008e4 to d72570b Compare April 23, 2018 16:16

Merge branch 'master' into android_channel

6a928f4

ericgribkoff force-pushed the android_channel branch from d72570b to 6a928f4 Compare April 23, 2018 16:19

ejona86 approved these changes Apr 23, 2018

View reviewed changes

ericgribkoff merged commit ba86a86 into grpc:master Apr 23, 2018

ericgribkoff deleted the android_channel branch April 23, 2018 18:49

lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019

android: add AndroidChannelBuilder #4172

android: add AndroidChannelBuilder #4172

Conversation

ericgribkoff commented Mar 5, 2018 • edited Loading

ericgribkoff commented Mar 16, 2018

ericgribkoff commented Apr 18, 2018

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

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

ericgribkoff left a 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

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

ericgribkoff commented Apr 23, 2018

ericgribkoff commented Mar 5, 2018 •

edited

Loading