Skip to content

Treat UncheckedIOException as IOExceptions #5319

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

Closed
wants to merge 9 commits into from

Conversation

anirudh9391
Copy link
Contributor

@anirudh9391 anirudh9391 commented Jun 21, 2024

Motivation and Context

Timeout triggered termination of the connection stream, causes SDK to not perform retries even if configured.

Modifications

Users of DynamoDB experienced and reported that API timeouts causing UncheckedIOException do not get retried, despite the client being configured to retry timeout exceptions.

Testing

Added unit test

Screenshots (if appropriate)

Types of changes

  • [ x ] Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

@anirudh9391 anirudh9391 requested a review from a team as a code owner June 21, 2024 00:03
@joviegas
Copy link
Contributor

joviegas commented Jun 21, 2024

Could you please mention some details like which service was facing this issue and how this issue surfaced ?
In this particular case who exactly was throwing UncheckedIOException ?
Also please add changelogs

@joviegas
Copy link
Contributor

Something to also consider while fixing is instance like

public final String asString(Charset charset) throws UncheckedIOException {
where UnCheckedIOException are exception which do not pass on retries .

@anirudh9391
Copy link
Contributor Author

aws-sdk-java-v2/core/sdk-core/src/main/java/software/amazon/awssdk/core/BytesWrapper.java

Yes, as discussed offline yesterday, we need to cut a broader task to identify and handle UncheckedIOExceptions throughout the SDK codebase. Now that we know it most certainly has to be retried. This SIM addresses the imminent task, which is timeout caused UncheckedIOExceptions.

Copy link

@joviegas
Copy link
Contributor

joviegas commented Jul 2, 2024

  • Can we please update the title and change log to reflect that these changes modify the handling of exceptions when closing the response stream during an SDKInterrupted exception, changing it to just log the cause of failure.
  • If possible, can we write a functional test or at least perform a one-off test to ensure this addresses the issue.
  • Can we please also update the fix for ApiCallAttemptTimeoutTrackingStage since the reported stack trace showed this class.

@@ -90,8 +92,8 @@ public Response<OutputT> execute(SdkHttpFullRequest request, RequestExecutionCon
* @return The translated exception.
*/
private Exception translatePipelineException(RequestExecutionContext context, Exception e) {
if (e instanceof InterruptedException || e instanceof IOException ||
e instanceof AbortedException || Thread.currentThread().isInterrupted()
if (e instanceof InterruptedException || e instanceof IOException
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we reformat it as before so that it doesnot show in diff , else in GitBlame it will show up as new change

@zoewangg
Copy link
Contributor

Closing in favor of #5439

@zoewangg zoewangg closed this Jul 29, 2024
@zoewangg zoewangg deleted the anirudkr/handle-unchecked-io branch July 29, 2024 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants