This repository has been archived by the owner on Jan 15, 2024. It is now read-only.
Fix retry logic sending empty request bodies on subsequent retries #109
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.
client.request()
performs retries if the server responds with a non-200 status code. It takes anio.Reader
for the request body to send. Since a reader is a stream, it can only be consumed once, and it becomes fully exhausted after the first request in the retry loop. Therefore, the following retries will always be sent with an empty request body, rather than the intended one.In order to retry a request at this layer, we must stash it in memory. If the
io.Reader
truly cannot be consumed twice, it's impossible to retry the request in the first place.This PR makes an in-memory copy of the request body which is then sent in each retry attempt.
I manually checked every use case of
c.request
in the client - every single caller is sending a byte slice stored in memory to begin with. Therefore, this PR does not run the risk of bringing a massive request into memory, as we'd already be doing it anyway in the current implementation. We also only perform the copy if the request both has a body, and opted into retries.An alternative approach would be to make
c.request()
take a byte slice straight up, since every caller is using it that way anyway. I can go down this route if you would prefer. I originally avoided it in order to touch a minimum amount of code.Discovered here: grafana/terraform-provider-grafana#630