Skip to content

Commit

Permalink
mobile: Add response codes to the error details (#34067)
Browse files Browse the repository at this point in the history 
The HTTP response code is not reported in the Cronvoy exceptions, and
the error_code, which maps from some small subset of response codes to
envoy_error_code_t, is not used in the onError function. As a first
step, we want to append the HTTP response code and the Envoy error code
to the error details.

A subsequent PR will add more support for mapping Http::Code to
envoy_error_code_t (or getting rid of envoy_error_code_t all together).

Signed-off-by: Ali Beyad <abeyad@google.com>
  • Loading branch information
@abeyad
abeyad committed May 10, 2024
1 parent d28004d commit f83c395
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 17 deletions.
16 changes: 15 additions & 1 deletion mobile/library/common/http/client.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
#include "source/common/http/headers.h"
#include "source/common/http/utility.h"

#include "absl/strings/str_cat.h"
#include "absl/strings/str_join.h"
#include "library/common/bridge/utility.h"
#include "library/common/http/header_utility.h"
#include "library/common/stream_info/extra_stream_info.h"
Expand Down Expand Up @@ -436,16 +438,28 @@ void Client::DirectStreamCallbacks::latchError() {
}
const auto& info = request_decoder->streamInfo();

std::vector<std::string> error_msg_details;
if (info.responseCode().has_value()) {
error_->error_code = Bridge::Utility::errorCodeFromLocalStatus(
static_cast<Http::Code>(info.responseCode().value()));
error_msg_details.push_back(absl::StrCat("RESPONSE_CODE: ", info.responseCode().value()));
} else if (StreamInfo::isStreamIdleTimeout(info)) {
error_->error_code = ENVOY_REQUEST_TIMEOUT;
} else {
error_->error_code = ENVOY_STREAM_RESET;
}

error_->message = info.responseCodeDetails().value_or("");
error_msg_details.push_back(absl::StrCat("ERROR_CODE: ", error_->error_code));
if (std::string resp_code_details = info.responseCodeDetails().value_or("");
!resp_code_details.empty()) {
error_msg_details.push_back(absl::StrCat("DETAILS: ", std::move(resp_code_details)));
}
// The format of the error message propogated to callbacks is:
// RESPONSE_CODE: {RESPONSE_CODE}|ERROR_CODE: {ERROR_CODE}|DETAILS: {DETAILS}
// Where RESPONSE_CODE is the HTTP response code from StreamInfo::responseCode().
// ERROR_CODE is of the envoy_error_code_t enum type, and gets mapped from RESPONSE_CODE.
// DETAILS is the contents of StreamInfo::responseCodeDetails().
error_->message = absl::StrJoin(std::move(error_msg_details), "|");
error_->attempt_count = info.attemptCount().value_or(0);
}

Expand Down
7 changes: 5 additions & 2 deletions mobile/test/common/http/client_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

using testing::_;
using testing::AnyNumber;
using testing::ContainsRegex;
using testing::NiceMock;
using testing::Return;
using testing::ReturnPointee;
Expand Down Expand Up @@ -444,6 +445,8 @@ TEST_P(ClientTest, EnvoyLocalError) {
stream_callbacks.on_error_ = [&](EnvoyError error, envoy_stream_intel,
envoy_final_stream_intel) -> void {
EXPECT_EQ(error.error_code, ENVOY_CONNECTION_FAILURE);
EXPECT_THAT(error.message,
ContainsRegex("RESPONSE_CODE: 503|ERROR_CODE: 2|DETAILS: failed miserably"));
EXPECT_EQ(error.attempt_count, 123);
callbacks_called.on_error_calls_++;
};
Expand All @@ -463,7 +466,7 @@ TEST_P(ClientTest, EnvoyLocalError) {
EXPECT_CALL(dispatcher_, popTrackedObject(_));
EXPECT_CALL(dispatcher_, deferredDelete_(_));
stream_info_.setResponseCode(503);
stream_info_.setResponseCodeDetails("nope");
stream_info_.setResponseCodeDetails("failed miserably");
stream_info_.setAttemptCount(123);
response_encoder_->getStream().resetStream(Http::StreamResetReason::LocalConnectionFailure);
ASSERT_EQ(callbacks_called.on_headers_calls_, 0);
Expand Down Expand Up @@ -518,7 +521,7 @@ TEST_P(ClientTest, RemoteResetAfterStreamStart) {
stream_callbacks.on_error_ = [&](EnvoyError error, envoy_stream_intel,
envoy_final_stream_intel) -> void {
EXPECT_EQ(error.error_code, ENVOY_STREAM_RESET);
EXPECT_EQ(error.message.length(), 0);
EXPECT_THAT(error.message, ContainsRegex("ERROR_CODE: 1"));
EXPECT_EQ(error.attempt_count, 0);
callbacks_called.on_error_calls_++;
};
Expand Down
37 changes: 23 additions & 14 deletions mobile/test/java/org/chromium/net/CronetUrlRequestTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2056,20 +2056,24 @@ public void testDestroyUploadDataStreamAdapterOnSucceededCallback() throws Excep
@Feature({"Cronet"})
public void testErrorCodes() throws Exception {
checkSpecificErrorCode(EnvoyMobileError.DNS_RESOLUTION_FAILED, NetError.ERR_NAME_NOT_RESOLVED,
NetworkException.ERROR_HOSTNAME_NOT_RESOLVED, "NAME_NOT_RESOLVED",
false);
NetworkException.ERROR_HOSTNAME_NOT_RESOLVED, "NAME_NOT_RESOLVED", false,
/*error_details=*/"RESPONSE_CODE: 400|ERROR_CODE: 0");
checkSpecificErrorCode(EnvoyMobileError.UPSTREAM_CONNECTION_TERMINATION,
NetError.ERR_CONNECTION_CLOSED, NetworkException.ERROR_CONNECTION_CLOSED,
"CONNECTION_CLOSED", true);
"CONNECTION_CLOSED", true,
/*error_details=*/"RESPONSE_CODE: 400|ERROR_CODE: 0");
checkSpecificErrorCode(EnvoyMobileError.UPSTREAM_CONNECTION_FAILURE,
NetError.ERR_CONNECTION_REFUSED,
NetworkException.ERROR_CONNECTION_REFUSED, "CONNECTION_REFUSED", false);
NetworkException.ERROR_CONNECTION_REFUSED, "CONNECTION_REFUSED", false,
/*error_details=*/"RESPONSE_CODE: 400|ERROR_CODE: 0");
checkSpecificErrorCode(EnvoyMobileError.UPSTREAM_REMOTE_RESET, NetError.ERR_CONNECTION_RESET,
NetworkException.ERROR_CONNECTION_RESET, "CONNECTION_RESET", true);
NetworkException.ERROR_CONNECTION_RESET, "CONNECTION_RESET", true,
/*error_details=*/"RESPONSE_CODE: 400|ERROR_CODE: 0");
checkSpecificErrorCode(EnvoyMobileError.STREAM_IDLE_TIMEOUT, NetError.ERR_TIMED_OUT,
NetworkException.ERROR_TIMED_OUT, "TIMED_OUT", true);
checkSpecificErrorCode(0x2000, NetError.ERR_OTHER, NetworkException.ERROR_OTHER, "OTHER",
false);
NetworkException.ERROR_TIMED_OUT, "TIMED_OUT", true,
/*error_details=*/"RESPONSE_CODE: 400|ERROR_CODE: 0");
checkSpecificErrorCode(0x2000, NetError.ERR_OTHER, NetworkException.ERROR_OTHER, "OTHER", false,
/*error_details=*/"RESPONSE_CODE: 400|ERROR_CODE: 0");
}

/*
Expand All @@ -2091,9 +2095,10 @@ public void testInternetDisconnectedError() throws Exception {
androidNetworkMonitor.onReceive(getContext(), intent);

// send request and confirm errorcode
checkSpecificErrorCode(
EnvoyMobileError.DNS_RESOLUTION_FAILED, NetError.ERR_INTERNET_DISCONNECTED,
NetworkException.ERROR_INTERNET_DISCONNECTED, "INTERNET_DISCONNECTED", false);
checkSpecificErrorCode(EnvoyMobileError.DNS_RESOLUTION_FAILED,
NetError.ERR_INTERNET_DISCONNECTED,
NetworkException.ERROR_INTERNET_DISCONNECTED, "INTERNET_DISCONNECTED",
false, /*error_details=*/"RESPONSE_CODE: 400|ERROR_CODE: 0");

// bring back online since the AndroidNetworkMonitor class is a singleton
connectivityManager.setActiveNetworkInfo(networkInfo);
Expand Down Expand Up @@ -2205,8 +2210,8 @@ public void onCanceled(UrlRequest request, UrlResponseInfo info) {
}

private void checkSpecificErrorCode(@EnvoyMobileError long envoyMobileError, NetError netError,
int errorCode, String name, boolean immediatelyRetryable)
throws Exception {
int errorCode, String name, boolean immediatelyRetryable,
String errorDetails) throws Exception {
TestUrlRequestCallback callback =
startAndWaitForComplete(mMockUrlRequestJobFactory.getCronetEngine(),
MockUrlRequestJobFactory.getMockUrlWithFailure(envoyMobileError));
Expand All @@ -2221,7 +2226,7 @@ private void checkSpecificErrorCode(@EnvoyMobileError long envoyMobileError, Net
assertEquals(0, callback.mRedirectCount);
assertTrue(callback.mOnErrorCalled);
assertEquals(ResponseStep.ON_FAILED, callback.mResponseStep);
assertEquals("", ((CronvoyNetworkExceptionImpl)callback.mError).getErrorDetails());
assertEquals(errorDetails, ((CronvoyNetworkExceptionImpl)callback.mError).getErrorDetails());
}

// Returns the contents of byteBuffer, from its position() to its limit(),
Expand Down Expand Up @@ -2311,6 +2316,10 @@ public void test404Head() throws Exception {
NativeTestServer.getFileURL("/notfound.html"), callback, callback.getExecutor());
builder.setHttpMethod("HEAD").build().start();
callback.blockForDone();
checkSpecificErrorCode(EnvoyMobileError.UPSTREAM_CONNECTION_FAILURE,
NetError.ERR_CONNECTION_REFUSED,
NetworkException.ERROR_CONNECTION_REFUSED, "CONNECTION_REFUSED", false,
/*error_details=*/"RESPONSE_CODE: 400|ERROR_CODE: 0");
}

@Test
Expand Down

0 comments on commit f83c395

Please sign in to comment.