Skip to content

Commit

Permalink
Update Cronet to latest release. Move Away from ExperimentalAPIs in c…
Browse files Browse the repository at this point in the history 
…ronet.
  • Loading branch information
@Ashok-Varma
Ashok-Varma committed Apr 3, 2024
1 parent 58de563 commit 3e98f90
Show file tree
Hide file tree
Showing 8 changed files with 63 additions and 189 deletions.
92 changes: 11 additions & 81 deletions cronet/src/main/java/io/grpc/cronet/CronetChannelBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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(
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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;
Expand All @@ -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;
}
Expand All @@ -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;
Expand All @@ -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
Expand All @@ -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;
Expand All @@ -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");
Expand Down Expand Up @@ -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;
Expand All @@ -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);
}
}
}
}
}
53 changes: 8 additions & 45 deletions cronet/src/main/java/io/grpc/cronet/CronetClientStream.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

/**
Expand All @@ -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");
Expand Down Expand Up @@ -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);
}
}
}
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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.
Expand All @@ -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);
}
}
Expand Down Expand Up @@ -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));
Expand Down
21 changes: 9 additions & 12 deletions cronet/src/main/java/io/grpc/cronet/CronetClientTransport.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -79,7 +80,6 @@ class CronetClientTransport implements ConnectionClientTransport {
@GuardedBy("lock")
// Whether this transport has started.
private boolean started;
private StreamBuilderFactory streamFactory;

CronetClientTransport(
StreamBuilderFactory streamFactory,
Expand Down Expand Up @@ -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();
};
}

Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion cronet/src/main/java/io/grpc/cronet/InternalCronetChannelBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions cronet/src/test/java/io/grpc/cronet/CronetChannelBuilderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 =
Expand Down
Loading

0 comments on commit 3e98f90

Please sign in to comment.