-
Notifications
You must be signed in to change notification settings - Fork 39
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
Changes from 14 commits
4bb8267
d698f95
3affddb
ae1be54
c16a7a1
96a987a
b301b4c
8bfde0f
410168c
7db989f
875af74
308fc5a
0295548
2f2fc2b
edbf2ff
d56ce46
9a80be9
869647b
e66693e
a4666d6
2d29df7
e50a094
fe65019
3d1235f
eb040d8
af5f2e1
8621575
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,48 +11,61 @@ 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. | ||
*/ | ||
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); | ||
} 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() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Java SDK can do |
||
return exception; | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm confused whether the user is allowed to pass NULL for if they're allowed to pass NULL, then this will crash when you call if they're not allowed to pass NULL, why is there a probably, There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)->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); | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.