Skip to content

Fix requests with body #2

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
klauspost wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix requests with body #2

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions digest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,13 @@
package digest

import (
"bytes"
"crypto/md5"
"crypto/rand"
"errors"
"fmt"
"io"
"io/ioutil"
"net/http"
"strings"
)
Expand Down Expand Up @@ -240,6 +242,15 @@ func (t *Transport) RoundTrip(req *http.Request) (*http.Response, error) {
req2.Header[k] = s
}

// Copy body - we need it twice.
if req.Body != nil {
body, err := ioutil.ReadAll(req.Body)
if err != nil {
return nil, err
}
req.Body = ioutil.NopCloser(bytes.NewBuffer(body))
req2.Body = ioutil.NopCloser(bytes.NewBuffer(body))
}
Copy link
Copy Markdown

@magnusbaeck magnusbaeck May 28, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Author

@klauspost klauspost May 28, 2020

Choose a reason for hiding this comment

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

req2 is a shallow copy of req, so they both refer to the same reader.

Copy link
Copy Markdown

@magnusbaeck magnusbaeck May 28, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Author

@klauspost klauspost May 28, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown

@justinfx justinfx May 29, 2020

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

@magnusbaeck magnusbaeck May 29, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown

@AlexYaroshenko AlexYaroshenko May 29, 2020

Choose a reason for hiding this comment

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

@bobziuchkovski

// Make a request to get the 401 that contains the challenge.
resp, err := t.Transport.RoundTrip(req)
if err != nil || resp.StatusCode != 401 {
Expand Down