httpbp: Fix Retries middleware #647
Conversation
fishy
requested review from
kylelemons,
konradreiche and
pacejackson
and removed request for
a team
httpbp/client_middlewares.go
Outdated
| if len(retryOptions) == 0 { | ||
| retryOptions = []retry.Option{retry.Attempts(1)} | ||
| } | ||
| return func(next http.RoundTripper) http.RoundTripper { | ||
| return roundTripperFunc(func(req *http.Request) (resp *http.Response, err error) { | ||
| if req.GetBody != nil { |
There was a problem hiding this comment.
if GetBody is nil for a non-get / bodyful request, should we just hand off to next and do a once-printed error that retries are disabled for this request?
There was a problem hiding this comment.
that in most cases would result in 4xx errors anyways, I don't think we should try to get too smart.
maybe after the retry attempt, check if it's 4xx and the method is non-get and GetBody is nil, print a log message saying this is failed likely because you didn't set GetBody, but doing anything beyond the logging would be "too smart" in my books :)
also since http.NewRequest(-WithContext) auto populates GetBody with *bytes.Buffer/*bytes.Reader/*strings.Reader which should already covers like 95% of the cases how people give the request a body, I won't worry too much about that.
There was a problem hiding this comment.
a few things:
- we shouldn't assume that sending a request without a body is even safe, let alone that we'll be able to distinguish the error the server will return.
- what if it's a DELETE /some/resource and the body specifies that only part of the resource was to be deleted, and an empty body deletes the whole thing?
- failing to have a GetBody can fail even before you have to do a retry, if the request redirects, so having the possibility of preventing more bugs seems like an added benefit of the log message
- there are tons of instances of &http.Request{ that are not calling NewRequest. Honestly I rarely call it, because I don't like handling the error.
I think the right thing to do is to skip the middleware (with a log message indicating why) if we're not going to be able to function properly, since we can't do the better thing and return an error immediately (since that would be a breaking change).
There was a problem hiding this comment.
I don't think there's anywhere in the RFC to say that a POST request must have a body. A server can be lenient on the method (want GET but don't complain if it's a POST instead) and the client can misuse POST instead of GET without a body. So being too smart can break RFC compliant code (although definitely not ideal code).
But I guess rejecting requests with non-nil Body and nil GetBody is probably safe enough.
There was a problem hiding this comment.
I struggle to draw the line here between useful defaults and not trying to be too clever.
At least I can see a few cases where POST and nil-body makes sense:
- REST endpoint to create an entity but the server side creates it with default attributes, no body needed
- Upload of an empty file
While the latter maybe should be an error I'd think it's not for the middleware to decide that.
But maybe Kyle knows of instances where this just leads to more headaches.
httpbp/client_middlewares.go
Outdated
| err = retrybp.Do(req.Context(), func() error { | ||
| // include ClientErrorWrapper to ensure retry is applied for | ||
| // some HTTP 5xx responses | ||
| resp, err = ClientErrorWrapper(limit)(next).RoundTrip(req) | ||
| resp, err = ClientErrorWrapper(maxErrorReadAhead)(next).RoundTrip(req) |
There was a problem hiding this comment.
also, shouldn't this be cloning the request? or does ClientErrorWrapper do that?
There was a problem hiding this comment.
is cloning the request required? the doccomment of http.Request.Clone doesn't say a request cannot be reused explicitly (the unit test also works, but maybe some more complicated features like trailer would fail).
There was a problem hiding this comment.
Looking over the fields of http.Request I can see there are a couple of fields including a channel for cancellation where it's unclear how it would behave if we don't always clone the request.
When we set GetBody in http.Request, it's expected that Body is also set, add special handling in Retries to make sure we also set Body when retrying when GetBody is also set before each retry attempt. Also always clone the request before each retry attempt to avoid some subtle errors, and skip the Retries middleware altogether if Body is set but GetBody is not.
|
💇 Fixed this for real. |
| func TestRetry(t *testing.T) { | ||
| t.Run("retry for timeout", func(t *testing.T) { |
There was a problem hiding this comment.
this subtest is removed because we don't really retry for timeouts (there's no timeout budget left for any retries), the expected attempt in this subtest is also 1.
| return roundTripperFunc(func(req *http.Request) (resp *http.Response, err error) { | ||
| if req.Body != nil && req.Body != http.NoBody && req.GetBody == nil { | ||
| slog.WarnContext( |
There was a problem hiding this comment.
🔕 do you want to this as a once or do it on a rate limit?
There was a problem hiding this comment.
this will only log once per request (it's not inside the retry loop), so if it's logged multiple times that means users have multiple requests constructed incorrectly, which I think it's warranted to log multiple times (for each incorrect request)
and we are using slog, so if users really want to rate-limit it/suppress it they can do so in their slog handler :)
wdyt?
When we set GetBody in http.Request, it's expected that Body is also
set, add special handling in Retries to make sure we also set Body when
retrying when GetBody is also set before each retry attempt.
Also always clone the request before each retry attempt to avoid some
subtle errors, and skip the Retries middleware altogether if Body is set
but GetBody is not.