Skip to content
This repository has been archived by the owner on Jan 15, 2024. It is now read-only.

Fix retry mechanism not sending body if first request fails before reading the body #150

Merged
merged 8 commits into from
May 12, 2023

Conversation

inkel
Copy link
Contributor

@inkel inkel commented May 11, 2023

In #109 we added the capability to have a retry mechanism for internal server or too many requests errors. While the logic was good, it had a very hard to reproduce bug in that if the call to c.client.Do fails before reading the request body (e.g. a network connection issue) then all the subsequent requests will be sent with an empty body because bodyBuffer never gets populated.

This PR forces to always create a slice of bytes holding the request body, passing a new bytes.Buffer to c.client.Do.

inkel added 3 commits May 11, 2023 16:22
Signed-off-by: Leandro López (inkel) <leandro.lopez@grafana.com>
Even if successful by always reading the body we can make certain that
the body will be sent to the server, even if the request fails before
reading the body. See parent commit for further reference.

Signed-off-by: Leandro López (inkel) <leandro.lopez@grafana.com>
Signed-off-by: Leandro López (inkel) <leandro.lopez@grafana.com>
@inkel inkel self-assigned this May 11, 2023
@inkel inkel requested a review from a team May 11, 2023 19:30
@inkel
Copy link
Contributor Author

inkel commented May 11, 2023

I'm creating this as a draft for now because I'm planning on changing *Client.request signature to accept a body []byte instead of body io.Reader. I did a quick search and it seems all the places we are passing a body are actually calling bytes.NewReader(data) before passing the argument.

inkel added 3 commits May 11, 2023 16:47
Signed-off-by: Leandro López (inkel) <leandro.lopez@grafana.com>
Signed-off-by: Leandro López (inkel) <leandro.lopez@grafana.com>
We are declaring Go 1.14 for this library, so the new io.ReadAll
function doesn't exist (yet).

Signed-off-by: Leandro López (inkel) <leandro.lopez@grafana.com>
@inkel inkel marked this pull request as ready for review May 12, 2023 12:54
@inkel
Copy link
Contributor Author

inkel commented May 12, 2023

Oh come on! I did run goimports on these files, but it seems that because I'm running a newer version of Go in my system is complaining about these. I'm going to try and fix them.

Never versions of golangci-lint will complain about this and we'll
need to revert this commit.

Signed-off-by: Leandro López (inkel) <leandro.lopez@grafana.com>
client_test.go Outdated Show resolved Hide resolved
Co-authored-by: Julien Duchesne <julien.duchesne@grafana.com>
@inkel inkel merged commit e5fb6a3 into master May 12, 2023
2 checks passed
@inkel inkel deleted the inkel/fix-retry-missing-body branch May 12, 2023 17:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants