Fix POST retries #45
Merged
Fix POST retries #45
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
* Verify that call will return error when Context-cancelled during backoff * Verify that error is due to Context-cancellation
* Context cancellation would result in no result being sent, thus causing the operation to hang in many circumstances
* Add test for POST-requests provided by Do() with a body * Add test for POST-requests provided by Post() with a body * Add middlewareServer to support POST tests
* Provide each concurrent request with its own Request so they do not interfere with each other. Eg reading/closing each others bodies. * Reset body before retrying when requests have bodies
sethgrid
approved these changes
Feb 9, 2022
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.
PR fixes a couple of bugs that we discovered when investigating a situation where a failed POST request was causing a job to time out:
One-line fix for a Context-cancellation issue that can cause indefinite hangs when the Context expires during a backoff wait period (a bug I introduced in Listen for context cancellation during backoff period #44 😬 )
Larger fix around request body handling. The general gist of it is that currently when pester retries requests with request bodies, it will either return an
errorcomplaining aboutContentLength=<some number> with Body length 0or send the destination server a request with an empty body, depending on the type ofio.Readerused and how you made the request. If you checkout c1c03d2 the added tests will demonstrate this behaviour. This issue badly exacerbates the first one since it means that any request with a body will never succeed on retries, and thus any request with a non-empty body that doesn't succeed the first time will result in an indefinite hang unless it happens to reachMaxAttemptsfirst or be Context-cancelled while making the request.