Skip to content
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.

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

net/http: clarify documentation around when or if it is safe to reuse a request body #26409

Closed
twmb opened this issue Jul 16, 2018 · 4 comments
Closed
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@twmb
Copy link
Contributor

twmb commented Jul 16, 2018

The net/http library's Request documents that a client's Transport is responsible for calling a request body's Close method.

A response can be returned before a request's body is entirely written. I looked a bit and there appears to be no guarantee about a request body being closed at any time (even after a response body hits io.EOF and is closed).

To ensure that a request body is safe to reuse, we have to wrap it in a type that tracks an outstanding close.

However, if a body passed to NewRequest is one of a few types, bytes.Reader being one of those types, a request automatically has its GetBody field populated. On redirect, if any of the body has been read, the body is "cloned" and the original body is closed. If I want to wrap a bytes.Reader in this close counter, I lose the ability to have GetBody used on redirects. My alternative is to set GetBody myself. That makes this close counter a bit more difficult.

I know that GetBody is not called after we have a response since there will be no more redirects to follow. So, being careful, I can write a close counter that increments the number of expected closes if I set GetBody appropriately and it is called.

I currently have the following code:

type ReReqBody struct {
        buf []byte
        at  int 
        wg  *sync.WaitGroup
        ch  chan struct{}
}

func (r *ReReqBody) Read(b []byte) (int, error) {
        n := copy(b, r.buf[r.at:])
        if n == 0 { 
                return 0, io.EOF
        }
        r.at += n
        return n, nil 
}

func (r *ReReqBody) Close() error {
        r.wg.Done()
        return nil 
}

func (r *ReReqBody) Clone() *ReReqBody {
        r.wg.Add(1)
        return &ReReqBody{
                buf: r.buf,
                wg:  r.wg,
        }
}

func (r *ReReqBody) lazyInit() {
        if r.wg == nil {
                r.wg = new(sync.WaitGroup)
                r.ch = make(chan struct{}, 1)
        }
}

func (r *ReReqBody) Wait() <-chan struct{} {
        r.lazyInit()
        go func() {
                r.wg.Wait()
                r.ch <- struct{}{}
        }() 
        return r.ch
}

func (r *ReReqBody) Reset(buf []byte) {
        r.lazyInit()
        r.buf = buf 
        r.at = 0 
        r.wg.Add(1)
}

and it can be used somehow like the following:

<-rrb.Wait() // wait from a prior request
myBody = ... // modify an old request body
req := http.NewRequest("POST", "https://www.example.com", nil)
rrb.Reset(myBody)
req.Body = rrb
req.GetBody = func() (io.ReadCloser, error) { return rrb.Clone() }

I've gathered most of this by reading a bit of the net/http source, but it would be great to have documentation guidance for when exactly it is safe to reuse a request body and if it is safe at all to reuse a request body with GetBody.

@FiloSottile FiloSottile added this to the Go1.12 milestone Jul 16, 2018
@FiloSottile FiloSottile added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 16, 2018
@twmb
Copy link
Contributor Author

twmb commented Jul 16, 2018

(note that the code fails until #26408 is addressed)

@odeke-em
Copy link
Member

Tagging @broady @tombergan @bradfitz who might all be interested too, as per @broady's previously addressed issue #19653 that requested for:
a) Clarification on when a request's body can be reused
which was addressed with the documentation https://golang.org/pkg/net/http/#RoundTripper from CL 75671 aka commit 3039bff
bodyreuse

@twmb, @bradfitz talked about perhaps documenting that callers can wrap the Body themselves, as you have done to ensure body reading hits EOF before reuse, as you have done too, and he mentioned this in #19653 (comment) but the docs don't explicitly say this, but we allude to it in the docs "Callers should not modify the Request's body until the Response's Body has been closed". Perhaps we need to do more for GetBody clarification?

@twmb
Copy link
Contributor Author

twmb commented Jul 17, 2018

A bit of clarification around GetBody usage in combination with the existing "arrange to wait" paragraph would suffice here, I think.

@gopherbot
Copy link

Change https://golang.org/cl/124315 mentions this issue: net/http: fix double-close of req.Body

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants