Update Cronet to latest release. Move Away from ExperimentalAPIs in cronet.

Author: Ashok-Varma

cronet/src/main/java/io/grpc/cronet/CronetChannelBuilder.java

@@ -20,7 +20,6 @@
 import static com.google.common.base.Preconditions.checkNotNull;
 import static io.grpc.internal.GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE;
 
-import android.util.Log;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.common.util.concurrent.MoreExecutors;
@@ -38,8 +37,6 @@
 import io.grpc.internal.ManagedChannelImplBuilder.ClientTransportFactoryBuilder;
 import io.grpc.internal.SharedResourceHolder;
 import io.grpc.internal.TransportTracer;
-import java.lang.reflect.InvocationTargetException;
-import java.lang.reflect.Method;
 import java.net.InetSocketAddress;
 import java.net.SocketAddress;
 import java.util.Collection;
@@ -49,15 +46,11 @@
 import javax.annotation.Nullable;
 import org.chromium.net.BidirectionalStream;
 import org.chromium.net.CronetEngine;
-import org.chromium.net.ExperimentalBidirectionalStream;
-import org.chromium.net.ExperimentalCronetEngine;
 
 /** Convenience class for building channels with the cronet transport. */
 @ExperimentalApi("There is no plan to make this API stable, given transport API instability")
 public final class CronetChannelBuilder extends ForwardingChannelBuilder2<CronetChannelBuilder> {
 
-  private static final String LOG_TAG = "CronetChannelBuilder";
-
   /** BidirectionalStream.Builder factory used for getting the gRPC BidirectionalStream. */
   public static abstract class StreamBuilderFactory {
     public abstract BidirectionalStream.Builder newBidirectionalStreamBuilder(
@@ -91,7 +84,7 @@ public static CronetChannelBuilder forAddress(String name, int port) {
 
   private final CronetEngine cronetEngine;
   private final ManagedChannelImplBuilder managedChannelImplBuilder;
-  private TransportTracer.Factory transportTracerFactory = TransportTracer.getDefaultFactory();
+  private final TransportTracer.Factory transportTracerFactory = TransportTracer.getDefaultFactory();
 
   private boolean alwaysUsePut = false;
 
@@ -139,7 +132,7 @@ protected ManagedChannelBuilder<?> delegate() {
    * Sets the maximum message size allowed to be received on the channel. If not called,
    * defaults to {@link io.grpc.internal.GrpcUtil#DEFAULT_MAX_MESSAGE_SIZE}.
    */
-  public final CronetChannelBuilder maxMessageSize(int maxMessageSize) {
+  public CronetChannelBuilder maxMessageSize(int maxMessageSize) {
     checkArgument(maxMessageSize >= 0, "maxMessageSize must be >= 0");
     this.maxMessageSize = maxMessageSize;
     return this;
@@ -148,7 +141,7 @@ public final CronetChannelBuilder maxMessageSize(int maxMessageSize) {
   /**
    * Sets the Cronet channel to always use PUT instead of POST. Defaults to false.
    */
-  public final CronetChannelBuilder alwaysUsePut(boolean enable) {
+  public CronetChannelBuilder alwaysUsePut(boolean enable) {
     this.alwaysUsePut = enable;
     return this;
   }
@@ -170,7 +163,7 @@ public final CronetChannelBuilder alwaysUsePut(boolean enable) {
    *     application.
    * @return the builder to facilitate chaining.
    */
-  final CronetChannelBuilder setTrafficStatsTag(int tag) {
+  CronetChannelBuilder setTrafficStatsTag(int tag) {
     trafficStatsTagSet = true;
     trafficStatsTag = tag;
     return this;
@@ -180,7 +173,7 @@ final CronetChannelBuilder setTrafficStatsTag(int tag) {
    * Sets specific UID to use when accounting socket traffic caused by this channel. See {@link
    * android.net.TrafficStats} for more information. Designed for use when performing an operation
    * on behalf of another application. Caller must hold {@link
-   * android.Manifest.permission#MODIFY_NETWORK_ACCOUNTING} permission. By default traffic is
+   * android.Manifest.permission#UPDATE_DEVICE_STATS} permission. By default traffic is
    * attributed to UID of caller.
    *
    * <p><b>NOTE:</b>Setting a UID disallows sharing of sockets with channels with other UIDs, which
@@ -191,7 +184,7 @@ final CronetChannelBuilder setTrafficStatsTag(int tag) {
    * @param uid the UID to attribute socket traffic caused by this channel.
    * @return the builder to facilitate chaining.
    */
-  final CronetChannelBuilder setTrafficStatsUid(int uid) {
+  CronetChannelBuilder setTrafficStatsUid(int uid) {
     trafficStatsUidSet = true;
     trafficStatsUid = uid;
     return this;
@@ -207,7 +200,7 @@ final CronetChannelBuilder setTrafficStatsUid(int uid) {
    *
    * @since 1.12.0
    */
-  public final CronetChannelBuilder scheduledExecutorService(
+  public CronetChannelBuilder scheduledExecutorService(
       ScheduledExecutorService scheduledExecutorService) {
     this.scheduledExecutorService =
         checkNotNull(scheduledExecutorService, "scheduledExecutorService");
@@ -296,11 +289,6 @@ public Collection<Class<? extends SocketAddress>> getSupportedSocketAddressTypes
    * StreamBuilderFactory impl that applies TrafficStats tags to stream builders that are produced.
    */
   private static class TaggingStreamFactory extends StreamBuilderFactory {
-    private static volatile boolean loadSetTrafficStatsTagAttempted;
-    private static volatile boolean loadSetTrafficStatsUidAttempted;
-    private static volatile Method setTrafficStatsTagMethod;
-    private static volatile Method setTrafficStatsUidMethod;
-
     private final CronetEngine cronetEngine;
     private final boolean trafficStatsTagSet;
     private final int trafficStatsTag;
@@ -323,74 +311,16 @@ private static class TaggingStreamFactory extends StreamBuilderFactory {
     @Override
     public BidirectionalStream.Builder newBidirectionalStreamBuilder(
         String url, BidirectionalStream.Callback callback, Executor executor) {
-      ExperimentalBidirectionalStream.Builder builder =
-          ((ExperimentalCronetEngine) cronetEngine)
+      BidirectionalStream.Builder builder =
+          cronetEngine
               .newBidirectionalStreamBuilder(url, callback, executor);
       if (trafficStatsTagSet) {
-        setTrafficStatsTag(builder, trafficStatsTag);
+        builder.setTrafficStatsTag(trafficStatsTag);
       }
       if (trafficStatsUidSet) {
-        setTrafficStatsUid(builder, trafficStatsUid);
+        builder.setTrafficStatsUid(trafficStatsUid);
       }
       return builder;
     }
-
-    private static void setTrafficStatsTag(ExperimentalBidirectionalStream.Builder builder,
-        int tag) {
-      if (!loadSetTrafficStatsTagAttempted) {
-        synchronized (TaggingStreamFactory.class) {
-          if (!loadSetTrafficStatsTagAttempted) {
-            try {
-              setTrafficStatsTagMethod = ExperimentalBidirectionalStream.Builder.class
-                  .getMethod("setTrafficStatsTag", int.class);
-            } catch (NoSuchMethodException e) {
-              Log.w(LOG_TAG,
-                  "Failed to load method ExperimentalBidirectionalStream.Builder.setTrafficStatsTag",
-                  e);
-            } finally {
-              loadSetTrafficStatsTagAttempted = true;
-            }
-          }
-        }
-      }
-      if (setTrafficStatsTagMethod != null) {
-        try {
-          setTrafficStatsTagMethod.invoke(builder, tag);
-        } catch (InvocationTargetException e) {
-          throw new RuntimeException(e.getCause() == null ? e.getTargetException() : e.getCause());
-        } catch (IllegalAccessException e) {
-          Log.w(LOG_TAG, "Failed to set traffic stats tag: " + tag, e);
-        }
-      }
-    }
-
-    private static void setTrafficStatsUid(ExperimentalBidirectionalStream.Builder builder,
-        int uid) {
-      if (!loadSetTrafficStatsUidAttempted) {
-        synchronized (TaggingStreamFactory.class) {
-          if (!loadSetTrafficStatsUidAttempted) {
-            try {
-              setTrafficStatsUidMethod = ExperimentalBidirectionalStream.Builder.class
-                  .getMethod("setTrafficStatsUid", int.class);
-            } catch (NoSuchMethodException e) {
-              Log.w(LOG_TAG,
-                  "Failed to load method ExperimentalBidirectionalStream.Builder.setTrafficStatsUid",
-                  e);
-            } finally {
-              loadSetTrafficStatsUidAttempted = true;
-            }
-          }
-        }
-      }
-      if (setTrafficStatsUidMethod != null) {
-        try {
-          setTrafficStatsUidMethod.invoke(builder, uid);
-        } catch (InvocationTargetException e) {
-          throw new RuntimeException(e.getCause() == null ? e.getTargetException() : e.getCause());
-        } catch (IllegalAccessException e) {
-          Log.w(LOG_TAG, "Failed to set traffic stats uid: " + uid, e);
-        }
-      }
-    }
   }
 }

cronet/src/main/java/io/grpc/cronet/CronetClientStream.java

@@ -40,11 +40,9 @@
 import io.grpc.internal.TransportFrameUtil;
 import io.grpc.internal.TransportTracer;
 import io.grpc.internal.WritableBuffer;
-import java.lang.reflect.InvocationTargetException;
-import java.lang.reflect.Method;
 import java.nio.Buffer;
 import java.nio.ByteBuffer;
-import java.nio.charset.Charset;
+import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
@@ -55,7 +53,6 @@
 import javax.annotation.concurrent.GuardedBy;
 import org.chromium.net.BidirectionalStream;
 import org.chromium.net.CronetException;
-import org.chromium.net.ExperimentalBidirectionalStream;
 import org.chromium.net.UrlResponseInfo;
 
 /**
@@ -66,9 +63,6 @@ class CronetClientStream extends AbstractClientStream {
   private static final ByteBuffer EMPTY_BUFFER = ByteBuffer.allocateDirect(0);
   private static final String LOG_TAG = "grpc-java-cronet";
 
-  private static volatile boolean loadAddRequestAnnotationAttempted;
-  private static volatile Method addRequestAnnotationMethod;
-
   @Deprecated
   static final CallOptions.Key<Object> CRONET_ANNOTATION_KEY =
       CallOptions.Key.create("cronet-annotation");
@@ -194,14 +188,12 @@ public void writeHeaders(Metadata metadata, byte[] payload) {
         builder.delayRequestHeadersUntilFirstFlush(true);
       }
       if (annotation != null || annotations != null) {
-        ExperimentalBidirectionalStream.Builder expBidiStreamBuilder =
-            (ExperimentalBidirectionalStream.Builder) builder;
         if (annotation != null) {
-          addRequestAnnotation(expBidiStreamBuilder, annotation);
+          builder.addRequestAnnotation(annotation);
         }
         if (annotations != null) {
           for (Object o : annotations) {
-            addRequestAnnotation(expBidiStreamBuilder, o);
+            builder.addRequestAnnotation(o);
           }
         }
       }
@@ -255,7 +247,7 @@ public void cancel(Status reason) {
   class TransportState extends Http2ClientStreamTransportState {
     private final Object lock;
     @GuardedBy("lock")
-    private Collection<PendingData> pendingData = new ArrayList<PendingData>();
+    private final Collection<PendingData> pendingData = new ArrayList<>();
     @GuardedBy("lock")
     private boolean streamReady;
     @GuardedBy("lock")
@@ -367,35 +359,6 @@ private static boolean isApplicationHeader(String key) {
         && !TE_HEADER.name().equalsIgnoreCase(key);
   }
 
-  private static void addRequestAnnotation(ExperimentalBidirectionalStream.Builder builder,
-      Object annotation) {
-    if (!loadAddRequestAnnotationAttempted) {
-      synchronized (CronetClientStream.class) {
-        if (!loadAddRequestAnnotationAttempted) {
-          try {
-            addRequestAnnotationMethod = ExperimentalBidirectionalStream.Builder.class
-                .getMethod("addRequestAnnotation", Object.class);
-          } catch (NoSuchMethodException e) {
-            Log.w(LOG_TAG,
-                "Failed to load method ExperimentalBidirectionalStream.Builder.addRequestAnnotation",
-                e);
-          } finally {
-            loadAddRequestAnnotationAttempted = true;
-          }
-        }
-      }
-    }
-    if (addRequestAnnotationMethod != null) {
-      try {
-        addRequestAnnotationMethod.invoke(builder, annotation);
-      } catch (InvocationTargetException e) {
-        throw new RuntimeException(e.getCause() == null ? e.getTargetException() : e.getCause());
-      } catch (IllegalAccessException e) {
-        Log.w(LOG_TAG, "Failed to add request annotation: " + annotation, e);
-      }
-    }
-  }
-
   private void setGrpcHeaders(BidirectionalStream.Builder builder) {
     // Psuedo-headers are set by cronet.
     // All non-pseudo headers must come after pseudo headers.
@@ -409,10 +372,10 @@ private void setGrpcHeaders(BidirectionalStream.Builder builder) {
     // String and byte array.
     byte[][] serializedHeaders = TransportFrameUtil.toHttp2Headers(headers);
     for (int i = 0; i < serializedHeaders.length; i += 2) {
-      String key = new String(serializedHeaders[i], Charset.forName("UTF-8"));
+      String key = new String(serializedHeaders[i], StandardCharsets.UTF_8);
       // TODO(ericgribkoff): log an error or throw an exception
       if (isApplicationHeader(key)) {
-        String value = new String(serializedHeaders[i + 1], Charset.forName("UTF-8"));
+        String value = new String(serializedHeaders[i + 1], StandardCharsets.UTF_8);
         builder.addHeader(key, value);
       }
     }
@@ -589,8 +552,8 @@ private void reportHeaders(List<Map.Entry<String, String>> headers, boolean endO
 
       byte[][] headerValues = new byte[headerList.size()][];
       for (int i = 0; i < headerList.size(); i += 2) {
-        headerValues[i] = headerList.get(i).getBytes(Charset.forName("UTF-8"));
-        headerValues[i + 1] = headerList.get(i + 1).getBytes(Charset.forName("UTF-8"));
+        headerValues[i] = headerList.get(i).getBytes(StandardCharsets.UTF_8);
+        headerValues[i + 1] = headerList.get(i + 1).getBytes(StandardCharsets.UTF_8);
       }
       Metadata metadata =
           InternalMetadata.newMetadata(TransportFrameUtil.toRawSerializedHeaders(headerValues));

cronet/src/main/java/io/grpc/cronet/CronetClientTransport.java

@@ -56,14 +56,15 @@ class CronetClientTransport implements ConnectionClientTransport {
   private final Object lock = new Object();
   @GuardedBy("lock")
   private final Set<CronetClientStream> streams = Collections.newSetFromMap(
-          new IdentityHashMap<CronetClientStream, Boolean>());
+          new IdentityHashMap<>());
   private final Executor executor;
   private final int maxMessageSize;
   private final boolean alwaysUsePut;
   private final TransportTracer transportTracer;
   private Attributes attrs;
   private final boolean useGetForSafeMethods;
   private final boolean usePutForIdempotentMethods;
+  private final StreamBuilderFactory streamFactory;
   // Indicates the transport is in go-away state: no new streams will be processed,
   // but existing streams may continue.
   @GuardedBy("lock")
@@ -79,7 +80,6 @@ class CronetClientTransport implements ConnectionClientTransport {
   @GuardedBy("lock")
   // Whether this transport has started.
   private boolean started;
-  private StreamBuilderFactory streamFactory;
 
   CronetClientTransport(
       StreamBuilderFactory streamFactory,
@@ -166,13 +166,10 @@ public Runnable start(Listener listener) {
     synchronized (lock) {
       started = true;
     }
-    return new Runnable() {
-      @Override
-      public void run() {
-        attrs = CronetClientTransport.this.listener.filterTransport(attrs);
-        // Listener callbacks should not be called simultaneously
-        CronetClientTransport.this.listener.transportReady();
-      }
+    return () -> {
+      attrs = CronetClientTransport.this.listener.filterTransport(attrs);
+      // Listener callbacks should not be called simultaneously
+      CronetClientTransport.this.listener.transportReady();
     };
   }
 
@@ -205,9 +202,9 @@ public void shutdownNow(Status status) {
       // streams.remove()
       streamsCopy = new ArrayList<>(streams);
     }
-    for (int i = 0; i < streamsCopy.size(); i++) {
+    for (CronetClientStream cronetClientStream : streamsCopy) {
       // Avoid deadlock by calling into stream without lock held
-      streamsCopy.get(i).cancel(status);
+      cronetClientStream.cancel(status);
     }
     stopIfNecessary();
   }
@@ -255,7 +252,7 @@ public InternalLogId getLogId() {
    */
   void stopIfNecessary() {
     synchronized (lock) {
-      if (goAway && !stopped && streams.size() == 0) {
+      if (goAway && !stopped && streams.isEmpty()) {
         stopped = true;
       } else {
         return;

cronet/src/main/java/io/grpc/cronet/InternalCronetChannelBuilder.java

@@ -47,7 +47,7 @@ public static void setTrafficStatsTag(CronetChannelBuilder builder, int tag) {
    * Sets specific UID to use when accounting socket traffic caused by this channel. See {@link
    * android.net.TrafficStats} for more information. Designed for use when performing an operation
    * on behalf of another application. Caller must hold {@link
-   * android.Manifest.permission#MODIFY_NETWORK_ACCOUNTING} permission. By default traffic is
+   * android.Manifest.permission#UPDATE_DEVICE_STATS} permission. By default traffic is
    * attributed to UID of caller.
    *
    * <p><b>NOTE:</b>Setting a UID disallows sharing of sockets with channels with other UIDs, which

cronet/src/test/java/io/grpc/cronet/CronetChannelBuilderTest.java

@@ -35,7 +35,7 @@
 import io.grpc.testing.TestMethodDescriptors;
 import java.net.InetSocketAddress;
 import java.util.concurrent.ScheduledExecutorService;
-import org.chromium.net.ExperimentalCronetEngine;
+import org.chromium.net.CronetEngine;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -50,7 +50,7 @@
 public final class CronetChannelBuilderTest {
   @Rule public final MockitoRule mocks = MockitoJUnit.rule();
 
-  @Mock private ExperimentalCronetEngine mockEngine;
+  @Mock private CronetEngine mockEngine;
   @Mock private ChannelLogger channelLogger;
 
   private final ClientStreamTracer[] tracers =