Skip to content
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

Merged
merged 2 commits into from
Sep 1, 2022

Conversation

alexweav
Copy link
Contributor

@alexweav alexweav commented Aug 31, 2022

client.request() performs retries if the server responds with a non-200 status code. It takes an io.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

Copy link
Member

@julienduchesne julienduchesne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@julienduchesne julienduchesne merged commit 16d3180 into master Sep 1, 2022
@julienduchesne julienduchesne deleted the alexweav/retry-copy branch September 1, 2022 10:33
julienduchesne added a commit to grafana/terraform-provider-grafana that referenced this pull request Sep 1, 2022
To include grafana/grafana-api-golang-client#109
This should stop random 400s from happening on tests!
julienduchesne added a commit to grafana/terraform-provider-grafana that referenced this pull request Sep 1, 2022
To include grafana/grafana-api-golang-client#109
This should stop random 400s from happening on tests!
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
Archived in project
2 participants