Skip to content

cronet : Update library + Move to StableAPIs in Cronet #11068

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
temawi merged 4 commits into from
Apr 22, 2024
Merged

cronet : Update library + Move to StableAPIs in Cronet #11068

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
42ff5a4
cronet: Update Cronet to latest release + Move to Stable Cronet APIs.
Ashok-Varma Apr 3, 2024
e728136
cronet: @javadoc update android permission MODIFY_NETWORK_ACCOUNTING …
Ashok-Varma Apr 7, 2024
ed457b5
cronet: Update to Java-8 API's and tighten the scopes
Ashok-Varma Apr 7, 2024
cb2e1af
cronet: Update to StandardCharsets and assertNotNull API's
Ashok-Varma Apr 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 11 additions & 81 deletions cronet/src/main/java/io/grpc/cronet/CronetChannelBuilder.java
View file
Open in desktop
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
Open in desktop
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
10 changes: 5 additions & 5 deletions cronet/src/main/java/io/grpc/cronet/CronetClientTransport.java
View file
Open in desktop
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 @@ -205,9 +205,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 +255,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
Open in desktop
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
Open in desktop
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