mark recoverability of DNS errors correctly #1088
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This changes HTTP client to not retry on certain DNS errors. From the [list of possible DNS errors](https://github.com/JuliaLang/julia/blob/ec8df3da3597d0acd503ff85ac84a5f8f73f625b/stdlib/Sockets/src/addrinfo.jl#L108-L112) only EAI_AGAIN may be recoverable. This also changes the error code comparison to use `UV_EAI_AGAIN`, because that is what a [DNSError instance would contain](https://github.com/JuliaLang/julia/blob/ec8df3da3597d0acd503ff85ac84a5f8f73f625b/stdlib/Sockets/src/addrinfo.jl#L108-L112).
Codecov Report
@@ Coverage Diff @@
## master #1088 +/- ##
==========================================
- Coverage 82.33% 82.32% -0.02%
==========================================
Files 32 32
Lines 3040 3043 +3
==========================================
+ Hits 2503 2505 +2
- Misses 537 538 +1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
quinnj
approved these changes
Aug 2, 2023
|
can we add a test please? |
|
We had faced this in a likely stressed condition in our application. I was testing this with a modified julia binary to artificially throw the DNS error. I found it hard to replicate the error otherwise. I could add a unit test for |
|
@nickrobinson251 Actually I think I can add some relevant tests. And I also missed out some more changes to |
Thanks!
yeah, i think unit tests would be helpful |
tanmaykm
changed the title
tanmaykm
added a commit
that referenced
this pull request
Aug 2, 2023
Added some tests for `isrecoverable`, particularly for DNS errors. Added a test for one of the new behaviors, i.e. EAI_NONAME is not retried. ref: #1088
tanmaykm
added a commit
that referenced
this pull request
Aug 2, 2023
Added some tests for `isrecoverable`, particularly for DNS errors. Added a test for one of the new behaviors, i.e. EAI_NONAME is not retried. If EAI_EAGAIN is thrown requests are retried. ref: #1088
quinnj
pushed a commit
that referenced
this pull request
Aug 3, 2023
Added some tests for `isrecoverable`, particularly for DNS errors. Added a test for one of the new behaviors, i.e. EAI_NONAME is not retried. If EAI_EAGAIN is thrown requests are retried. ref: #1088
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This changes HTTP client to not retry on certain DNS errors. From the list of possible DNS errors only EAI_AGAIN may be recoverable. This also changes the error code comparison to use
UV_EAI_AGAIN, because that is what a DNSError instance would contain.fixes: #1085