From f396a9bee0bccfb1e711857eb6f4a62cbcb9b07e Mon Sep 17 00:00:00 2001 From: Charles Le Borgne Date: Sun, 16 Jan 2022 21:50:04 +0000 Subject: [PATCH 01/12] Partially enable the "received byte count" feature. Signed-off-by: Charles Le Borgne --- .../filters/http/platform_bridge/filter.cc | 2 +- library/common/http/client.cc | 3 ++ library/common/http/client.h | 2 +- library/common/jni/jni_utility.cc | 3 +- library/common/types/c_types.h | 2 ++ .../engine/EnvoyStreamIntelImpl.java | 7 ++++ .../engine/types/EnvoyStreamIntel.java | 4 +++ .../chromium/net/impl/CronetUrlRequest.java | 32 +++++++++++++++++-- .../envoymobile/mocks/MockStream.kt | 1 + .../integration/client_integration_test.cc | 7 ++-- .../chromium/net/CronetUrlRequestTest.java | 14 +++++--- .../chromium/net/testing/CronetTestRule.java | 8 +++-- 12 files changed, 69 insertions(+), 16 deletions(-) diff --git a/library/common/extensions/filters/http/platform_bridge/filter.cc b/library/common/extensions/filters/http/platform_bridge/filter.cc index d107d718c7..02929ad467 100644 --- a/library/common/extensions/filters/http/platform_bridge/filter.cc +++ b/library/common/extensions/filters/http/platform_bridge/filter.cc @@ -202,7 +202,7 @@ envoy_stream_intel PlatformBridgeFilter::streamIntel() { RELEASE_ASSERT(decoder_callbacks_, "StreamInfo accessed before filter callbacks are set"); auto& info = decoder_callbacks_->streamInfo(); // FIXME: Stream handle cannot currently be set from the filter context. - envoy_stream_intel stream_intel{-1, -1, 0}; + envoy_stream_intel stream_intel{-1, -1, 0, 0}; if (info.upstreamInfo()) { stream_intel.connection_id = info.upstreamInfo()->upstreamConnectionId().value_or(-1); } diff --git a/library/common/http/client.cc b/library/common/http/client.cc index 0095bd57c9..0e379497ed 100644 --- a/library/common/http/client.cc +++ b/library/common/http/client.cc @@ -298,6 +298,9 @@ void Client::DirectStream::saveLatestStreamIntel() { } stream_intel_.stream_id = static_cast(stream_handle_); stream_intel_.attempt_count = info.attemptCount().value_or(0); + if (info.getUpstreamBytesMeter()) { + stream_intel_.received_byte_count = info.getUpstreamBytesMeter()->wireBytesReceived(); + } } void Client::DirectStream::saveFinalStreamIntel() { diff --git a/library/common/http/client.h b/library/common/http/client.h index 623cd399b4..40fbcf567d 100644 --- a/library/common/http/client.h +++ b/library/common/http/client.h @@ -279,7 +279,7 @@ class Client : public Logger::Loggable { // read faster than the mobile caller can process it. bool explicit_flow_control_ = false; // Latest intel data retrieved from the StreamInfo. - envoy_stream_intel stream_intel_{-1, -1, 0}; + envoy_stream_intel stream_intel_{-1, -1, 0, 0}; envoy_final_stream_intel envoy_final_stream_intel_; StreamInfo::BytesMeterSharedPtr bytes_meter_; }; diff --git a/library/common/jni/jni_utility.cc b/library/common/jni/jni_utility.cc index 7362673d59..5cc999c188 100644 --- a/library/common/jni/jni_utility.cc +++ b/library/common/jni/jni_utility.cc @@ -85,12 +85,13 @@ jbyteArray native_data_to_array(JNIEnv* env, envoy_data data) { } jlongArray native_stream_intel_to_array(JNIEnv* env, envoy_stream_intel stream_intel) { - jlongArray j_array = env->NewLongArray(3); + jlongArray j_array = env->NewLongArray(4); jlong* critical_array = static_cast(env->GetPrimitiveArrayCritical(j_array, nullptr)); RELEASE_ASSERT(critical_array != nullptr, "unable to allocate memory in jni_utility"); critical_array[0] = static_cast(stream_intel.stream_id); critical_array[1] = static_cast(stream_intel.connection_id); critical_array[2] = static_cast(stream_intel.attempt_count); + critical_array[3] = static_cast(stream_intel.received_byte_count); // Here '0' (for which there is no named constant) indicates we want to commit the changes back // to the JVM and free the c array, where applicable. env->ReleasePrimitiveArrayCritical(j_array, critical_array, 0); diff --git a/library/common/types/c_types.h b/library/common/types/c_types.h index 417a1872aa..cfe55498e7 100644 --- a/library/common/types/c_types.h +++ b/library/common/types/c_types.h @@ -153,6 +153,8 @@ typedef struct { int64_t connection_id; // The number of internal attempts to carry out a request/operation. 0 if not present. uint64_t attempt_count; + // The number of bytes received from upstream. + uint64_t received_byte_count; } envoy_stream_intel; /** diff --git a/library/java/io/envoyproxy/envoymobile/engine/EnvoyStreamIntelImpl.java b/library/java/io/envoyproxy/envoymobile/engine/EnvoyStreamIntelImpl.java index 3e6f35a5d8..47e4cb1ed0 100644 --- a/library/java/io/envoyproxy/envoymobile/engine/EnvoyStreamIntelImpl.java +++ b/library/java/io/envoyproxy/envoymobile/engine/EnvoyStreamIntelImpl.java @@ -6,11 +6,13 @@ class EnvoyStreamIntelImpl implements EnvoyStreamIntel { private long streamId; private long connectionId; private long attemptCount; + private long receivedByteCount; EnvoyStreamIntelImpl(long[] values) { streamId = values[0]; connectionId = values[1]; attemptCount = values[2]; + receivedByteCount = values[3]; } @Override @@ -27,4 +29,9 @@ public long getConnectionId() { public long getAttemptCount() { return attemptCount; } + + @Override + public long getReceivedByteCount() { + return receivedByteCount; + } } diff --git a/library/java/io/envoyproxy/envoymobile/engine/types/EnvoyStreamIntel.java b/library/java/io/envoyproxy/envoymobile/engine/types/EnvoyStreamIntel.java index 5a8a305350..a64409135c 100644 --- a/library/java/io/envoyproxy/envoymobile/engine/types/EnvoyStreamIntel.java +++ b/library/java/io/envoyproxy/envoymobile/engine/types/EnvoyStreamIntel.java @@ -18,4 +18,8 @@ public interface EnvoyStreamIntel { * The number of internal attempts to carry out a request/operation. */ public long getAttemptCount(); + /* + * The number of bytes received from upstream. + */ + public long getReceivedByteCount(); } diff --git a/library/java/org/chromium/net/impl/CronetUrlRequest.java b/library/java/org/chromium/net/impl/CronetUrlRequest.java index 4190214389..36b68f5476 100644 --- a/library/java/org/chromium/net/impl/CronetUrlRequest.java +++ b/library/java/org/chromium/net/impl/CronetUrlRequest.java @@ -88,7 +88,6 @@ public final class CronetUrlRequest extends UrlRequestBase { private final String mUserAgent; private final HeadersList mRequestHeaders = new HeadersList(); - private final List mUrlChain = new ArrayList<>(); private final CronetUrlRequestContext mRequestContext; private final AtomicBoolean mWaitingOnRedirect = new AtomicBoolean(false); private final AtomicBoolean mWaitingOnRead = new AtomicBoolean(false); @@ -131,6 +130,8 @@ public final class CronetUrlRequest extends UrlRequestBase { /* These change with redirects. */ private final AtomicReference mStream = new AtomicReference<>(); + private final List mUrlChain = new ArrayList<>(); + private final List mEnvoyFinalStreamIntels = new ArrayList<>(); private CronvoyHttpCallbacks mCronvoyCallbacks; private String mCurrentUrl; private UrlResponseInfoImpl mUrlResponseInfo; @@ -631,6 +632,25 @@ private boolean streamEnded() { return cronvoyCallbacks != null && cronvoyCallbacks.mEndStream; } + private void recordEnvoyFinalStreamIntel(EnvoyFinalStreamIntel envoyFinalStreamIntel) { + mEnvoyFinalStreamIntels.add(envoyFinalStreamIntel); + long bytesReceived = 0; + // This in only called by the network Thread - no concurrency issue. + for (EnvoyFinalStreamIntel intel : mEnvoyFinalStreamIntels) { + bytesReceived += intel.getReceivedByteCount(); + } + mUrlResponseInfo.setReceivedByteCount(bytesReceived); + } + + private void recordEnvoyStreamIntel(EnvoyStreamIntel envoyStreamIntel) { + long bytesReceived = envoyStreamIntel.getReceivedByteCount(); + // This in only called by the network Thread - no concurrency issue. + for (EnvoyFinalStreamIntel intel : mEnvoyFinalStreamIntels) { + bytesReceived += intel.getReceivedByteCount(); + } + mUrlResponseInfo.setReceivedByteCount(bytesReceived); + } + private static class HeadersList extends ArrayList> {} private static class DirectExecutor implements Executor { @@ -666,6 +686,7 @@ public Executor getExecutor() { @Override public void onHeaders(Map> headers, boolean endStream, EnvoyStreamIntel streamIntel) { + recordEnvoyStreamIntel(streamIntel); if (isAbandoned()) { return; } @@ -731,6 +752,7 @@ public void run() { @Override public void onData(ByteBuffer data, boolean endStream, EnvoyStreamIntel streamIntel) { + recordEnvoyStreamIntel(streamIntel); if (isAbandoned()) { return; } @@ -768,6 +790,7 @@ public void run() { @Override public void onTrailers(Map> trailers, EnvoyStreamIntel streamIntel) { + recordEnvoyStreamIntel(streamIntel); if (isAbandoned()) { return; } @@ -786,6 +809,7 @@ public void onTrailers(Map> trailers, EnvoyStreamIntel stre @Override public void onError(int errorCode, String message, int attemptCount, EnvoyStreamIntel streamIntel, EnvoyFinalStreamIntel finalStreamIntel) { + recordEnvoyFinalStreamIntel(finalStreamIntel); if (isAbandoned()) { return; } @@ -808,6 +832,7 @@ public void onError(int errorCode, String message, int attemptCount, @Override public void onCancel(EnvoyStreamIntel streamIntel, EnvoyFinalStreamIntel finalStreamIntel) { + recordEnvoyFinalStreamIntel(finalStreamIntel); if (isAbandoned()) { return; } @@ -828,6 +853,7 @@ public void onCancel(EnvoyStreamIntel streamIntel, EnvoyFinalStreamIntel finalSt @Override public void onSendWindowAvailable(EnvoyStreamIntel streamIntel) { + recordEnvoyStreamIntel(streamIntel); if (isAbandoned()) { return; } @@ -849,10 +875,10 @@ public void onSendWindowAvailable(EnvoyStreamIntel streamIntel) { @Override public void onComplete(EnvoyStreamIntel streamIntel, EnvoyFinalStreamIntel finalStreamIntel) { + recordEnvoyFinalStreamIntel(finalStreamIntel); if (isAbandoned()) { return; } - mUrlResponseInfo.setReceivedByteCount(finalStreamIntel.getSentByteCount()); if (successReady(SucceededState.ON_COMPLETE_RECEIVED)) { onSucceeded(); } @@ -900,7 +926,7 @@ void readData(int size) { */ void cancel() { EnvoyHTTPStream stream = mStream.get(); - if (isAbandoned() || mEndStream) { + if (this != mCronvoyCallbacks || mEndStream) { return; } @CancelState int oldState = mCancelState.getAndSet(CancelState.CANCELLED); diff --git a/library/kotlin/io/envoyproxy/envoymobile/mocks/MockStream.kt b/library/kotlin/io/envoyproxy/envoymobile/mocks/MockStream.kt index 7f326d22ab..afe1bfbf62 100644 --- a/library/kotlin/io/envoyproxy/envoymobile/mocks/MockStream.kt +++ b/library/kotlin/io/envoyproxy/envoymobile/mocks/MockStream.kt @@ -15,6 +15,7 @@ class MockStream internal constructor(underlyingStream: MockEnvoyHTTPStream) : S override fun getStreamId(): Long { return 0 } override fun getConnectionId(): Long { return 0 } override fun getAttemptCount(): Long { return 0 } + override fun getReceivedByteCount(): Long { return 0 } } private val mockFinalStreamIntel = object : EnvoyFinalStreamIntel { diff --git a/test/common/integration/client_integration_test.cc b/test/common/integration/client_integration_test.cc index 6e104cd2bb..643d8a75ed 100644 --- a/test/common/integration/client_integration_test.cc +++ b/test/common/integration/client_integration_test.cc @@ -36,6 +36,7 @@ typedef struct { uint32_t on_complete_calls; uint32_t on_error_calls; uint32_t on_cancel_calls; + uint32_t received_byte_count; std::string status; ConditionalInitializer* terminal_callback; } callbacks_called; @@ -62,12 +63,13 @@ class ClientIntegrationTest : public BaseIntegrationTest, }); bridge_callbacks_.context = &cc_; - bridge_callbacks_.on_headers = [](envoy_headers c_headers, bool, envoy_stream_intel, + bridge_callbacks_.on_headers = [](envoy_headers c_headers, bool, envoy_stream_intel intel, void* context) -> void* { Http::ResponseHeaderMapPtr response_headers = toResponseHeaders(c_headers); callbacks_called* cc_ = static_cast(context); cc_->on_headers_calls++; cc_->status = response_headers->Status()->value().getStringView(); + cc_->received_byte_count = intel.received_byte_count; return nullptr; }; bridge_callbacks_.on_data = [](envoy_data c_data, bool, envoy_stream_intel, @@ -143,7 +145,7 @@ name: api_listener Http::ClientPtr http_client_{}; envoy_http_callbacks bridge_callbacks_; ConditionalInitializer terminal_callback_; - callbacks_called cc_ = {0, 0, 0, 0, 0, "", &terminal_callback_}; + callbacks_called cc_ = {0, 0, 0, 0, 0, 0, "", &terminal_callback_}; }; INSTANTIATE_TEST_SUITE_P(IpVersions, ClientIntegrationTest, @@ -206,6 +208,7 @@ TEST_P(ClientIntegrationTest, Basic) { ASSERT_EQ(cc_.status, "200"); ASSERT_EQ(cc_.on_data_calls, 2); ASSERT_EQ(cc_.on_complete_calls, 1); + ASSERT_EQ(cc_.received_byte_count, 67); // stream_success gets charged for 2xx status codes. test_server_->waitForCounterEq("http.client.stream_success", 1); diff --git a/test/java/org/chromium/net/CronetUrlRequestTest.java b/test/java/org/chromium/net/CronetUrlRequestTest.java index 6d27c568e4..275a80ebea 100644 --- a/test/java/org/chromium/net/CronetUrlRequestTest.java +++ b/test/java/org/chromium/net/CronetUrlRequestTest.java @@ -219,8 +219,9 @@ public void testRedirectAsync() throws Exception { checkResponseInfoHeader(callback.mRedirectResponseInfoList.get(0), "redirect-header", "header-value"); + // Original bytesReceived: 73 UrlResponseInfo expected = createUrlResponseInfo( - new String[] {NativeTestServer.getRedirectURL()}, "Found", 302, 73, "Content-Length", "92", + new String[] {NativeTestServer.getRedirectURL()}, "Found", 302, -1, "Content-Length", "92", "Location", "/success.txt", "redirect-header", "header-value"); mTestRule.assertResponseEquals(expected, callback.mRedirectResponseInfoList.get(0)); @@ -266,9 +267,10 @@ public void testRedirectAsync() throws Exception { assertEquals(ResponseStep.ON_SUCCEEDED, callback.mResponseStep); assertEquals(NativeTestServer.SUCCESS_BODY, callback.mResponseAsString); + // Original bytesReceived: 258 UrlResponseInfo urlResponseInfo = createUrlResponseInfo( new String[] {NativeTestServer.getRedirectURL(), NativeTestServer.getSuccessURL()}, "OK", - 200, 258, "Content-Length", "20", "Content-Type", "text/plain", + 200, -1, "Content-Length", "20", "Content-Type", "text/plain", "Access-Control-Allow-Origin", "*", "header-name", "header-value", "multi-header-name", "header-value1", "multi-header-name", "header-value2"); @@ -666,17 +668,19 @@ public void testMockMultiRedirect() throws Exception { assertEquals(2, callback.mRedirectResponseInfoList.size()); // Check first redirect (multiredirect.html -> redirect.html) + // Original receivedBytes: 76 UrlResponseInfo firstExpectedResponseInfo = createUrlResponseInfo( - new String[] {NativeTestServer.getMultiRedirectURL()}, "Found", 302, 76, "Content-Length", + new String[] {NativeTestServer.getMultiRedirectURL()}, "Found", 302, -1, "Content-Length", "92", "Location", "/redirect.html", "redirect-header0", "header-value"); UrlResponseInfo firstRedirectResponseInfo = callback.mRedirectResponseInfoList.get(0); mTestRule.assertResponseEquals(firstExpectedResponseInfo, firstRedirectResponseInfo); // Check second redirect (redirect.html -> success.txt) + // Original receivedBytes: 334 UrlResponseInfo secondExpectedResponseInfo = createUrlResponseInfo( new String[] {NativeTestServer.getMultiRedirectURL(), NativeTestServer.getRedirectURL(), NativeTestServer.getSuccessURL()}, - "OK", 200, 334, "Content-Length", "20", "Content-Type", "text/plain", + "OK", 200, -1, "Content-Length", "20", "Content-Type", "text/plain", "Access-Control-Allow-Origin", "*", "header-name", "header-value", "multi-header-name", "header-value1", "multi-header-name", "header-value2"); @@ -693,7 +697,7 @@ public void testMockNotFound() throws Exception { TestUrlRequestCallback callback = startAndWaitForComplete(NativeTestServer.getNotFoundURL()); UrlResponseInfo expected = createUrlResponseInfo(new String[] {NativeTestServer.getNotFoundURL()}, "Not Found", 404, - 140, "Content-Length", "96"); + 142, "Content-Length", "96"); mTestRule.assertResponseEquals(expected, callback.mResponseInfo); assertTrue(callback.mHttpResponseDataLength != 0); assertEquals(0, callback.mRedirectCount); diff --git a/test/java/org/chromium/net/testing/CronetTestRule.java b/test/java/org/chromium/net/testing/CronetTestRule.java index c25e46a45d..ae222858b6 100644 --- a/test/java/org/chromium/net/testing/CronetTestRule.java +++ b/test/java/org/chromium/net/testing/CronetTestRule.java @@ -68,7 +68,7 @@ public static class CronetTestFramework { private static ExperimentalCronetEngine createEngine(Context context) { ExperimentalCronetEngine.Builder builder = new ExperimentalCronetEngine.Builder(context); - ((CronetEngineBuilderImpl)builder.getBuilderDelegate()).setLogLevel("info"); + ((CronetEngineBuilderImpl)builder.getBuilderDelegate()).setLogLevel("off"); return builder.enableQuic(true).build(); } @@ -253,8 +253,10 @@ public void assertResponseEquals(UrlResponseInfo expected, UrlResponseInfo actua assertEquals(expected.getUrl(), actual.getUrl()); // Transferred bytes and proxy server are not supported in pure java if (!testingJavaImpl()) { - // TODO("https://github.com/envoyproxy/envoy-mobile/issues/1426"): uncomment the assert - // assertEquals(expected.getReceivedByteCount(), actual.getReceivedByteCount()); + // TODO("https://github.com/envoyproxy/envoy-mobile/issues/1426"): remove the "if" crutch + if (expected.getReceivedByteCount() >= 0) { + assertEquals(expected.getReceivedByteCount(), actual.getReceivedByteCount()); + } assertEquals(expected.getProxyServer(), actual.getProxyServer()); // This is a place where behavior intentionally differs between native and java assertEquals(expected.getNegotiatedProtocol(), actual.getNegotiatedProtocol()); From 28dd8483a6176cfda1a0bbd5324df2d9d4984fd9 Mon Sep 17 00:00:00 2001 From: Charles Le Borgne Date: Mon, 17 Jan 2022 15:03:24 +0000 Subject: [PATCH 02/12] Kick CI Signed-off-by: Charles Le Borgne --- library/java/org/chromium/net/impl/CronetUrlRequest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/library/java/org/chromium/net/impl/CronetUrlRequest.java b/library/java/org/chromium/net/impl/CronetUrlRequest.java index 36b68f5476..19ad827034 100644 --- a/library/java/org/chromium/net/impl/CronetUrlRequest.java +++ b/library/java/org/chromium/net/impl/CronetUrlRequest.java @@ -962,6 +962,7 @@ private void setUrlResponseInfo(Map> responseHeaders, int r // TODO(https://github.com/envoyproxy/envoy-mobile/issues/1426) set receivedByteCount // TODO(https://github.com/envoyproxy/envoy-mobile/issues/1622) support proxy // TODO(https://github.com/envoyproxy/envoy-mobile/issues/1546) negotiated protocol + // TODO(https://github.com/envoyproxy/envoy-mobile/issues/1578) http caching mUrlResponseInfo.setResponseValues( new ArrayList<>(mUrlChain), responseCode, HttpReason.getReason(responseCode), Collections.unmodifiableList(headerList), false, selectedTransport, ":0"); From 5087b6b6711dc74ab511afd836130fda6f68d29c Mon Sep 17 00:00:00 2001 From: Charles Le Borgne Date: Sat, 22 Jan 2022 15:07:22 +0000 Subject: [PATCH 03/12] Use `getUpstreamBytesMeter()->headerBytesReceived()` to report the bytes received on `encodeHeaders` Signed-off-by: Charles Le Borgne --- library/common/http/client.cc | 12 ++- library/common/http/client.h | 7 +- .../engine/types/EnvoyFinalStreamIntel.java | 2 + .../chromium/net/impl/CronetUrlRequest.java | 80 ++++++++++++------- .../net/impl/CronetUrlRequestContext.java | 4 +- .../envoymobile/FinalStreamIntel.kt | 1 + .../envoymobile/mocks/MockStream.kt | 22 ++--- .../chromium/net/CronetUrlRequestTest.java | 3 +- 8 files changed, 76 insertions(+), 55 deletions(-) diff --git a/library/common/http/client.cc b/library/common/http/client.cc index 0e379497ed..90f25bfa17 100644 --- a/library/common/http/client.cc +++ b/library/common/http/client.cc @@ -41,7 +41,7 @@ void Client::DirectStreamCallbacks::encodeHeaders(const ResponseHeaderMap& heade ASSERT(http_client_.getStream(direct_stream_.stream_handle_, GetStreamFilters::ALLOW_FOR_ALL_STREAMS)); - direct_stream_.saveLatestStreamIntel(); + direct_stream_.saveLatestStreamIntel(streamInfo().getUpstreamBytesMeter()->headerBytesReceived()); if (end_stream) { closeStream(); } @@ -84,7 +84,7 @@ void Client::DirectStreamCallbacks::encodeData(Buffer::Instance& data, bool end_ ASSERT(http_client_.getStream(direct_stream_.stream_handle_, GetStreamFilters::ALLOW_FOR_ALL_STREAMS)); - direct_stream_.saveLatestStreamIntel(); + direct_stream_.saveLatestStreamIntel(streamInfo().getUpstreamBytesMeter()->wireBytesReceived()); if (end_stream) { closeStream(); } @@ -147,7 +147,7 @@ void Client::DirectStreamCallbacks::encodeTrailers(const ResponseTrailerMap& tra ASSERT(http_client_.getStream(direct_stream_.stream_handle_, GetStreamFilters::ALLOW_FOR_ALL_STREAMS)); - direct_stream_.saveLatestStreamIntel(); + direct_stream_.saveLatestStreamIntel(streamInfo().getUpstreamBytesMeter()->wireBytesReceived()); closeStream(); // Trailers always indicate the end of the stream. // For explicit flow control, don't send data unless prompted. @@ -291,16 +291,14 @@ envoy_final_stream_intel& Client::DirectStreamCallbacks::finalStreamIntel() { return direct_stream_.envoy_final_stream_intel_; } -void Client::DirectStream::saveLatestStreamIntel() { +void Client::DirectStream::saveLatestStreamIntel(uint64_t received_byte_count) { const auto& info = request_decoder_->streamInfo(); if (info.upstreamInfo()) { stream_intel_.connection_id = info.upstreamInfo()->upstreamConnectionId().value_or(-1); } stream_intel_.stream_id = static_cast(stream_handle_); stream_intel_.attempt_count = info.attemptCount().value_or(0); - if (info.getUpstreamBytesMeter()) { - stream_intel_.received_byte_count = info.getUpstreamBytesMeter()->wireBytesReceived(); - } + stream_intel_.received_byte_count = received_byte_count; } void Client::DirectStream::saveFinalStreamIntel() { diff --git a/library/common/http/client.h b/library/common/http/client.h index b07ad397df..e4056e9620 100644 --- a/library/common/http/client.h +++ b/library/common/http/client.h @@ -170,6 +170,9 @@ class Client : public Logger::Loggable { private: bool hasBufferedData() { return response_data_.get() && response_data_->length() != 0; } + const StreamInfo::StreamInfo& streamInfo() { + return direct_stream_.request_decoder_->streamInfo(); + } void sendDataToBridge(Buffer::Instance& data, bool end_stream); void sendTrailersToBridge(const ResponseTrailerMap& trailers); @@ -248,7 +251,7 @@ class Client : public Logger::Loggable { } // Latches stream information as it may not be available when accessed. - void saveLatestStreamIntel(); + void saveLatestStreamIntel(uint64_t received_byte_count); // Latches latency info from stream info before it goes away. void saveFinalStreamIntel(); @@ -279,7 +282,7 @@ class Client : public Logger::Loggable { // read faster than the mobile caller can process it. bool explicit_flow_control_ = false; // Latest intel data retrieved from the StreamInfo. - envoy_stream_intel stream_intel_{-1, -1, 0}; + envoy_stream_intel stream_intel_{-1, -1, 0, 0}; envoy_final_stream_intel envoy_final_stream_intel_{-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, 0, 0, 0}; StreamInfo::BytesMeterSharedPtr bytes_meter_; diff --git a/library/java/io/envoyproxy/envoymobile/engine/types/EnvoyFinalStreamIntel.java b/library/java/io/envoyproxy/envoymobile/engine/types/EnvoyFinalStreamIntel.java index 4936176a56..fda62703a2 100644 --- a/library/java/io/envoyproxy/envoymobile/engine/types/EnvoyFinalStreamIntel.java +++ b/library/java/io/envoyproxy/envoymobile/engine/types/EnvoyFinalStreamIntel.java @@ -2,6 +2,8 @@ /** * Exposes internal HTTP stream metrics, context, and other details sent once on stream end. + * + * Note: a value of -1 means "not present" for any field where the name is suffixed with "Ms". */ public interface EnvoyFinalStreamIntel { /* diff --git a/library/java/org/chromium/net/impl/CronetUrlRequest.java b/library/java/org/chromium/net/impl/CronetUrlRequest.java index 19ad827034..6d782a56fb 100644 --- a/library/java/org/chromium/net/impl/CronetUrlRequest.java +++ b/library/java/org/chromium/net/impl/CronetUrlRequest.java @@ -16,6 +16,7 @@ import java.util.AbstractMap; import java.util.AbstractMap.SimpleEntry; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.LinkedHashMap; import java.util.List; @@ -29,6 +30,7 @@ import org.chromium.net.CallbackException; import org.chromium.net.CronetException; import org.chromium.net.InlineExecutionProhibitedException; +import org.chromium.net.RequestFinishedInfo; import org.chromium.net.UploadDataProvider; /** UrlRequest, backed by Envoy-Mobile. */ @@ -88,6 +90,7 @@ public final class CronetUrlRequest extends UrlRequestBase { private final String mUserAgent; private final HeadersList mRequestHeaders = new HeadersList(); + private final Collection mRequestAnnotations; private final CronetUrlRequestContext mRequestContext; private final AtomicBoolean mWaitingOnRedirect = new AtomicBoolean(false); private final AtomicBoolean mWaitingOnRead = new AtomicBoolean(false); @@ -110,11 +113,16 @@ public final class CronetUrlRequest extends UrlRequestBase { /* These don't change with redirects */ private String mInitialMethod; - private CronetUploadDataStream mUploadDataStream; private final Executor mUserExecutor; private final VersionSafeCallbacks.UrlRequestCallback mCallback; + private final String mInitialUrl; + private final VersionSafeCallbacks.RequestFinishedInfoListener mRequestFinishedListener; private final ConditionVariable mStartBlock = new ConditionVariable(); + private CronetUploadDataStream mUploadDataStream; + + private volatile CronetException mException; + /** * Holds a subset of StatusValues - {@link State#STARTED} can represent {@link * Status#SENDING_REQUEST} or {@link Status#WAITING_FOR_RESPONSE}. While the distinction isn't @@ -126,15 +134,16 @@ public final class CronetUrlRequest extends UrlRequestBase { * only used with the STARTED state, so it's inconsequential. */ @StatusValues private volatile int mAdditionalStatusDetails = Status.INVALID; - private final AtomicReference mError = new AtomicReference<>(); /* These change with redirects. */ private final AtomicReference mStream = new AtomicReference<>(); private final List mUrlChain = new ArrayList<>(); - private final List mEnvoyFinalStreamIntels = new ArrayList<>(); + private EnvoyFinalStreamIntel mEnvoyFinalStreamIntel; + private long mBytesReceivedFromRedirects = 0; + private long mBytesReceivedFromLastRedirect = 0; private CronvoyHttpCallbacks mCronvoyCallbacks; private String mCurrentUrl; - private UrlResponseInfoImpl mUrlResponseInfo; + private volatile UrlResponseInfoImpl mUrlResponseInfo; private String mPendingRedirectUrl; /** @@ -143,8 +152,9 @@ public final class CronetUrlRequest extends UrlRequestBase { */ CronetUrlRequest(CronetUrlRequestContext cronvoyEngine, Callback callback, Executor executor, String url, String userAgent, boolean allowDirectExecutor, - boolean trafficStatsTagSet, int trafficStatsTag, boolean trafficStatsUidSet, - int trafficStatsUid) { + Collection connectionAnnotations, boolean trafficStatsTagSet, + int trafficStatsTag, boolean trafficStatsUidSet, int trafficStatsUid, + RequestFinishedInfo.Listener requestFinishedListener) { if (url == null) { throw new NullPointerException("URL is required"); } @@ -155,11 +165,17 @@ public final class CronetUrlRequest extends UrlRequestBase { throw new NullPointerException("Executor is required"); } mCallback = new VersionSafeCallbacks.UrlRequestCallback(callback); + mRequestFinishedListener = + requestFinishedListener != null + ? new VersionSafeCallbacks.RequestFinishedInfoListener(requestFinishedListener) + : null; mRequestContext = cronvoyEngine; mAllowDirectExecutor = allowDirectExecutor; mUserExecutor = executor; + mInitialUrl = url; mCurrentUrl = url; mUserAgent = userAgent; + mRequestAnnotations = connectionAnnotations; } @Override @@ -306,8 +322,8 @@ public boolean isDone() { @Override public void getStatus(StatusListener listener) { + @StatusValues int extraStatus = mAdditionalStatusDetails; @State int state = mState.get(); - int extraStatus = mAdditionalStatusDetails; @StatusValues final int status; switch (state) { @@ -402,7 +418,6 @@ private static int determineNextErrorState(boolean streamEnded, @State int origi } private void enterErrorState(CronetException error) { - mError.compareAndSet(null, error); @State int originalState; @State int updatedState; do { @@ -415,6 +430,7 @@ private void enterErrorState(CronetException error) { if (isTerminalState(originalState)) { return; } + mException = error; fireCloseUploadDataProvider(); if (updatedState == State.ERROR_PENDING_CANCEL) { CronvoyHttpCallbacks cronvoyCallbacks = this.mCronvoyCallbacks; @@ -470,12 +486,17 @@ private void fireCloseUploadDataProvider() { } } + // This method is only called when in STARTED state. This means a "cancel" request won't be + // executed immediately - that quite important here, otherwise this would lead to unfortunate + // race conditions. A "cancel" request will then be honnored on the first callback. private void fireOpenConnection() { if (mInitialMethod == null) { mInitialMethod = "GET"; } + mUrlResponseInfo = null; + mEnvoyFinalStreamIntel = null; + mBytesReceivedFromRedirects += mBytesReceivedFromLastRedirect; mAdditionalStatusDetails = Status.CONNECTING; - mUrlResponseInfo = new UrlResponseInfoImpl(); mUrlChain.add(mCurrentUrl); Map> envoyRequestHeaders = buildEnvoyRequestHeaders(mInitialMethod, mRequestHeaders, mUploadDataStream, mUserAgent, @@ -569,7 +590,7 @@ public void run() { try { mCallback.onCanceled(CronetUrlRequest.this, mUrlResponseInfo); } catch (Exception exception) { - Log.e(TAG, "Exception in onCanceled method", exception); + Log.e(CronetUrlRequestContext.LOG_TAG, "Exception in onCanceled method", exception); } } }; @@ -583,7 +604,7 @@ public void run() { try { mCallback.onSucceeded(CronetUrlRequest.this, mUrlResponseInfo); } catch (Exception exception) { - Log.e(TAG, "Exception in onSucceeded method", exception); + Log.e(CronetUrlRequestContext.LOG_TAG, "Exception in onSucceeded method", exception); } } }; @@ -595,9 +616,9 @@ void onFailed() { @Override public void run() { try { - mCallback.onFailed(CronetUrlRequest.this, mUrlResponseInfo, mError.get()); + mCallback.onFailed(CronetUrlRequest.this, mUrlResponseInfo, mException); } catch (Exception exception) { - Log.e(TAG, "Exception in onFailed method", exception); + Log.e(CronetUrlRequestContext.LOG_TAG, "Exception in onFailed method", exception); } } }; @@ -633,22 +654,16 @@ private boolean streamEnded() { } private void recordEnvoyFinalStreamIntel(EnvoyFinalStreamIntel envoyFinalStreamIntel) { - mEnvoyFinalStreamIntels.add(envoyFinalStreamIntel); - long bytesReceived = 0; - // This in only called by the network Thread - no concurrency issue. - for (EnvoyFinalStreamIntel intel : mEnvoyFinalStreamIntels) { - bytesReceived += intel.getReceivedByteCount(); + mEnvoyFinalStreamIntel = envoyFinalStreamIntel; + if (mUrlResponseInfo != null) { // Null if cancelled before receiving a Response. + mUrlResponseInfo.setReceivedByteCount(envoyFinalStreamIntel.getReceivedByteCount() + + mBytesReceivedFromRedirects); } - mUrlResponseInfo.setReceivedByteCount(bytesReceived); } private void recordEnvoyStreamIntel(EnvoyStreamIntel envoyStreamIntel) { - long bytesReceived = envoyStreamIntel.getReceivedByteCount(); - // This in only called by the network Thread - no concurrency issue. - for (EnvoyFinalStreamIntel intel : mEnvoyFinalStreamIntels) { - bytesReceived += intel.getReceivedByteCount(); - } - mUrlResponseInfo.setReceivedByteCount(bytesReceived); + mUrlResponseInfo.setReceivedByteCount(envoyStreamIntel.getReceivedByteCount() + + mBytesReceivedFromRedirects); } private static class HeadersList extends ArrayList> {} @@ -686,6 +701,8 @@ public Executor getExecutor() { @Override public void onHeaders(Map> headers, boolean endStream, EnvoyStreamIntel streamIntel) { + System.err.println("RRRRR: " + streamIntel.getReceivedByteCount()); + mUrlResponseInfo = new UrlResponseInfoImpl(); recordEnvoyStreamIntel(streamIntel); if (isAbandoned()) { return; @@ -719,6 +736,7 @@ public void onHeaders(Map> headers, boolean endStream, } if (locationField != null) { + mBytesReceivedFromLastRedirect = streamIntel.getReceivedByteCount(); cancel(); // Abort the the original request - we are being redirected. } @@ -752,10 +770,11 @@ public void run() { @Override public void onData(ByteBuffer data, boolean endStream, EnvoyStreamIntel streamIntel) { - recordEnvoyStreamIntel(streamIntel); if (isAbandoned()) { return; } + recordEnvoyStreamIntel(streamIntel); + System.err.println("TTTTT: " + streamIntel.getReceivedByteCount()); mEndStream = endStream; @State int originalState; @State int updatedState; @@ -790,7 +809,6 @@ public void run() { @Override public void onTrailers(Map> trailers, EnvoyStreamIntel streamIntel) { - recordEnvoyStreamIntel(streamIntel); if (isAbandoned()) { return; } @@ -809,10 +827,10 @@ public void onTrailers(Map> trailers, EnvoyStreamIntel stre @Override public void onError(int errorCode, String message, int attemptCount, EnvoyStreamIntel streamIntel, EnvoyFinalStreamIntel finalStreamIntel) { - recordEnvoyFinalStreamIntel(finalStreamIntel); if (isAbandoned()) { return; } + recordEnvoyFinalStreamIntel(finalStreamIntel); mEndStream = true; @State int originalState; @State int updatedState; @@ -832,10 +850,10 @@ public void onError(int errorCode, String message, int attemptCount, @Override public void onCancel(EnvoyStreamIntel streamIntel, EnvoyFinalStreamIntel finalStreamIntel) { - recordEnvoyFinalStreamIntel(finalStreamIntel); if (isAbandoned()) { return; } + recordEnvoyFinalStreamIntel(finalStreamIntel); mEndStream = true; @State int originalState; @State int updatedState; @@ -853,7 +871,6 @@ public void onCancel(EnvoyStreamIntel streamIntel, EnvoyFinalStreamIntel finalSt @Override public void onSendWindowAvailable(EnvoyStreamIntel streamIntel) { - recordEnvoyStreamIntel(streamIntel); if (isAbandoned()) { return; } @@ -875,10 +892,10 @@ public void onSendWindowAvailable(EnvoyStreamIntel streamIntel) { @Override public void onComplete(EnvoyStreamIntel streamIntel, EnvoyFinalStreamIntel finalStreamIntel) { - recordEnvoyFinalStreamIntel(finalStreamIntel); if (isAbandoned()) { return; } + recordEnvoyFinalStreamIntel(finalStreamIntel); if (successReady(SucceededState.ON_COMPLETE_RECEIVED)) { onSucceeded(); } @@ -931,6 +948,7 @@ void cancel() { } @CancelState int oldState = mCancelState.getAndSet(CancelState.CANCELLED); if (oldState == CancelState.READY) { + System.err.println("CANCELLING"); stream.cancel(); } } diff --git a/library/java/org/chromium/net/impl/CronetUrlRequestContext.java b/library/java/org/chromium/net/impl/CronetUrlRequestContext.java index d12b41a955..00e9f3a226 100644 --- a/library/java/org/chromium/net/impl/CronetUrlRequestContext.java +++ b/library/java/org/chromium/net/impl/CronetUrlRequestContext.java @@ -126,8 +126,8 @@ void setTaskToExecuteWhenInitializationIsCompleted(Runnable runnable) { int trafficStatsUid, RequestFinishedInfo.Listener requestFinishedListener, int idempotency) { return new CronetUrlRequest(this, callback, executor, url, mUserAgent, allowDirectExecutor, - trafficStatsTagSet, trafficStatsTag, trafficStatsUidSet, - trafficStatsUid); + connectionAnnotations, trafficStatsTagSet, trafficStatsTag, + trafficStatsUidSet, trafficStatsUid, requestFinishedListener); } @Override diff --git a/library/kotlin/io/envoyproxy/envoymobile/FinalStreamIntel.kt b/library/kotlin/io/envoyproxy/envoymobile/FinalStreamIntel.kt index 5375146578..561758060b 100644 --- a/library/kotlin/io/envoyproxy/envoymobile/FinalStreamIntel.kt +++ b/library/kotlin/io/envoyproxy/envoymobile/FinalStreamIntel.kt @@ -4,6 +4,7 @@ import io.envoyproxy.envoymobile.engine.types.EnvoyFinalStreamIntel /** * Exposes one time HTTP stream metrics, context, and other details. + * Note: a value of -1 means "not present" for any field where the name is suffixed with "Ms". * @param requestStartMs The time the request started, in ms since the epoch. * @param dnsStartMs The time the DNS resolution for this request started, in ms since the epoch. * @param dnsEndMs The time the DNS resolution for this request completed, in ms since the epoch. diff --git a/library/kotlin/io/envoyproxy/envoymobile/mocks/MockStream.kt b/library/kotlin/io/envoyproxy/envoymobile/mocks/MockStream.kt index afe1bfbf62..54bc7ed31c 100644 --- a/library/kotlin/io/envoyproxy/envoymobile/mocks/MockStream.kt +++ b/library/kotlin/io/envoyproxy/envoymobile/mocks/MockStream.kt @@ -19,17 +19,17 @@ class MockStream internal constructor(underlyingStream: MockEnvoyHTTPStream) : S } private val mockFinalStreamIntel = object : EnvoyFinalStreamIntel { - override fun getRequestStartMs(): Long { return 0 } - override fun getDnsStartMs(): Long { return 0 } - override fun getDnsEndMs(): Long { return 0 } - override fun getConnectStartMs(): Long { return 0 } - override fun getConnectEndMs(): Long { return 0 } - override fun getSslStartMs(): Long { return 0 } - override fun getSslEndMs(): Long { return 0 } - override fun getSendingStartMs(): Long { return 0 } - override fun getSendingEndMs(): Long { return 0 } - override fun getResponseStartMs(): Long { return 0 } - override fun getRequestEndMs(): Long { return 0 } + override fun getRequestStartMs(): Long { return -1 } + override fun getDnsStartMs(): Long { return -1 } + override fun getDnsEndMs(): Long { return -1 } + override fun getConnectStartMs(): Long { return -1 } + override fun getConnectEndMs(): Long { return -1 } + override fun getSslStartMs(): Long { return -1 } + override fun getSslEndMs(): Long { return -1 } + override fun getSendingStartMs(): Long { return -1 } + override fun getSendingEndMs(): Long { return -1 } + override fun getResponseStartMs(): Long { return -1 } + override fun getRequestEndMs(): Long { return -1 } override fun getSocketReused(): Boolean { return false } override fun getSentByteCount(): Long { return 0 } override fun getReceivedByteCount(): Long { return 0 } diff --git a/test/java/org/chromium/net/CronetUrlRequestTest.java b/test/java/org/chromium/net/CronetUrlRequestTest.java index 275a80ebea..cbac5a3ee4 100644 --- a/test/java/org/chromium/net/CronetUrlRequestTest.java +++ b/test/java/org/chromium/net/CronetUrlRequestTest.java @@ -219,9 +219,8 @@ public void testRedirectAsync() throws Exception { checkResponseInfoHeader(callback.mRedirectResponseInfoList.get(0), "redirect-header", "header-value"); - // Original bytesReceived: 73 UrlResponseInfo expected = createUrlResponseInfo( - new String[] {NativeTestServer.getRedirectURL()}, "Found", 302, -1, "Content-Length", "92", + new String[] {NativeTestServer.getRedirectURL()}, "Found", 302, 72, "Content-Length", "92", "Location", "/success.txt", "redirect-header", "header-value"); mTestRule.assertResponseEquals(expected, callback.mRedirectResponseInfoList.get(0)); From 277eb61a6b24ec1b4e42aaf408a349bb38ef68b7 Mon Sep 17 00:00:00 2001 From: Charles Le Borgne Date: Sat, 22 Jan 2022 19:18:33 +0000 Subject: [PATCH 04/12] Update test Signed-off-by: Charles Le Borgne --- library/common/http/client.cc | 6 +++++- .../integration/client_integration_test.cc | 16 ++++++++++------ 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/library/common/http/client.cc b/library/common/http/client.cc index 90f25bfa17..18cd504ad3 100644 --- a/library/common/http/client.cc +++ b/library/common/http/client.cc @@ -305,7 +305,11 @@ void Client::DirectStream::saveFinalStreamIntel() { if (!request_decoder_ || !parent_.getStream(stream_handle_, ALLOW_ONLY_FOR_OPEN_STREAMS)) { return; } - StreamInfo::setFinalStreamIntel(request_decoder_->streamInfo(), envoy_final_stream_intel_); + StreamInfo::StreamInfo& stream_info = request_decoder_->streamInfo(); + if (stream_info.getUpstreamBytesMeter()) { + stream_intel_.received_byte_count = stream_info.getUpstreamBytesMeter()->wireBytesReceived(); + } + StreamInfo::setFinalStreamIntel(stream_info, envoy_final_stream_intel_); } envoy_error Client::DirectStreamCallbacks::streamError() { diff --git a/test/common/integration/client_integration_test.cc b/test/common/integration/client_integration_test.cc index 643d8a75ed..6c784e8490 100644 --- a/test/common/integration/client_integration_test.cc +++ b/test/common/integration/client_integration_test.cc @@ -36,7 +36,8 @@ typedef struct { uint32_t on_complete_calls; uint32_t on_error_calls; uint32_t on_cancel_calls; - uint32_t received_byte_count; + uint32_t on_header_received_byte_count; + uint32_t on_complete_received_byte_count; std::string status; ConditionalInitializer* terminal_callback; } callbacks_called; @@ -69,7 +70,7 @@ class ClientIntegrationTest : public BaseIntegrationTest, callbacks_called* cc_ = static_cast(context); cc_->on_headers_calls++; cc_->status = response_headers->Status()->value().getStringView(); - cc_->received_byte_count = intel.received_byte_count; + cc_->on_header_received_byte_count = intel.received_byte_count; return nullptr; }; bridge_callbacks_.on_data = [](envoy_data c_data, bool, envoy_stream_intel, @@ -79,11 +80,13 @@ class ClientIntegrationTest : public BaseIntegrationTest, release_envoy_data(c_data); return nullptr; }; - bridge_callbacks_.on_complete = [](envoy_stream_intel, envoy_final_stream_intel, - void* context) -> void* { + bridge_callbacks_.on_complete = + [](envoy_stream_intel intel, envoy_final_stream_intel final_intel, void* context) -> void* { callbacks_called* cc_ = static_cast(context); cc_->on_complete_calls++; cc_->terminal_callback->setReady(); + EXPECT_EQ(intel.received_byte_count, final_intel.received_byte_count); + cc_->on_complete_received_byte_count = final_intel.received_byte_count; return nullptr; }; bridge_callbacks_.on_error = [](envoy_error error, envoy_stream_intel, envoy_final_stream_intel, @@ -145,7 +148,7 @@ name: api_listener Http::ClientPtr http_client_{}; envoy_http_callbacks bridge_callbacks_; ConditionalInitializer terminal_callback_; - callbacks_called cc_ = {0, 0, 0, 0, 0, 0, "", &terminal_callback_}; + callbacks_called cc_ = {0, 0, 0, 0, 0, 0, 0, "", &terminal_callback_}; }; INSTANTIATE_TEST_SUITE_P(IpVersions, ClientIntegrationTest, @@ -208,7 +211,8 @@ TEST_P(ClientIntegrationTest, Basic) { ASSERT_EQ(cc_.status, "200"); ASSERT_EQ(cc_.on_data_calls, 2); ASSERT_EQ(cc_.on_complete_calls, 1); - ASSERT_EQ(cc_.received_byte_count, 67); + ASSERT_EQ(cc_.on_header_received_byte_count, 27); + ASSERT_EQ(cc_.on_complete_received_byte_count, 67); // stream_success gets charged for 2xx status codes. test_server_->waitForCounterEq("http.client.stream_success", 1); From 9fadc39bf4dc149323f6a2b5d10627829a233d02 Mon Sep 17 00:00:00 2001 From: Charles Le Borgne Date: Sat, 22 Jan 2022 20:03:29 +0000 Subject: [PATCH 05/12] Don't assume that upstream stats are available. Signed-off-by: Charles Le Borgne --- library/common/http/client.cc | 13 +++++-------- library/common/http/client.h | 10 ++++++++++ 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/library/common/http/client.cc b/library/common/http/client.cc index 18cd504ad3..a2bf2e8c54 100644 --- a/library/common/http/client.cc +++ b/library/common/http/client.cc @@ -41,7 +41,7 @@ void Client::DirectStreamCallbacks::encodeHeaders(const ResponseHeaderMap& heade ASSERT(http_client_.getStream(direct_stream_.stream_handle_, GetStreamFilters::ALLOW_FOR_ALL_STREAMS)); - direct_stream_.saveLatestStreamIntel(streamInfo().getUpstreamBytesMeter()->headerBytesReceived()); + direct_stream_.saveLatestStreamIntel(headerBytesReceived()); if (end_stream) { closeStream(); } @@ -84,7 +84,7 @@ void Client::DirectStreamCallbacks::encodeData(Buffer::Instance& data, bool end_ ASSERT(http_client_.getStream(direct_stream_.stream_handle_, GetStreamFilters::ALLOW_FOR_ALL_STREAMS)); - direct_stream_.saveLatestStreamIntel(streamInfo().getUpstreamBytesMeter()->wireBytesReceived()); + direct_stream_.saveLatestStreamIntel(bytesReceived()); if (end_stream) { closeStream(); } @@ -147,7 +147,7 @@ void Client::DirectStreamCallbacks::encodeTrailers(const ResponseTrailerMap& tra ASSERT(http_client_.getStream(direct_stream_.stream_handle_, GetStreamFilters::ALLOW_FOR_ALL_STREAMS)); - direct_stream_.saveLatestStreamIntel(streamInfo().getUpstreamBytesMeter()->wireBytesReceived()); + direct_stream_.saveLatestStreamIntel(bytesReceived()); closeStream(); // Trailers always indicate the end of the stream. // For explicit flow control, don't send data unless prompted. @@ -305,11 +305,8 @@ void Client::DirectStream::saveFinalStreamIntel() { if (!request_decoder_ || !parent_.getStream(stream_handle_, ALLOW_ONLY_FOR_OPEN_STREAMS)) { return; } - StreamInfo::StreamInfo& stream_info = request_decoder_->streamInfo(); - if (stream_info.getUpstreamBytesMeter()) { - stream_intel_.received_byte_count = stream_info.getUpstreamBytesMeter()->wireBytesReceived(); - } - StreamInfo::setFinalStreamIntel(stream_info, envoy_final_stream_intel_); + StreamInfo::setFinalStreamIntel(request_decoder_->streamInfo(), envoy_final_stream_intel_); + stream_intel_.received_byte_count = envoy_final_stream_intel_.received_byte_count; } envoy_error Client::DirectStreamCallbacks::streamError() { diff --git a/library/common/http/client.h b/library/common/http/client.h index e4056e9620..8b3f3c5ce1 100644 --- a/library/common/http/client.h +++ b/library/common/http/client.h @@ -173,6 +173,16 @@ class Client : public Logger::Loggable { const StreamInfo::StreamInfo& streamInfo() { return direct_stream_.request_decoder_->streamInfo(); } + uint64_t headerBytesReceived() { + return streamInfo().getUpstreamBytesMeter() + ? streamInfo().getUpstreamBytesMeter()->headerBytesReceived() + : 0; + } + uint64_t bytesReceived() { + return streamInfo().getUpstreamBytesMeter() + ? streamInfo().getUpstreamBytesMeter()->wireBytesReceived() + : 0; + } void sendDataToBridge(Buffer::Instance& data, bool end_stream); void sendTrailersToBridge(const ResponseTrailerMap& trailers); From f1aef3cb90e7fe98224af84a4d7db3815a9e6a3f Mon Sep 17 00:00:00 2001 From: Charles Le Borgne Date: Mon, 24 Jan 2022 17:54:11 +0000 Subject: [PATCH 06/12] Finalize the PR Signed-off-by: Charles Le Borgne --- library/common/http/client.cc | 1 + library/java/org/chromium/net/impl/CronetUrlRequest.java | 6 ------ test/common/integration/client_integration_test.cc | 6 +++--- test/java/org/chromium/net/testing/CronetTestRule.java | 2 +- 4 files changed, 5 insertions(+), 10 deletions(-) diff --git a/library/common/http/client.cc b/library/common/http/client.cc index a2bf2e8c54..838f71d4d8 100644 --- a/library/common/http/client.cc +++ b/library/common/http/client.cc @@ -306,6 +306,7 @@ void Client::DirectStream::saveFinalStreamIntel() { return; } StreamInfo::setFinalStreamIntel(request_decoder_->streamInfo(), envoy_final_stream_intel_); + // stream_intel_ may have an outdated received_byte_count - the final one is correct. stream_intel_.received_byte_count = envoy_final_stream_intel_.received_byte_count; } diff --git a/library/java/org/chromium/net/impl/CronetUrlRequest.java b/library/java/org/chromium/net/impl/CronetUrlRequest.java index 6d782a56fb..21822397b8 100644 --- a/library/java/org/chromium/net/impl/CronetUrlRequest.java +++ b/library/java/org/chromium/net/impl/CronetUrlRequest.java @@ -701,12 +701,8 @@ public Executor getExecutor() { @Override public void onHeaders(Map> headers, boolean endStream, EnvoyStreamIntel streamIntel) { - System.err.println("RRRRR: " + streamIntel.getReceivedByteCount()); mUrlResponseInfo = new UrlResponseInfoImpl(); recordEnvoyStreamIntel(streamIntel); - if (isAbandoned()) { - return; - } mEndStream = endStream; List statuses = headers.get(":status"); final int responseCode = @@ -774,7 +770,6 @@ public void onData(ByteBuffer data, boolean endStream, EnvoyStreamIntel streamIn return; } recordEnvoyStreamIntel(streamIntel); - System.err.println("TTTTT: " + streamIntel.getReceivedByteCount()); mEndStream = endStream; @State int originalState; @State int updatedState; @@ -948,7 +943,6 @@ void cancel() { } @CancelState int oldState = mCancelState.getAndSet(CancelState.CANCELLED); if (oldState == CancelState.READY) { - System.err.println("CANCELLING"); stream.cancel(); } } diff --git a/test/common/integration/client_integration_test.cc b/test/common/integration/client_integration_test.cc index 6c784e8490..9ab90f7975 100644 --- a/test/common/integration/client_integration_test.cc +++ b/test/common/integration/client_integration_test.cc @@ -36,8 +36,8 @@ typedef struct { uint32_t on_complete_calls; uint32_t on_error_calls; uint32_t on_cancel_calls; - uint32_t on_header_received_byte_count; - uint32_t on_complete_received_byte_count; + uint64_t on_header_received_byte_count; + uint64_t on_complete_received_byte_count; std::string status; ConditionalInitializer* terminal_callback; } callbacks_called; @@ -83,10 +83,10 @@ class ClientIntegrationTest : public BaseIntegrationTest, bridge_callbacks_.on_complete = [](envoy_stream_intel intel, envoy_final_stream_intel final_intel, void* context) -> void* { callbacks_called* cc_ = static_cast(context); + cc_->on_complete_received_byte_count = final_intel.received_byte_count; cc_->on_complete_calls++; cc_->terminal_callback->setReady(); EXPECT_EQ(intel.received_byte_count, final_intel.received_byte_count); - cc_->on_complete_received_byte_count = final_intel.received_byte_count; return nullptr; }; bridge_callbacks_.on_error = [](envoy_error error, envoy_stream_intel, envoy_final_stream_intel, diff --git a/test/java/org/chromium/net/testing/CronetTestRule.java b/test/java/org/chromium/net/testing/CronetTestRule.java index ae222858b6..31c5349bb0 100644 --- a/test/java/org/chromium/net/testing/CronetTestRule.java +++ b/test/java/org/chromium/net/testing/CronetTestRule.java @@ -68,7 +68,7 @@ public static class CronetTestFramework { private static ExperimentalCronetEngine createEngine(Context context) { ExperimentalCronetEngine.Builder builder = new ExperimentalCronetEngine.Builder(context); - ((CronetEngineBuilderImpl)builder.getBuilderDelegate()).setLogLevel("off"); + ((CronetEngineBuilderImpl)builder.getBuilderDelegate()).setLogLevel("warning"); return builder.enableQuic(true).build(); } From 181b155b6a4abf765b2617c98e67817ad784fe32 Mon Sep 17 00:00:00 2001 From: Charles Le Borgne Date: Mon, 24 Jan 2022 18:12:51 +0000 Subject: [PATCH 07/12] Remove unrelated files Signed-off-by: Charles Le Borgne --- .../envoymobile/FinalStreamIntel.kt | 1 - .../envoymobile/mocks/MockStream.kt | 23 +++++++++---------- .../chromium/net/CronetUrlRequestTest.java | 16 ++++++------- .../chromium/net/testing/CronetTestRule.java | 5 +--- 4 files changed, 20 insertions(+), 25 deletions(-) diff --git a/library/kotlin/io/envoyproxy/envoymobile/FinalStreamIntel.kt b/library/kotlin/io/envoyproxy/envoymobile/FinalStreamIntel.kt index 561758060b..5375146578 100644 --- a/library/kotlin/io/envoyproxy/envoymobile/FinalStreamIntel.kt +++ b/library/kotlin/io/envoyproxy/envoymobile/FinalStreamIntel.kt @@ -4,7 +4,6 @@ import io.envoyproxy.envoymobile.engine.types.EnvoyFinalStreamIntel /** * Exposes one time HTTP stream metrics, context, and other details. - * Note: a value of -1 means "not present" for any field where the name is suffixed with "Ms". * @param requestStartMs The time the request started, in ms since the epoch. * @param dnsStartMs The time the DNS resolution for this request started, in ms since the epoch. * @param dnsEndMs The time the DNS resolution for this request completed, in ms since the epoch. diff --git a/library/kotlin/io/envoyproxy/envoymobile/mocks/MockStream.kt b/library/kotlin/io/envoyproxy/envoymobile/mocks/MockStream.kt index 54bc7ed31c..7f326d22ab 100644 --- a/library/kotlin/io/envoyproxy/envoymobile/mocks/MockStream.kt +++ b/library/kotlin/io/envoyproxy/envoymobile/mocks/MockStream.kt @@ -15,21 +15,20 @@ class MockStream internal constructor(underlyingStream: MockEnvoyHTTPStream) : S override fun getStreamId(): Long { return 0 } override fun getConnectionId(): Long { return 0 } override fun getAttemptCount(): Long { return 0 } - override fun getReceivedByteCount(): Long { return 0 } } private val mockFinalStreamIntel = object : EnvoyFinalStreamIntel { - override fun getRequestStartMs(): Long { return -1 } - override fun getDnsStartMs(): Long { return -1 } - override fun getDnsEndMs(): Long { return -1 } - override fun getConnectStartMs(): Long { return -1 } - override fun getConnectEndMs(): Long { return -1 } - override fun getSslStartMs(): Long { return -1 } - override fun getSslEndMs(): Long { return -1 } - override fun getSendingStartMs(): Long { return -1 } - override fun getSendingEndMs(): Long { return -1 } - override fun getResponseStartMs(): Long { return -1 } - override fun getRequestEndMs(): Long { return -1 } + override fun getRequestStartMs(): Long { return 0 } + override fun getDnsStartMs(): Long { return 0 } + override fun getDnsEndMs(): Long { return 0 } + override fun getConnectStartMs(): Long { return 0 } + override fun getConnectEndMs(): Long { return 0 } + override fun getSslStartMs(): Long { return 0 } + override fun getSslEndMs(): Long { return 0 } + override fun getSendingStartMs(): Long { return 0 } + override fun getSendingEndMs(): Long { return 0 } + override fun getResponseStartMs(): Long { return 0 } + override fun getRequestEndMs(): Long { return 0 } override fun getSocketReused(): Boolean { return false } override fun getSentByteCount(): Long { return 0 } override fun getReceivedByteCount(): Long { return 0 } diff --git a/test/java/org/chromium/net/CronetUrlRequestTest.java b/test/java/org/chromium/net/CronetUrlRequestTest.java index cbac5a3ee4..4c6f9b7807 100644 --- a/test/java/org/chromium/net/CronetUrlRequestTest.java +++ b/test/java/org/chromium/net/CronetUrlRequestTest.java @@ -269,7 +269,7 @@ public void testRedirectAsync() throws Exception { // Original bytesReceived: 258 UrlResponseInfo urlResponseInfo = createUrlResponseInfo( new String[] {NativeTestServer.getRedirectURL(), NativeTestServer.getSuccessURL()}, "OK", - 200, -1, "Content-Length", "20", "Content-Type", "text/plain", + 200, 284, "Content-Length", "20", "Content-Type", "text/plain", "Access-Control-Allow-Origin", "*", "header-name", "header-value", "multi-header-name", "header-value1", "multi-header-name", "header-value2"); @@ -669,7 +669,7 @@ public void testMockMultiRedirect() throws Exception { // Check first redirect (multiredirect.html -> redirect.html) // Original receivedBytes: 76 UrlResponseInfo firstExpectedResponseInfo = createUrlResponseInfo( - new String[] {NativeTestServer.getMultiRedirectURL()}, "Found", 302, -1, "Content-Length", + new String[] {NativeTestServer.getMultiRedirectURL()}, "Found", 302, 75, "Content-Length", "92", "Location", "/redirect.html", "redirect-header0", "header-value"); UrlResponseInfo firstRedirectResponseInfo = callback.mRedirectResponseInfoList.get(0); mTestRule.assertResponseEquals(firstExpectedResponseInfo, firstRedirectResponseInfo); @@ -679,7 +679,7 @@ public void testMockMultiRedirect() throws Exception { UrlResponseInfo secondExpectedResponseInfo = createUrlResponseInfo( new String[] {NativeTestServer.getMultiRedirectURL(), NativeTestServer.getRedirectURL(), NativeTestServer.getSuccessURL()}, - "OK", 200, -1, "Content-Length", "20", "Content-Type", "text/plain", + "OK", 200, 359, "Content-Length", "20", "Content-Type", "text/plain", "Access-Control-Allow-Origin", "*", "header-name", "header-value", "multi-header-name", "header-value1", "multi-header-name", "header-value2"); @@ -2140,26 +2140,26 @@ public void testLegacyOnFailedCallback() throws Exception { final ConditionVariable done = new ConditionVariable(); UrlRequest.Callback callback = new UrlRequest.Callback() { @Override - public void onRedirectReceived(UrlRequest request, UrlResponseInfo info, - String newLocationUrl) { + public void onSucceeded(UrlRequest request, UrlResponseInfo info) { failedExpectation.set(true); fail(); } @Override - public void onResponseStarted(UrlRequest request, UrlResponseInfo info) { + public void onRedirectReceived(UrlRequest request, UrlResponseInfo info, + String newLocationUrl) { failedExpectation.set(true); fail(); } @Override - public void onReadCompleted(UrlRequest request, UrlResponseInfo info, ByteBuffer byteBuffer) { + public void onResponseStarted(UrlRequest request, UrlResponseInfo info) { failedExpectation.set(true); fail(); } @Override - public void onSucceeded(UrlRequest request, UrlResponseInfo info) { + public void onReadCompleted(UrlRequest request, UrlResponseInfo info, ByteBuffer byteBuffer) { failedExpectation.set(true); fail(); } diff --git a/test/java/org/chromium/net/testing/CronetTestRule.java b/test/java/org/chromium/net/testing/CronetTestRule.java index 31c5349bb0..b15eece987 100644 --- a/test/java/org/chromium/net/testing/CronetTestRule.java +++ b/test/java/org/chromium/net/testing/CronetTestRule.java @@ -253,10 +253,7 @@ public void assertResponseEquals(UrlResponseInfo expected, UrlResponseInfo actua assertEquals(expected.getUrl(), actual.getUrl()); // Transferred bytes and proxy server are not supported in pure java if (!testingJavaImpl()) { - // TODO("https://github.com/envoyproxy/envoy-mobile/issues/1426"): remove the "if" crutch - if (expected.getReceivedByteCount() >= 0) { - assertEquals(expected.getReceivedByteCount(), actual.getReceivedByteCount()); - } + assertEquals(expected.getReceivedByteCount(), actual.getReceivedByteCount()); assertEquals(expected.getProxyServer(), actual.getProxyServer()); // This is a place where behavior intentionally differs between native and java assertEquals(expected.getNegotiatedProtocol(), actual.getNegotiatedProtocol()); From 4c8e4a74590958abd9fe6d4e19ee62d0b941ff61 Mon Sep 17 00:00:00 2001 From: Charles Le Borgne Date: Mon, 24 Jan 2022 18:26:52 +0000 Subject: [PATCH 08/12] Add missing file Signed-off-by: Charles Le Borgne --- library/kotlin/io/envoyproxy/envoymobile/mocks/MockStream.kt | 1 + 1 file changed, 1 insertion(+) diff --git a/library/kotlin/io/envoyproxy/envoymobile/mocks/MockStream.kt b/library/kotlin/io/envoyproxy/envoymobile/mocks/MockStream.kt index 7f326d22ab..afe1bfbf62 100644 --- a/library/kotlin/io/envoyproxy/envoymobile/mocks/MockStream.kt +++ b/library/kotlin/io/envoyproxy/envoymobile/mocks/MockStream.kt @@ -15,6 +15,7 @@ class MockStream internal constructor(underlyingStream: MockEnvoyHTTPStream) : S override fun getStreamId(): Long { return 0 } override fun getConnectionId(): Long { return 0 } override fun getAttemptCount(): Long { return 0 } + override fun getReceivedByteCount(): Long { return 0 } } private val mockFinalStreamIntel = object : EnvoyFinalStreamIntel { From e854aa46f5ffcf247b422967f79c582267db1229 Mon Sep 17 00:00:00 2001 From: Charles Le Borgne Date: Mon, 24 Jan 2022 19:55:58 +0000 Subject: [PATCH 09/12] Update c_types.h comments Signed-off-by: Charles Le Borgne --- library/common/types/c_types.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/library/common/types/c_types.h b/library/common/types/c_types.h index 9a558dbeb1..3dbaefc9d5 100644 --- a/library/common/types/c_types.h +++ b/library/common/types/c_types.h @@ -153,7 +153,10 @@ typedef struct { int64_t connection_id; // The number of internal attempts to carry out a request/operation. 0 if not present. uint64_t attempt_count; - // The number of bytes received from upstream. + // Mostly the number of bytes received from upstream. When this struct is sent through + // the onHeaders callback, its value is the size of the "trimmed" headers" - this does + // not include the status line. For all the other callbacks, it is truly what has been + // received up to now, not necessarily what has been processed by the callbacks. uint64_t received_byte_count; } envoy_stream_intel; From 38c956e47942a2ec52f74ccf873d6ad9cddbab1a Mon Sep 17 00:00:00 2001 From: Charles Le Borgne Date: Thu, 27 Jan 2022 00:09:12 +0000 Subject: [PATCH 10/12] Updated with latest logic. Signed-off-by: Charles Le Borgne --- library/common/http/client.cc | 37 ++++++++++--------- library/common/http/client.h | 15 +------- library/common/jni/jni_utility.cc | 2 +- library/common/types/c_types.h | 11 +++--- .../integration/client_integration_test.cc | 11 +++--- 5 files changed, 33 insertions(+), 43 deletions(-) diff --git a/library/common/http/client.cc b/library/common/http/client.cc index 838f71d4d8..dfb4fc77d1 100644 --- a/library/common/http/client.cc +++ b/library/common/http/client.cc @@ -41,20 +41,24 @@ void Client::DirectStreamCallbacks::encodeHeaders(const ResponseHeaderMap& heade ASSERT(http_client_.getStream(direct_stream_.stream_handle_, GetStreamFilters::ALLOW_FOR_ALL_STREAMS)); - direct_stream_.saveLatestStreamIntel(headerBytesReceived()); - if (end_stream) { - closeStream(); - } + // Capture some metadata before potentially closing the stream. absl::string_view alpn = ""; uint64_t response_status = Utility::getResponseStatus(headers); - if (direct_stream_.request_decoder_ && - direct_stream_.request_decoder_->streamInfo().upstreamInfo() && - direct_stream_.request_decoder_->streamInfo().upstreamInfo()->upstreamSslConnection()) { - alpn = direct_stream_.request_decoder_->streamInfo() - .upstreamInfo() - ->upstreamSslConnection() - ->alpn(); + if (direct_stream_.request_decoder_) { + direct_stream_.saveLatestStreamIntel(); + const auto& info = direct_stream_.request_decoder_->streamInfo(); + // Set ghe initial number of byte consumed from the response by this non terminal callbacks. + direct_stream_.stream_intel_.consumed_bytes_from_response = + info.getUpstreamBytesMeter() ? info.getUpstreamBytesMeter()->headerBytesReceived() : 0; + // Capture the alpn if available. + if (info.upstreamInfo() && info.upstreamInfo()->upstreamSslConnection()) { + alpn = info.upstreamInfo()->upstreamSslConnection()->alpn(); + } + } + + if (end_stream) { + closeStream(); } // Track success for later bookkeeping (stream could still be reset). @@ -84,7 +88,7 @@ void Client::DirectStreamCallbacks::encodeData(Buffer::Instance& data, bool end_ ASSERT(http_client_.getStream(direct_stream_.stream_handle_, GetStreamFilters::ALLOW_FOR_ALL_STREAMS)); - direct_stream_.saveLatestStreamIntel(bytesReceived()); + direct_stream_.saveLatestStreamIntel(); if (end_stream) { closeStream(); } @@ -123,6 +127,8 @@ void Client::DirectStreamCallbacks::sendDataToBridge(Buffer::Instance& data, boo // Cap by bytes_to_send_ if and only if applying explicit flow control. uint32_t bytes_to_send = calculateBytesToSend(data, bytes_to_send_); + // Update the number of byte consumed by this non terminal callbacks from the response. + direct_stream_.stream_intel_.consumed_bytes_from_response += bytes_to_send; // Only send end stream if all data is being sent. bool send_end_stream = end_stream && (bytes_to_send == data.length()); @@ -147,7 +153,7 @@ void Client::DirectStreamCallbacks::encodeTrailers(const ResponseTrailerMap& tra ASSERT(http_client_.getStream(direct_stream_.stream_handle_, GetStreamFilters::ALLOW_FOR_ALL_STREAMS)); - direct_stream_.saveLatestStreamIntel(bytesReceived()); + direct_stream_.saveLatestStreamIntel(); closeStream(); // Trailers always indicate the end of the stream. // For explicit flow control, don't send data unless prompted. @@ -291,14 +297,13 @@ envoy_final_stream_intel& Client::DirectStreamCallbacks::finalStreamIntel() { return direct_stream_.envoy_final_stream_intel_; } -void Client::DirectStream::saveLatestStreamIntel(uint64_t received_byte_count) { +void Client::DirectStream::saveLatestStreamIntel() { const auto& info = request_decoder_->streamInfo(); if (info.upstreamInfo()) { stream_intel_.connection_id = info.upstreamInfo()->upstreamConnectionId().value_or(-1); } stream_intel_.stream_id = static_cast(stream_handle_); stream_intel_.attempt_count = info.attemptCount().value_or(0); - stream_intel_.received_byte_count = received_byte_count; } void Client::DirectStream::saveFinalStreamIntel() { @@ -306,8 +311,6 @@ void Client::DirectStream::saveFinalStreamIntel() { return; } StreamInfo::setFinalStreamIntel(request_decoder_->streamInfo(), envoy_final_stream_intel_); - // stream_intel_ may have an outdated received_byte_count - the final one is correct. - stream_intel_.received_byte_count = envoy_final_stream_intel_.received_byte_count; } envoy_error Client::DirectStreamCallbacks::streamError() { diff --git a/library/common/http/client.h b/library/common/http/client.h index 8b3f3c5ce1..e1b45ecaac 100644 --- a/library/common/http/client.h +++ b/library/common/http/client.h @@ -170,19 +170,6 @@ class Client : public Logger::Loggable { private: bool hasBufferedData() { return response_data_.get() && response_data_->length() != 0; } - const StreamInfo::StreamInfo& streamInfo() { - return direct_stream_.request_decoder_->streamInfo(); - } - uint64_t headerBytesReceived() { - return streamInfo().getUpstreamBytesMeter() - ? streamInfo().getUpstreamBytesMeter()->headerBytesReceived() - : 0; - } - uint64_t bytesReceived() { - return streamInfo().getUpstreamBytesMeter() - ? streamInfo().getUpstreamBytesMeter()->wireBytesReceived() - : 0; - } void sendDataToBridge(Buffer::Instance& data, bool end_stream); void sendTrailersToBridge(const ResponseTrailerMap& trailers); @@ -261,7 +248,7 @@ class Client : public Logger::Loggable { } // Latches stream information as it may not be available when accessed. - void saveLatestStreamIntel(uint64_t received_byte_count); + void saveLatestStreamIntel(); // Latches latency info from stream info before it goes away. void saveFinalStreamIntel(); diff --git a/library/common/jni/jni_utility.cc b/library/common/jni/jni_utility.cc index 5cc999c188..b2743cc9b0 100644 --- a/library/common/jni/jni_utility.cc +++ b/library/common/jni/jni_utility.cc @@ -91,7 +91,7 @@ jlongArray native_stream_intel_to_array(JNIEnv* env, envoy_stream_intel stream_i critical_array[0] = static_cast(stream_intel.stream_id); critical_array[1] = static_cast(stream_intel.connection_id); critical_array[2] = static_cast(stream_intel.attempt_count); - critical_array[3] = static_cast(stream_intel.received_byte_count); + critical_array[3] = static_cast(stream_intel.consumed_bytes_from_response); // Here '0' (for which there is no named constant) indicates we want to commit the changes back // to the JVM and free the c array, where applicable. env->ReleasePrimitiveArrayCritical(j_array, critical_array, 0); diff --git a/library/common/types/c_types.h b/library/common/types/c_types.h index 3dbaefc9d5..2af2de2067 100644 --- a/library/common/types/c_types.h +++ b/library/common/types/c_types.h @@ -153,11 +153,12 @@ typedef struct { int64_t connection_id; // The number of internal attempts to carry out a request/operation. 0 if not present. uint64_t attempt_count; - // Mostly the number of bytes received from upstream. When this struct is sent through - // the onHeaders callback, its value is the size of the "trimmed" headers" - this does - // not include the status line. For all the other callbacks, it is truly what has been - // received up to now, not necessarily what has been processed by the callbacks. - uint64_t received_byte_count; + // Number of bytes consumed by the non terminal callbacks out of the response. + // NOTE: on terminal callbacks (on_complete, on_error_, on_cancel), this value will not be equal + // to envoy_final_stream_intel.received_byte_count. The latter represents the real number + // of bytes received before decompression. consumed_bytes_from_response omits the number + // number of bytes related to the Status Line, and is after decompression. + uint64_t consumed_bytes_from_response; } envoy_stream_intel; /** diff --git a/test/common/integration/client_integration_test.cc b/test/common/integration/client_integration_test.cc index 9ab90f7975..0425f915a9 100644 --- a/test/common/integration/client_integration_test.cc +++ b/test/common/integration/client_integration_test.cc @@ -36,7 +36,7 @@ typedef struct { uint32_t on_complete_calls; uint32_t on_error_calls; uint32_t on_cancel_calls; - uint64_t on_header_received_byte_count; + uint64_t on_header_consumed_bytes_from_response; uint64_t on_complete_received_byte_count; std::string status; ConditionalInitializer* terminal_callback; @@ -70,7 +70,7 @@ class ClientIntegrationTest : public BaseIntegrationTest, callbacks_called* cc_ = static_cast(context); cc_->on_headers_calls++; cc_->status = response_headers->Status()->value().getStringView(); - cc_->on_header_received_byte_count = intel.received_byte_count; + cc_->on_header_consumed_bytes_from_response = intel.consumed_bytes_from_response; return nullptr; }; bridge_callbacks_.on_data = [](envoy_data c_data, bool, envoy_stream_intel, @@ -80,13 +80,12 @@ class ClientIntegrationTest : public BaseIntegrationTest, release_envoy_data(c_data); return nullptr; }; - bridge_callbacks_.on_complete = - [](envoy_stream_intel intel, envoy_final_stream_intel final_intel, void* context) -> void* { + bridge_callbacks_.on_complete = [](envoy_stream_intel, envoy_final_stream_intel final_intel, + void* context) -> void* { callbacks_called* cc_ = static_cast(context); cc_->on_complete_received_byte_count = final_intel.received_byte_count; cc_->on_complete_calls++; cc_->terminal_callback->setReady(); - EXPECT_EQ(intel.received_byte_count, final_intel.received_byte_count); return nullptr; }; bridge_callbacks_.on_error = [](envoy_error error, envoy_stream_intel, envoy_final_stream_intel, @@ -211,7 +210,7 @@ TEST_P(ClientIntegrationTest, Basic) { ASSERT_EQ(cc_.status, "200"); ASSERT_EQ(cc_.on_data_calls, 2); ASSERT_EQ(cc_.on_complete_calls, 1); - ASSERT_EQ(cc_.on_header_received_byte_count, 27); + ASSERT_EQ(cc_.on_header_consumed_bytes_from_response, 27); ASSERT_EQ(cc_.on_complete_received_byte_count, 67); // stream_success gets charged for 2xx status codes. From cd95929bb9fd6658b3ff3554a21ea537a7fde805 Mon Sep 17 00:00:00 2001 From: Charles Le Borgne Date: Thu, 27 Jan 2022 01:06:19 +0000 Subject: [PATCH 11/12] Fix typos Signed-off-by: Charles Le Borgne --- library/common/http/client.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/common/http/client.cc b/library/common/http/client.cc index dfb4fc77d1..d18262cf82 100644 --- a/library/common/http/client.cc +++ b/library/common/http/client.cc @@ -44,11 +44,10 @@ void Client::DirectStreamCallbacks::encodeHeaders(const ResponseHeaderMap& heade // Capture some metadata before potentially closing the stream. absl::string_view alpn = ""; - uint64_t response_status = Utility::getResponseStatus(headers); if (direct_stream_.request_decoder_) { direct_stream_.saveLatestStreamIntel(); const auto& info = direct_stream_.request_decoder_->streamInfo(); - // Set ghe initial number of byte consumed from the response by this non terminal callbacks. + // Set the initial number of bytes consumed for the non terminal callbacks. direct_stream_.stream_intel_.consumed_bytes_from_response = info.getUpstreamBytesMeter() ? info.getUpstreamBytesMeter()->headerBytesReceived() : 0; // Capture the alpn if available. @@ -62,6 +61,7 @@ void Client::DirectStreamCallbacks::encodeHeaders(const ResponseHeaderMap& heade } // Track success for later bookkeeping (stream could still be reset). + uint64_t response_status = Utility::getResponseStatus(headers); success_ = CodeUtility::is2xx(response_status); ENVOY_LOG(debug, "[S{}] dispatching to platform response headers for stream (end_stream={}):\n{}", @@ -127,7 +127,7 @@ void Client::DirectStreamCallbacks::sendDataToBridge(Buffer::Instance& data, boo // Cap by bytes_to_send_ if and only if applying explicit flow control. uint32_t bytes_to_send = calculateBytesToSend(data, bytes_to_send_); - // Update the number of byte consumed by this non terminal callbacks from the response. + // Update the number of bytes consumed by this non terminal callback. direct_stream_.stream_intel_.consumed_bytes_from_response += bytes_to_send; // Only send end stream if all data is being sent. bool send_end_stream = end_stream && (bytes_to_send == data.length()); From b75695cab4fb07e6afee81c076dde6e371b7a987 Mon Sep 17 00:00:00 2001 From: Charles Le Borgne Date: Thu, 27 Jan 2022 15:14:49 +0000 Subject: [PATCH 12/12] Complete the renaming. Signed-off-by: Charles Le Borgne --- .../envoymobile/engine/EnvoyStreamIntelImpl.java | 8 ++++---- .../envoymobile/engine/types/EnvoyStreamIntel.java | 13 ++++++++++--- .../org/chromium/net/impl/CronetUrlRequest.java | 4 ++-- .../io/envoyproxy/envoymobile/mocks/MockStream.kt | 2 +- 4 files changed, 17 insertions(+), 10 deletions(-) diff --git a/library/java/io/envoyproxy/envoymobile/engine/EnvoyStreamIntelImpl.java b/library/java/io/envoyproxy/envoymobile/engine/EnvoyStreamIntelImpl.java index 47e4cb1ed0..d96d311bc2 100644 --- a/library/java/io/envoyproxy/envoymobile/engine/EnvoyStreamIntelImpl.java +++ b/library/java/io/envoyproxy/envoymobile/engine/EnvoyStreamIntelImpl.java @@ -6,13 +6,13 @@ class EnvoyStreamIntelImpl implements EnvoyStreamIntel { private long streamId; private long connectionId; private long attemptCount; - private long receivedByteCount; + private long consumedBytesFromResponse; EnvoyStreamIntelImpl(long[] values) { streamId = values[0]; connectionId = values[1]; attemptCount = values[2]; - receivedByteCount = values[3]; + consumedBytesFromResponse = values[3]; } @Override @@ -31,7 +31,7 @@ public long getAttemptCount() { } @Override - public long getReceivedByteCount() { - return receivedByteCount; + public long getConsumedBytesFromResponse() { + return consumedBytesFromResponse; } } diff --git a/library/java/io/envoyproxy/envoymobile/engine/types/EnvoyStreamIntel.java b/library/java/io/envoyproxy/envoymobile/engine/types/EnvoyStreamIntel.java index a64409135c..71f119bf71 100644 --- a/library/java/io/envoyproxy/envoymobile/engine/types/EnvoyStreamIntel.java +++ b/library/java/io/envoyproxy/envoymobile/engine/types/EnvoyStreamIntel.java @@ -4,6 +4,7 @@ * Exposes internal HTTP stream metrics, context, and other details. */ public interface EnvoyStreamIntel { + /** * An internal identifier for the stream. */ @@ -18,8 +19,14 @@ public interface EnvoyStreamIntel { * The number of internal attempts to carry out a request/operation. */ public long getAttemptCount(); - /* - * The number of bytes received from upstream. + + /** + * The number of bytes consumed by the non terminal callbacks, from the response. + * + *

>NOTE: on terminal callbacks (on_complete, on_error_, on_cancel), this value will not be + * equal to {@link EnvoyFinalStreamIntel#getReceivedByteCount()}. The latter represents the real + * number of bytes received before decompression. getConsumedBytesFromResponse() omits the number + * number of bytes related to the Status Line, and is after decompression. */ - public long getReceivedByteCount(); + public long getConsumedBytesFromResponse(); } diff --git a/library/java/org/chromium/net/impl/CronetUrlRequest.java b/library/java/org/chromium/net/impl/CronetUrlRequest.java index 21822397b8..787e51c857 100644 --- a/library/java/org/chromium/net/impl/CronetUrlRequest.java +++ b/library/java/org/chromium/net/impl/CronetUrlRequest.java @@ -662,7 +662,7 @@ private void recordEnvoyFinalStreamIntel(EnvoyFinalStreamIntel envoyFinalStreamI } private void recordEnvoyStreamIntel(EnvoyStreamIntel envoyStreamIntel) { - mUrlResponseInfo.setReceivedByteCount(envoyStreamIntel.getReceivedByteCount() + + mUrlResponseInfo.setReceivedByteCount(envoyStreamIntel.getConsumedBytesFromResponse() + mBytesReceivedFromRedirects); } @@ -732,7 +732,7 @@ public void onHeaders(Map> headers, boolean endStream, } if (locationField != null) { - mBytesReceivedFromLastRedirect = streamIntel.getReceivedByteCount(); + mBytesReceivedFromLastRedirect = streamIntel.getConsumedBytesFromResponse(); cancel(); // Abort the the original request - we are being redirected. } diff --git a/library/kotlin/io/envoyproxy/envoymobile/mocks/MockStream.kt b/library/kotlin/io/envoyproxy/envoymobile/mocks/MockStream.kt index bf43ed32b8..d85673347f 100644 --- a/library/kotlin/io/envoyproxy/envoymobile/mocks/MockStream.kt +++ b/library/kotlin/io/envoyproxy/envoymobile/mocks/MockStream.kt @@ -15,7 +15,7 @@ class MockStream internal constructor(underlyingStream: MockEnvoyHTTPStream) : S override fun getStreamId(): Long { return 0 } override fun getConnectionId(): Long { return 0 } override fun getAttemptCount(): Long { return 0 } - override fun getReceivedByteCount(): Long { return 0 } + override fun getConsumedBytesFromResponse(): Long { return 0 } } private val mockFinalStreamIntel = object : EnvoyFinalStreamIntel {