retry with request body and redirection #10
Conversation
openmohan
changed the title
| @@ -339,6 +339,9 @@ func (t *Transport) RoundTrip(req *http.Request) (*http.Response, error) { | |||
| for { | |||
| var cancel context.CancelFunc = func() {} // empty unless a timeout is set | |||
| reqWithTimeout := req | |||
| reqWithTimeout.GetBody = func() (io.ReadCloser, error) { | |||
| return req.Body, nil | |||
There was a problem hiding this comment.
I don't think this is correct. It works with the test because the body is not consumed in the redirects (only in the retries), but the Go documentation for the GetBody field mentions:
// GetBody defines an optional func to return a new copy of
// Body. It is used for client requests when a redirect requires
// reading the body more than once. Use of GetBody still
// requires setting Body.
Returning req.Body would not allow reading the body more than once. I think it should do similar to what is done on line 363 below when a retry is needed, reset the bytes.Reader to the beginning and create a new nop closer:
if _, err := br.Seek(0, 0); err != nil {
return nil, err
}
return ioutil.NopCloser(br), nil
There was a problem hiding this comment.
@mna In the (m *mockRoundTripper) RoundTrip we are consuming the body.
I think the issue itself is invalid.
- The redirect is on the
http.Clientlevel. Therehttpdoes not have to follow a redirect. - It is the responsibility of the request creator to set the
GetBody, although by default go provides support to common readers - And for every retry, we seek the cursor to start of the body.
There was a problem hiding this comment.
I think the issue itself is invalid.
You mean #9, i.e. there's nothing to fix in rehttp regarding GetBody ? If so I think you may be right. The reason the other Go package had to make changes regarding this was because AIUI it creates a new request, but rehttp does not create new requests, it uses the one provided by the caller. As you mention, it is the caller's responsibility to set GetBody, and for retries rehttp already handles that part.
And actually setting the GetBody field in rehttp would possibly overwrite whatever the caller did set, which would be wrong.
I'll link to this discussion in the issue #9 in case the person that created the issue has more input for this, but otherwise I think I'll just close it (and the PR) in a while if I don't hear back from them. Let me know if I misunderstood what you meant, though!
|
Closing as per the discussion. Thanks again @openmohan ! |
Fixes #9