fix: treat IO exceptions as retryable in HTTP engines #979
Conversation
| @@ -208,10 +208,9 @@ internal inline fun<T> mapOkHttpExceptions(block: () -> T): T = | |||
| try { | |||
| block() | |||
| } catch (ex: IOException) { | |||
| throw HttpException(ex, ex.errCode(), ex.isRetryable()) | |||
| throw HttpException(ex, ex.errCode(), retryable = true) // All IOExceptions are retryable | |||
There was a problem hiding this comment.
Question/Correctness: Is there any reason we wouldn't just make all IOExceptions retryable at the retry policy level?
There was a problem hiding this comment.
A few reasons came to mind:
IOExceptionisn't in Kotlin stdlib—it's a JVM exception. Our retry policy is currently in common code. We could introduce anexpect/actualfunction to dispatch down for platform-specific exceptions but it doesn't feel warranted for this use case.IOExceptionshouldn't be thrown by the CRT engine. If it is, then something is wrong that we likely don't understand and retrying may not be the best course of action.- Retry policies apply across the entirety of a request, not merely the HTTP call invocation.
IOExceptions may occur during local IO errors such as streaming from a file, reading system properties, string encoding problems, etc. It seemed wise to scope theseIOExceptionretries to just the HTTP call itself.
There was a problem hiding this comment.
IOException isn't in Kotlin stdlib—it's a JVM exception. Our retry policy is currently in common code. We could introduce an expect/actual function to dispatch down for platform-specific exceptions but it doesn't feel warranted for this use case.
We already have an expect/actual for it FYI: https://github.com/awslabs/smithy-kotlin/blob/main/runtime/runtime-core/common/src/aws/smithy/kotlin/runtime/io/Exceptions.kt#L8
IOException shouldn't be thrown by the CRT engine. If it is, then something is wrong that we likely don't understand and retrying may not be the best course of action.
It entirely depends on how we wrap the exceptions. To your point though CRT JVM bindings throw generic exceptions with a CRT specific error code IIRC. I wasn't thinking of CRT though I was thinking of other JVM specific bindings that customers may writer or that we may provide in the future. Doing it at the retry policy level would capture all of them.
Retry policies apply across the entirety of a request, not merely the HTTP call invocation. IOExceptions may occur during local IO errors such as streaming from a file, reading system properties, string encoding problems, etc. It seemed wise to scope these IOException retries to just the HTTP call itself.
I think we are wrapping all of our HTTP related exceptions into an HttpException with an error code if we can map it. I suppose because of this handling it at the specific engine level makes sense. We just have to remember to do it for each engine.
suggestion : If we are handling this at the specific engine level I think we ought to probably make the CRT exceptions retryable as well if we can in this PR. It would require looking at specific error codes probably.
ianbotsf
changed the title
| { | ||
| "id": "0645c929-bdfe-4d07-a131-b0b447422fff", | ||
| "type": "bugfix", | ||
| "description": "Treat all `IOException` errors in OkHttp engine as retryable (e.g., socket errors, DNS lookup failures, etc.)" |
There was a problem hiding this comment.
and now also the CRT engine! 🚀
|
Kudos, SonarCloud Quality Gate passed!
|
Issue #
(none)
Description of changes
This change enables retrying on:
IOExceptionerrors in the default OkHttp engineThis will enable transient problems like socket errors, DNS lookup failues, etc. to be retried the same as other retryable errors.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.