Skip to content

Conversation

@lestephane
Copy link

Sending the request body may fail due to an IOException when the server
has closed the underlying socket without a preliminary hint such as
a Connection: close response header.

This closure comes in the form of a server-initiated TLS alert
(close_notify). The cause of these intermittent non-orderly closures is
unclear. It could be based on the volume of data sent over the socket,
or the count of requests submitted. I suspect AWS uses an ELB fronting
an API Gateway on the endpoint side, and those decisions are made based
on some policy that the client has no control over, therefore there
closures will happen, and the requests need to be retried when that
happens.

However, When this does, the invokeSafely() wraps the IOException into an
UncheckedIOException which makes the retriable error non-retriable,
because UncheckedIOException extends RuntimeException, which to the AWS
SDK looks like a nondescript error condition that could / should not be
retried.

This Bugfix propagates the UncheckedIOException's inner IOException cause
so that the UrlConnectionHttpClient may benefit from the same retry policy
as the ApacheHttpClient for request body sending.

Motivation and Context

The UrlConnectionClient is not working as reliably as the ApacheClient
if it cannot benefit from the same retry capability.

The Pull Request #2848
improved the situation by allowing the use of the UrlConnectionClient
for requests with very large bodies. But this won't help if the client
cannot handle occasionally closed connections.

Description

Catch UncheckedIOException, and rethrow the IOException cause.

Testing

It is not possible to have the AWS endpoint close a connection in a
deterministic way.

I made the change in a private project, and was able to see such
IOExceptions be considered retriable, and be retried through the SDK
retry policy.

Screenshots (if appropriate)

Types of changes

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

Checklist

  • I have read the CONTRIBUTING document
  • Local run of mvn install succeeds
    -> ran install -pl :bom-internal,:s3 -P quick --am -T 2C
  • My code follows the code style of this project
    -> can't find the code style information
  • My change requires a change to the Javadoc documentation
  • I have updated the Javadoc documentation accordingly
  • I have read the README document
  • I have added tests to cover my changes
  • All new and existing tests passed
  • A short description of the change has been added to the CHANGELOG
  • My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

  • I confirm that this pull request can be released under the Apache 2 license

@lestephane lestephane requested a review from a team as a code owner December 6, 2021 15:28
@lestephane lestephane changed the title propagate ioexceptions during request body sending to benefit from retry policy [UrlConnectionHttpClient] propagate ioexceptions during request body sending to benefit from retry policy Dec 6, 2021
@lestephane lestephane changed the title [UrlConnectionHttpClient] propagate ioexceptions during request body sending to benefit from retry policy [UrlConnectionHttpClient] propagate IOExceptions during request body sending (to benefit from the aws sdk retry policy) Dec 6, 2021
@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 6, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

33.3% 33.3% Coverage
0.0% 0.0% Duplication

@lestephane
Copy link
Author

Closing until I have a test case for the abrupt-closure-from-the-server-side failure mode.

@lestephane lestephane closed this Dec 15, 2021
@Bennett-Lynch
Copy link
Contributor

Hi @lestephane. Instead of trying to propagate an IOException, what do you think about just adding UncheckedIOException to the list of the SDK's default retryable exceptions?

@Bennett-Lynch
Copy link
Contributor

Actually, the default retry policy should retry on UncheckedIOExceptions because RetryOnExceptionsCondition checks the cause of a given exception by default, and UncheckedIOException always wraps an IOException, which is part of our default retryable exception list.

RetryOnExceptionsCondition.create(SdkDefaultRetrySetting.RETRYABLE_EXCEPTIONS),

Predicate<Class<? extends Exception>> hasRetrableCause =
ex -> exception.getCause() != null && ex.isAssignableFrom(exception.getCause().getClass());

aws-sdk-java-automation added a commit that referenced this pull request Feb 6, 2024
…c59dc86fd

Pull request: release <- staging/6f43c303-a28b-4966-b004-d38c59dc86fd
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.

2 participants