Aws-sdk-java: RetryPolicy misses Exceptions

[rdifalco]

The catch in AmazonHttpClient will only retry IOExceptions. However, there are places where non-IOExceptions should be retries. Consider this code:

https://github.com/aws/aws-sdk-java/blob/2ad82e2478e5c1f1f01646a8ba60a7777b9fa3e3/aws-java-sdk-core/src/main/java/com/amazonaws/http/AmazonHttpClient.java#L1514

These are often transient issues but because it is wrapped in an SdkClientException and you only retry on IOExceptions below there is no way to create a RetryPolicy that handles these issues. So a user must resort to writing a retry handler AROUND the actual Amazon Client call, which will litter a lot of code. A preferable approach would be to pass those transient XmlStreamExceptions to the configured RetryPolicy.

https://github.com/aws/aws-sdk-java/blob/2ad82e2478e5c1f1f01646a8ba60a7777b9fa3e3/aws-java-sdk-core/src/main/java/com/amazonaws/http/AmazonHttpClient.java#L1035

https://github.com/aws/aws-sdk-java/issues/892

[dagnir]

Hi @rdifalco, as noted in #892, it's not safe to apply the retry policy for the case of exceptions thrown after the response has already been received from the service it's not possible to know if the operation is idempotent.

I'm inclined to think that widening the scope of exceptions passed to retry policies could break current customers who might start retrying on exceptions they weren't before.

[rdifalco]

I mean it seems like a bug to me. For example, you might get that exception because of socket read timeout or any number of other reasons that are retryable. For anyone who wants to retry those they end up having to manage retry logic in two different areas. Can you come up with a use case you think is not safe? For the built in RetryPolicies you can easily skip these exceptions so it would not impact existing customers who expect those errors not to be retried.

But even for operations that are not idempotent the user is in an indeterminate state when you get this error, right? And they would have to include this error in their RetryCondition for it to break anything they do now.

[rdifalco]

At the least you might want to think about modifying your retry2 package so that the user has the chance of retrying on these. If you consider real life scenarios, there's rarely a time that you don't want to retry on this kind of error.

[dagnir]

Okay I went back for another look through the http client code and I think you're actually right, Adding a clause to catch AmazonClientException might work. I'll need to double check with others on the team about this. Will update with what I learn.

[dagnir]

Hey @rdifalco , we made a change so that XMLStreamExceptions caused by IOE's are now wrapped in IOE and exposed to retry policies. Closing this issue.

[dagnir]

Forgot to add, the fix will be available in our upcoming release later today.

[dagnir]

The fix is available in 1.11.106

[sihil]

@rdifalco @dagnir 👏 🎉 you are my heroes.