Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Keep Record of Original Exception in S3 Callbacks #700

Merged
merged 27 commits into from
Oct 25, 2023
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/main/java/software/amazon/awssdk/crt/CrtRuntimeException.java
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,17 @@ public CrtRuntimeException(int errorCode) {
this.errorName = CRT.awsErrorName(errorCode);
}

/**
* Constructor for Crt exceptions due to native errors with a cause.
* @param errorCode native error code detailing the reason for the exception
* @param cause cause of the exception such as a Java exception in a callback
*/
public CrtRuntimeException(int errorCode, Throwable cause) {
super(CRT.awsErrorString(errorCode), cause);
this.errorCode = errorCode;
this.errorName = CRT.awsErrorName(errorCode);
}

@Override
public String toString() {
return String.format("%s %s(%d)", super.toString(), errorName, errorCode);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,17 +29,26 @@ public CrtS3RuntimeException(int errorCode, int responseStatus, String errorPayl
this.awsErrorMessage = GetElementFromPayload(errorPayload, messageBeginBlock, messageEndBlock);
}

public CrtS3RuntimeException(S3FinishedResponseContext context) {
super(context.getErrorCode());
String errorString = new String(context.getErrorPayload(), java.nio.charset.StandardCharsets.UTF_8);
this.statusCode = context.getResponseStatus();

public CrtS3RuntimeException(int errorCode, int responseStatus, byte[] errorPayload) {
super(errorCode);
String errorString = new String(errorPayload, java.nio.charset.StandardCharsets.UTF_8);
this.statusCode = responseStatus;
this.errorPayload = errorString;
this.awsErrorCode = GetElementFromPayload(this.errorPayload, codeBeginBlock, codeEndBlock);
this.awsErrorMessage = GetElementFromPayload(this.errorPayload, messageBeginBlock, messageEndBlock);
}

public CrtS3RuntimeException(int errorCode, int responseStatus, byte[] errorPayload) {
super(errorCode);

/**
* Helper function to create a runtime exception from S3 response and a cause.
* @param errorCode The CRT error code
* @param responseStatus statusCode of the HTTP response
* @param errorPayload body of the error response
* @param cause cause of the exception such as a Java exception in a callback
*/
public CrtS3RuntimeException(int errorCode, int responseStatus, byte[] errorPayload, Throwable cause) {
super(errorCode, cause);
String errorString = new String(errorPayload, java.nio.charset.StandardCharsets.UTF_8);
this.statusCode = responseStatus;
this.errorPayload = errorString;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,48 +11,62 @@ public class S3FinishedResponseContext {
private final ChecksumAlgorithm checksumAlgorithm;
private final boolean didValidateChecksum;

private CrtS3RuntimeException exception;

/*
* errorCode The CRT error code
* responseStatus statusCode of the HTTP response
* errorPayload body of the error response. Can be null if the request completed successfully
* checksumAlgorithm, the algorithm used to validate the Body, None if not validated
* didValidateChecksum which is true if the response was validated.
* exception CrtS3RuntimeException if any error occurred. May contain the cause of the exception such as a Java exception in a callback.
*/
S3FinishedResponseContext(final int errorCode, final int responseStatus, final byte[] errorPayload, final ChecksumAlgorithm checksumAlgorithm, final boolean didValidateChecksum) {
S3FinishedResponseContext(final int errorCode, final int responseStatus, final byte[] errorPayload, final ChecksumAlgorithm checksumAlgorithm, final boolean didValidateChecksum, Throwable cause) {
this.errorCode = errorCode;
this.responseStatus = responseStatus;
this.errorPayload = errorPayload;
this.checksumAlgorithm = checksumAlgorithm;
this.didValidateChecksum = didValidateChecksum;
if (cause != null) {
this.exception = new CrtS3RuntimeException(errorCode, responseStatus, errorPayload, cause);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the cause is not null, should we return the cause as-is without wrapping? This will make it eaiser for users to handle the exception. See similar feedback we got before aws/aws-sdk-java-v2#4356

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if we want to omit the CRT layer completely from the stack trace, even if the failure was from a Java callback. I do understand the annoyance of nested exceptions, and from a design perspective, we do want to improve error handling in a Java idiomatic way instead of giving access to the CRT error code. See #683 (comment). We should not have exposed the error code directly to begin with, but now we have to maintain backward compatibility.

Debatable: I think it is better for CRT to have a unified exception and the SDK can do exception.getCause() to de-nest it where required or pass S3ClientException as a cause to SDKClientException.

Copy link
Contributor

@graebm graebm Oct 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if Zoe says it's OK to just provide the underlying connection, I'm down with it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed it with @TingDaoK as well, and decided to remove CrtS3RuntimeException since the same data was repeated in two places.

} else if (errorCode != 0) {
this.exception = new CrtS3RuntimeException(errorCode, responseStatus, errorPayload);
}
}

public int getErrorCode () {
public int getErrorCode() {
return this.errorCode;
}

/*
* If the request didn't receive a response due to a connection
* failure or some other issue the response status will be 0.
*/
public int getResponseStatus () {
public int getResponseStatus() {
return this.responseStatus;
}

/*
* In the case of a failed http response get the payload of the response.
*/
public byte[] getErrorPayload () {
public byte[] getErrorPayload() {
return this.errorPayload;
}

/*
* if no checksum is found, or the request finished with an error the Algorithm will be None,
* otherwise the algorithm will correspond to the one attached to the object when uploaded.
*/
public ChecksumAlgorithm getChecksumAlgorithm () {
public ChecksumAlgorithm getChecksumAlgorithm() {
return this.checksumAlgorithm;
}
public boolean isChecksumValidated () {

public boolean isChecksumValidated() {
return this.didValidateChecksum;
}

public CrtS3RuntimeException getException() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we just make this return the (nullable) cause instead of CrtS3RuntimeException? In our current implementation, the CRT client in the SDK just checks the error code and returns our own SdkClientException and we can't change it now. https://github.com/aws/aws-sdk-java-v2/blob/master/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/crt/S3CrtResponseHandlerAdapter.java#L134

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Java SDK can do exception.errorMessage instead of parsing the errorCode.

return exception;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ int onResponseBody(byte[] bodyBytesIn, long objectRangeStart, long objectRangeEn
return this.responseHandler.onResponseBody(ByteBuffer.wrap(bodyBytesIn), objectRangeStart, objectRangeEnd);
}

void onFinished(int errorCode, int responseStatus, byte[] errorPayload, int checksumAlgorithm, boolean didValidateChecksum) {
S3FinishedResponseContext context = new S3FinishedResponseContext(errorCode, responseStatus, errorPayload, ChecksumAlgorithm.getEnumValueFromInteger(checksumAlgorithm), didValidateChecksum);
void onFinished(int errorCode, int responseStatus, byte[] errorPayload, int checksumAlgorithm, boolean didValidateChecksum, Throwable cause) {
S3FinishedResponseContext context = new S3FinishedResponseContext(errorCode, responseStatus, errorPayload, ChecksumAlgorithm.getEnumValueFromInteger(checksumAlgorithm), didValidateChecksum, cause);
this.responseHandler.onFinished(context);
}

Expand Down
12 changes: 12 additions & 0 deletions src/native/crt.c
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,18 @@ bool aws_jni_check_and_clear_exception(JNIEnv *env) {
return exception_pending;
}

bool aws_jni_get_and_clear_exception(JNIEnv *env, jthrowable *out) {
bool exception_pending = (*env)->ExceptionCheck(env);
if (exception_pending) {
(*env)->DeleteGlobalRef(env, *out);
waahm7 marked this conversation as resolved.
Show resolved Hide resolved
if (out != NULL) {
*out = (jthrowable)(*env)->NewGlobalRef(env, (*env)->ExceptionOccurred(env));
Copy link
Contributor

@graebm graebm Oct 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused whether the user is allowed to pass NULL for out?

if they're allowed to pass NULL, then this will crash when you call (*env)->DeleteGlobalRef(env, *out);

if they're not allowed to pass NULL, why is there a if (out != NULL) check?

probably, out should never be NULL

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I tried to make it NULL-tolerant but missed the (*env)->DeleteGlobalRef(env, *out);. I have removed the if (out != NULL) condition, since other functions like aws_jni_byte_array_from_cursor are also not NULL-tolerant.

}
(*env)->ExceptionClear(env);
}
return exception_pending;
}

int aws_size_t_from_java(JNIEnv *env, size_t *out_size, jlong java_long, const char *errmsg_prefix) {
if (java_long < 0) {
aws_jni_throw_illegal_argument_exception(env, "%s cannot be negative", errmsg_prefix);
Expand Down
15 changes: 15 additions & 0 deletions src/native/crt.h
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,21 @@ void aws_jni_throw_illegal_argument_exception(JNIEnv *env, const char *msg, ...)
******************************************************************************/
bool aws_jni_check_and_clear_exception(JNIEnv *env);

/*******************************************************************************
* Checks whether or not an exception is pending on the stack.
* If the exception is pending, deletes existing global reference of `out`, sets `out` to the new exception and clears
* it.
*
* @param env A pointer to the JNI environment, used to interact with the JVM.
* @param out A pointer to a jthrowable object. If an exception is pending, the function
* deletes any existing global reference pointed to by 'out', and sets 'out'
* to point to the new exception.
*
* @return true if an exception was pending and has been cleared; false otherwise.
*
******************************************************************************/
bool aws_jni_get_and_clear_exception(JNIEnv *env, jthrowable *out);

/*******************************************************************************
* Set a size_t based on a jlong.
* If conversion fails, a java IllegalArgumentException is thrown like
Expand Down
2 changes: 1 addition & 1 deletion src/native/java_class_ids.c
Original file line number Diff line number Diff line change
Expand Up @@ -693,7 +693,7 @@ static void s_cache_s3_meta_request_response_handler_native_adapter_properties(J
AWS_FATAL_ASSERT(s3_meta_request_response_handler_native_adapter_properties.onResponseBody);

s3_meta_request_response_handler_native_adapter_properties.onFinished =
(*env)->GetMethodID(env, cls, "onFinished", "(II[BIZ)V");
(*env)->GetMethodID(env, cls, "onFinished", "(II[BIZLjava/lang/Throwable;)V");
AWS_FATAL_ASSERT(s3_meta_request_response_handler_native_adapter_properties.onFinished);

s3_meta_request_response_handler_native_adapter_properties.onResponseHeaders =
Expand Down
15 changes: 11 additions & 4 deletions src/native/s3_client.c
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ struct s3_client_make_meta_request_callback_data {
jobject java_s3_meta_request_response_handler_native_adapter;
struct aws_input_stream *input_stream;
struct aws_signing_config_data signing_config_data;
jthrowable java_exception;
};

static void s_on_s3_client_shutdown_complete_callback(void *user_data);
Expand Down Expand Up @@ -354,10 +355,10 @@ static int s_on_s3_meta_request_body_callback(
range_start,
range_end);

if (aws_jni_check_and_clear_exception(env)) {
if (aws_jni_get_and_clear_exception(env, &(callback_data->java_exception))) {
AWS_LOGF_ERROR(
AWS_LS_S3_META_REQUEST,
"id=%p: Ignored Exception from S3MetaRequest.onResponseBody callback",
"id=%p: Received exception from S3MetaRequest.onResponseBody callback",
(void *)meta_request);
aws_raise_error(AWS_ERROR_HTTP_CALLBACK_FAILURE);
goto cleanup;
Expand Down Expand Up @@ -432,7 +433,7 @@ static int s_on_s3_meta_request_headers_callback(
response_status,
java_headers_buffer);

if (aws_jni_check_and_clear_exception(env)) {
if (aws_jni_get_and_clear_exception(env, &(callback_data->java_exception))) {
AWS_LOGF_ERROR(
AWS_LS_S3_META_REQUEST,
"id=%p: Exception thrown from S3MetaRequest.onResponseHeaders callback",
Expand Down Expand Up @@ -481,6 +482,10 @@ static void s_on_s3_meta_request_finish_callback(
error_response_cursor = aws_byte_cursor_from_buf(error_response_body);
}
jbyteArray jni_payload = aws_jni_byte_array_from_cursor(env, &error_response_cursor);
/* Only propagate java_exception if crt error code is callback failure */
jthrowable java_exception =
meta_request_result->error_code == AWS_ERROR_HTTP_CALLBACK_FAILURE ? callback_data->java_exception : NULL;

(*env)->CallVoidMethod(
env,
callback_data->java_s3_meta_request_response_handler_native_adapter,
Expand All @@ -489,7 +494,8 @@ static void s_on_s3_meta_request_finish_callback(
meta_request_result->response_status,
jni_payload,
meta_request_result->validation_algorithm,
meta_request_result->did_validate);
meta_request_result->did_validate,
java_exception);

if (aws_jni_check_and_clear_exception(env)) {
AWS_LOGF_ERROR(
Expand Down Expand Up @@ -571,6 +577,7 @@ static void s_s3_meta_request_callback_cleanup(
if (callback_data) {
(*env)->DeleteGlobalRef(env, callback_data->java_s3_meta_request);
(*env)->DeleteGlobalRef(env, callback_data->java_s3_meta_request_response_handler_native_adapter);
(*env)->DeleteGlobalRef(env, callback_data->java_exception);
aws_signing_config_data_clean_up(&callback_data->signing_config_data, env);
aws_mem_release(aws_jni_get_allocator(), callback_data);
}
Expand Down
58 changes: 54 additions & 4 deletions src/test/java/software/amazon/awssdk/crt/test/S3ClientTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,6 @@ public void testS3Get() {
skipIfAndroid();
skipIfNetworkUnavailable();
Assume.assumeTrue(hasAwsCredentials());

S3ClientOptions clientOptions = new S3ClientOptions().withRegion(REGION);
try (S3Client client = createS3Client(clientOptions)) {
CompletableFuture<Integer> onFinishedFuture = new CompletableFuture<>();
Expand All @@ -290,9 +289,7 @@ public void onFinished(S3FinishedResponseContext context) {
Log.log(Log.LogLevel.Info, Log.LogSubject.JavaCrtS3,
"Meta request finished with error code " + context.getErrorCode());
if (context.getErrorCode() != 0) {
onFinishedFuture.completeExceptionally(
new CrtS3RuntimeException(context.getErrorCode(), context.getResponseStatus(),
context.getErrorPayload()));
onFinishedFuture.completeExceptionally(context.getException());
return;
}
onFinishedFuture.complete(Integer.valueOf(context.getErrorCode()));
Expand All @@ -314,6 +311,59 @@ public void onFinished(S3FinishedResponseContext context) {
}
}

@Test
public void testS3CallbackExceptionIsProperlyPropagated() {
skipIfAndroid();
skipIfNetworkUnavailable();
Assume.assumeTrue(hasAwsCredentials());
S3ClientOptions clientOptions = new S3ClientOptions().withRegion(REGION);
RuntimeException expectedException = new RuntimeException("Exception From a Java Function");
boolean foundExpectedException = false;

try (S3Client client = createS3Client(clientOptions)) {
CompletableFuture<Integer> onFinishedFuture = new CompletableFuture<>();
S3MetaRequestResponseHandler responseHandler = new S3MetaRequestResponseHandler() {

@Override
public int onResponseBody(ByteBuffer bodyBytesIn, long objectRangeStart, long objectRangeEnd) {
throw expectedException;
}

@Override
public void onFinished(S3FinishedResponseContext context) {
Log.log(Log.LogLevel.Info, Log.LogSubject.JavaCrtS3,
"Meta request finished with error code " + context.getErrorCode());
if (context.getErrorCode() != 0) {
onFinishedFuture.completeExceptionally(context.getException());
return;
}
onFinishedFuture.complete(Integer.valueOf(context.getErrorCode()));
}
};

HttpHeader[] headers = { new HttpHeader("Host", ENDPOINT) };
HttpRequest httpRequest = new HttpRequest("GET", "/get_object_test_1MB.txt", headers, null);

S3MetaRequestOptions metaRequestOptions = new S3MetaRequestOptions()
.withMetaRequestType(MetaRequestType.GET_OBJECT).withHttpRequest(httpRequest)
.withResponseHandler(responseHandler);

try (S3MetaRequest metaRequest = client.makeMetaRequest(metaRequestOptions)) {
Assert.assertEquals(Integer.valueOf(0), onFinishedFuture.get());
}
} catch (InterruptedException | ExecutionException ex) {
Throwable cause = ex.getCause();
while (cause != null) {
if (cause == expectedException) {
foundExpectedException = true;
break;
}
cause = cause.getCause();
}
}
Assert.assertTrue("Expected exception not found in the cause chain", foundExpectedException);
}

@Test
public void testS3GetWithEndpoint() {
skipIfAndroid();
Expand Down
Loading