Fix requests with body #2
Conversation
The body is needed for both requests, so we need to copy it, otherwise the second request will fail with a zero length body.
|
@klauspost I've forked and added your change |
|
@edaniels something still doesn't work http: ContentLength=1170 with Body length 0 |
|
Hey @AlexYaroshenko, I didn't create this PR but I could try to help. Could you provide a more specific example that can be run? |
|
Works for me with: reader := bytes.NewReader(changes)
req, err := http.NewRequest(http.MethodPut, a.Host+"/articles/", reader) |
| } | ||
| req.Body = ioutil.NopCloser(bytes.NewBuffer(body)) | ||
| req2.Body = ioutil.NopCloser(bytes.NewBuffer(body)) | ||
| } |
There was a problem hiding this comment.
Reading the body into RAM isn't great if the payload is large. If req2.GetBody is non-nil we should use it to set req2.Body.
There was a problem hiding this comment.
req2 is a shallow copy of req, so they both refer to the same reader.
There was a problem hiding this comment.
Yes, and that's the issue being solved in this PR, with the drawback that the body is buffered in RAM. By calling http.Request.GetBody on the request we can get a fresh io.Reader for the second request and not have to copy the data.
There was a problem hiding this comment.
Ah, that is some new fancy tech from Go 1.8 :D
You could also check if the reader is seekable... but this PR is dead anyway. Just leaving it here for others, not using it any more.
There was a problem hiding this comment.
Wow what a coincidence that this was discussed/revived in the last 24 hours. I spent 30 minutes debugging this problem and then an hour or two of research to find that this library had the bug of not cloning the Body. Most of the other clients I have seen do read the body into memory to clone it and none of them have been using GetBody. Any movement on this merge request? I've since switched to another library that is copying:
https://github.com/gabstv/httpdigest/blob/master/transport.go#L105
There was a problem hiding this comment.
Oh dear, another digest package. I thought I'd found all of them yesterday. I'd say the chances of this PR getting merged are close to zero, but perhaps one of the other similar packages takes PRs. I've been using github.com/toaster/digest myself. I'll probably send a PR to it for adding support for http.Request.GetBody.
|
I've had it forked here for a while with no issues: https://github.com/mongodb-forks/digest. The duplicate body isn't a concern in my case since we limit the amount of bytes read/written initially. Happy to integrate a version using |
|
Thanks @edaniels, I've sent mongodb-forks#1 now. |
|
@bobziuchkovski bump |
The body is needed for both requests, so we need to copy it, otherwise the second request will fail with a zero length body.