From c92bd6ab4523de971e2c948af6eb0be0016e0fdd Mon Sep 17 00:00:00 2001 From: Charles Le Borgne Date: Tue, 10 May 2022 17:26:57 +0100 Subject: [PATCH 1/2] Add commnents to CronetBidirectionalStream Signed-off-by: Charles Le Borgne --- .../net/impl/CancelProofEnvoyStream.java | 1 + .../net/impl/CronetBidirectionalStream.java | 73 ++++++++++++++++++- .../chromium/net/BidirectionalStreamTest.java | 17 ++--- 3 files changed, 80 insertions(+), 11 deletions(-) diff --git a/library/java/org/chromium/net/impl/CancelProofEnvoyStream.java b/library/java/org/chromium/net/impl/CancelProofEnvoyStream.java index c037b9dbaf..b38cc6d580 100644 --- a/library/java/org/chromium/net/impl/CancelProofEnvoyStream.java +++ b/library/java/org/chromium/net/impl/CancelProofEnvoyStream.java @@ -76,6 +76,7 @@ void sendData(ByteBuffer buffer, boolean finalChunk) { if (buffer.position() == 0) { mStream.sendData(buffer, buffer.remaining(), finalChunk); } else { + // TODO(https://github.com/envoyproxy/envoy-mobile/issues/2247): avoid ByteBuffer copies ByteBuffer resizedBuffer = ByteBuffer.allocateDirect(buffer.remaining()); buffer.mark(); resizedBuffer.put(buffer); diff --git a/library/java/org/chromium/net/impl/CronetBidirectionalStream.java b/library/java/org/chromium/net/impl/CronetBidirectionalStream.java index eaa56ff068..be50e00fd5 100644 --- a/library/java/org/chromium/net/impl/CronetBidirectionalStream.java +++ b/library/java/org/chromium/net/impl/CronetBidirectionalStream.java @@ -40,6 +40,72 @@ /** * {@link BidirectionalStream} implementation using Envoy-Mobile stack. + * + *

C++ API differences between EM and Cronet: + *
The Cronet C++ API was carved to make the Java implementation of BidirectionalStream + * straightforward. EM C++ API is more bare bone. The missing/different logic is therefore being + * handled by this class. Here are the main differences: + *

+ * + *

Implementation strategy: + *
Implementation wise, the most noticeable difference between the Cronet implementation and + * this one is the avoidance of any java "synchronized". This implementation is based on "Compare + * And Swap" logic to guarantee correctness. The State of the Stream is kept in a single atomic + * Integer owned by {@link CronetBidirectionalState}. That state is a set of bits. Technically it + * could have been the conjunction of Enums held inside a single Integer. Using bits turned out + * to avoid more complex "if" logic. Still, the most important point here is the fact that the whole + * state is a single Atomic Integer: it eases the avoidance of race conditions, especially when + * "cancel" is involved. + *

+ * */ public final class CronetBidirectionalStream extends ExperimentalBidirectionalStream implements EnvoyHTTPCallbacks { @@ -53,12 +119,14 @@ public final class CronetBidirectionalStream private final Executor mExecutor; private final VersionSafeCallbacks.BidirectionalStreamCallback mCallback; private final String mInitialUrl; + // TODO(https://github.com/envoyproxy/envoy-mobile/issues/1641): Priority? What should we do. private final int mInitialPriority; private final String mMethod; private final boolean mReadOnly; // if mInitialMethod is GET or HEAD, then this is true. private final List> mRequestHeaders; private final boolean mDelayRequestHeadersUntilFirstFlush; private final Collection mRequestAnnotations; + // TODO(https://github.com/envoyproxy/envoy-mobile/issues/1521): implement traffic tagging. private final boolean mTrafficStatsTagSet; private final int mTrafficStatsTag; private final boolean mTrafficStatsUidSet; @@ -917,7 +985,8 @@ public void onData(ByteBuffer data, boolean endStream, EnvoyStreamIntel streamIn case NextAction.INVOKE_ON_READ_COMPLETED: ReadBuffer readBuffer = mLatestBufferRead.getAndSet(null); ByteBuffer userBuffer = readBuffer.mByteBuffer; - // TODO: copy buffer on network Thread - consider doing on the user Thread. + // TODO: this copies buffer on the Network Thread - consider doing on the user Thread. + // Or even better, revamp EM API to avoid as much as possible copying ByteBuffers. userBuffer.mark(); userBuffer.put(data); // NPE ==> BUG, BufferOverflowException ==> User not behaving. userBuffer.reset(); @@ -963,7 +1032,7 @@ public void onError(int errorCode, String message, int attemptCount, EnvoyStream mEnvoyFinalStreamIntel = finalStreamIntel; switch (mState.nextAction(Event.ON_ERROR)) { case NextAction.NOTIFY_USER_NETWORK_ERROR: - // TODO: fix error scheme. + // TODO(https://github.com/envoyproxy/envoy-mobile/issues/1594): fix error scheme. onErrorReceived(errorCode, /* nativeError= */ -1, /* nativeQuicError */ 0, message, finalStreamIntel.getReceivedByteCount()); break; diff --git a/test/java/org/chromium/net/BidirectionalStreamTest.java b/test/java/org/chromium/net/BidirectionalStreamTest.java index 35bf753517..512290db82 100644 --- a/test/java/org/chromium/net/BidirectionalStreamTest.java +++ b/test/java/org/chromium/net/BidirectionalStreamTest.java @@ -508,7 +508,6 @@ private ByteBuffer getDummyData() { @Feature({"Cronet"}) @OnlyRunNativeCronet public void testSimpleGetWithFlush() throws Exception { - // TODO(xunjieli): Use ParameterizedTest instead of the loop. for (int i = 0; i < 2; i++) { String url = Http2TestServer.getEchoStreamUrl(); TestBidirectionalStreamCallback callback = new TestBidirectionalStreamCallback() { @@ -562,7 +561,6 @@ public void onStreamReady(BidirectionalStream stream) { @Feature({"Cronet"}) @OnlyRunNativeCronet public void testSimplePostWithFlushAfterOneWrite() throws Exception { - // TODO(xunjieli): Use ParameterizedTest instead of the loop. for (int i = 0; i < 2; i++) { String url = Http2TestServer.getEchoStreamUrl(); TestBidirectionalStreamCallback callback = new TestBidirectionalStreamCallback(); @@ -591,7 +589,6 @@ public void testSimplePostWithFlushAfterOneWrite() throws Exception { @Feature({"Cronet"}) @OnlyRunNativeCronet public void testSimplePostWithFlushTwice() throws Exception { - // TODO(xunjieli): Use ParameterizedTest instead of the loop. for (int i = 0; i < 2; i++) { String url = Http2TestServer.getEchoStreamUrl(); TestBidirectionalStreamCallback callback = new TestBidirectionalStreamCallback(); @@ -1216,7 +1213,7 @@ public void testSimpleGetBufferUpdates() throws Exception { // The expected received bytes count is lower than it would be for the first request on the // connection, because the server includes an HPACK dynamic table size update only in the // first response HEADERS frame. - // TODO: fix expected ReceivedByteCount - quite unpredictable + // TODO(https://github.com/envoyproxy/envoy-mobile/issues/2265): fix expected ReceivedByteCount // runSimpleGetWithExpectedReceivedByteCount(27); } @@ -1300,8 +1297,9 @@ private void throwOrCancel(FailureType failureType, ResponseStep failureStep, failureStep == ResponseStep.ON_READ_COMPLETED || failureStep == ResponseStep.ON_TRAILERS) { // For steps after response headers are received, there will be // connect timing metrics. - // TODO(https://github.com/envoyproxy/envoy-mobile/issues/2192) uncomment this line - // MetricsTestUtil.checkTimingMetrics(metrics, startTime, endTime); + // TODO(https://github.com/envoyproxy/envoy-mobile/issues/2192): flaky. + MetricsTestUtil.checkTimingMetrics(metrics, startTime, endTime); + // TODO(https://github.com/envoyproxy/envoy-mobile/issues/2192): flaky. MetricsTestUtil.checkHasConnectTiming(metrics, startTime, endTime, true); assertTrue(metrics.getSentByteCount() > 0); assertTrue(metrics.getReceivedByteCount() > 0); @@ -1334,12 +1332,13 @@ private void throwOrCancel(FailureType failureType, ResponseStep failureStep, @OnlyRunNativeCronet @Ignore("Flaky: crashes EM") public void testFailures() throws Exception { - // TODO: start time and end time are not set. + // TODO(https://github.com/envoyproxy/envoy-mobile/issues/2192): start/end time are not set. // throwOrCancel(FailureType.CANCEL_SYNC, ResponseStep.ON_STREAM_READY, false); // throwOrCancel(FailureType.CANCEL_ASYNC, ResponseStep.ON_STREAM_READY, false); // throwOrCancel(FailureType.CANCEL_ASYNC_WITHOUT_PAUSE, ResponseStep.ON_STREAM_READY, false); // throwOrCancel(FailureType.THROW_SYNC, ResponseStep.ON_STREAM_READY, true); + // TODO(https://github.com/envoyproxy/envoy-mobile/issues/2192): start/end time are flaky. throwOrCancel(FailureType.CANCEL_SYNC, ResponseStep.ON_RESPONSE_STARTED, false); throwOrCancel(FailureType.CANCEL_ASYNC, ResponseStep.ON_RESPONSE_STARTED, false); throwOrCancel(FailureType.CANCEL_ASYNC_WITHOUT_PAUSE, ResponseStep.ON_RESPONSE_STARTED, false); @@ -1541,7 +1540,7 @@ public void testCronetEngineShutdownAfterStreamCancel() throws Exception { @Feature({"Cronet"}) @Test @OnlyRunNativeCronet - @Ignore("https://github.com/envoyproxy/envoy-mobile/issues/1550") + @Ignore("https://github.com/envoyproxy/envoy-mobile/issues/1594") public void testErrorCodes() throws Exception { // Non-BidirectionalStream specific error codes. checkSpecificErrorCode(NetError.ERR_NAME_NOT_RESOLVED, @@ -1561,7 +1560,7 @@ public void testErrorCodes() throws Exception { checkSpecificErrorCode(NetError.ERR_TIMED_OUT, NetworkException.ERROR_TIMED_OUT, true); checkSpecificErrorCode(NetError.ERR_ADDRESS_UNREACHABLE, NetworkException.ERROR_ADDRESS_UNREACHABLE, false); - // TODO("enable") + // TODO(https://github.com/envoyproxy/envoy-mobile/issues/1594) Missing error - code this. // BidirectionalStream specific retryable error codes. // checkSpecificErrorCode(NetError.ERR_HTTP2_PING_FAILED, NetworkException.ERROR_OTHER, true); // checkSpecificErrorCode( From ddfedfa9de86b7c5b933991cd56be7472b35ba09 Mon Sep 17 00:00:00 2001 From: Charles Le Borgne Date: Tue, 10 May 2022 17:43:13 +0100 Subject: [PATCH 2/2] Nit Signed-off-by: Charles Le Borgne --- .../java/org/chromium/net/impl/CronetBidirectionalStream.java | 1 - 1 file changed, 1 deletion(-) diff --git a/library/java/org/chromium/net/impl/CronetBidirectionalStream.java b/library/java/org/chromium/net/impl/CronetBidirectionalStream.java index be50e00fd5..0c9f0425ff 100644 --- a/library/java/org/chromium/net/impl/CronetBidirectionalStream.java +++ b/library/java/org/chromium/net/impl/CronetBidirectionalStream.java @@ -105,7 +105,6 @@ * invoked immediately. And to avoid invoking further Stream methods once "cancel" has been invoked, * a dedicated class handles this business: {@link CancelProofEnvoyStream}. * - * */ public final class CronetBidirectionalStream extends ExperimentalBidirectionalStream implements EnvoyHTTPCallbacks {