Keep Record of Original Exception in S3 Callbacks#700
Conversation
waahm7
changed the title
| (*env)->DeleteGlobalRef(env, *out); | ||
| if (out != NULL) { | ||
| *out = (jthrowable)(*env)->NewGlobalRef(env, (*env)->ExceptionOccurred(env)); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| this.checksumAlgorithm = checksumAlgorithm; | ||
| this.didValidateChecksum = didValidateChecksum; | ||
| if (cause != null) { | ||
| this.exception = new CrtS3RuntimeException(errorCode, responseStatus, errorPayload, cause); |
There was a problem hiding this comment.
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.
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.
if Zoe says it's OK to just provide the underlying connection, I'm down with it
There was a problem hiding this comment.
Discussed it with @TingDaoK as well, and decided to remove CrtS3RuntimeException since the same data was repeated in two places.
| return this.didValidateChecksum; | ||
| } | ||
|
|
||
| public CrtS3RuntimeException getException() { |
There was a problem hiding this comment.
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.
The Java SDK can do exception.errorMessage instead of parsing the errorCode.
waahm7
changed the title
…e the error body directly.
4 participants
Issue #, if available:
#697
Description of changes:
Adds a new exception parameter to S3FinishedResponseContext, which can contain the cause of the exception if a Java exception is thrown from the onHeaders or onBody callback.
Some caveats:
Example output:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.