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 whether Requests can be reused #19653

Closed
broady opened this issue Mar 22, 2017 · 8 comments

Comments

Projects
None yet
4 participants
@broady
Copy link
Member

commented Mar 22, 2017

Re-using requests is currently unreliable.

If that's intended, then I think it should be documented. If not intended, then I can provide steps to reproduce.

@bradfitz bradfitz added this to the Go1.9 milestone Mar 22, 2017

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jun 13, 2017

Sorry, just getting to this.

Can you clarify what you mean by unreliable?

@broady

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2017

Yep, really wish I'd put the steps to reproduce in the original report.

I think it was code like this:

req, _ := http.NewRequest("GET", "https://google.com", nil)
for i := 0; i < 10; i++ {
  go func() {
    for {
      resp, err := http.DefaultClient.Do(req)
      // ...
    }
  }()
}

(or maybe even without any concurrency)

It's unclear that state is held in the request struct and that a Request shouldn't be re-used.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jun 13, 2017

There's no state in the Request struct. There's only one unexported field and it's just the context, which we don't want people to mutate.

The only thing I can think of is the old pre-cancel/pre-context Transport.reqCanceler map that keys by *Request, but that should be harmless.

I don't see why Requests couldn't be used concurrently.

Could you try to whip up a repro?

@broady

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2017

Tried, couldn't repro.

Code: https://github.com/broady/upwatch/blob/master/http.go#L86

Changes:

diff --git a/http.go b/http.go
index 99f775f..928b03d 100644
--- a/http.go
+++ b/http.go
@@ -83,7 +83,8 @@ func boom(w http.ResponseWriter, r *http.Request) {
                return
        }
        log.Printf("GET %q", url)
-       if _, err := http.NewRequest("GET", url, nil); err != nil {
+       req, err := http.NewRequest("GET", url, nil)
+       if err != nil {
                http.Error(w, "Invalid URL", http.StatusBadRequest)
                return
        }
@@ -116,7 +117,8 @@ func boom(w http.ResponseWriter, r *http.Request) {
                <-sem
                go func() {
                        start := time.Now()
-                       resp, err := http.Get(url)
+                       resp, err := http.DefaultClient.Do(req)
                        if err != nil {
                                r := result{err: err, duration: time.Now().Sub(start)}
                                sem <- true
@gopherbot

This comment has been minimized.

Copy link

commented Nov 2, 2017

Change https://golang.org/cl/75530 mentions this issue: http2: fix transport data race on reused *http.Request objects

gopherbot pushed a commit to golang/net that referenced this issue Nov 2, 2017

http2: fix transport data race on reused *http.Request objects
Based on golang/go#19653, it should be possible to reuse an http.Request
object after the outstanding request has completed. This CL fixes a race
in the http/2 library that occurs when a caller tries to reuse an
http.Request just after the request completed.

The new test failed with -race before this CL and passes after this CL.
Verified with -count 10000.

Updates golang/go#21316

Change-Id: I014cf9cefd0dd21f6f41763ba554d23ddc7fca40
Reviewed-on: https://go-review.googlesource.com/75530
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@tombergan

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2017

@bradfitz I think this is actually more subtle because of HTTP/2. Suppose a client does:

req, _ := http.NewRequest("GET", url, nil)
resp, _ := http.DefaultTransport.Roundtrip(req)
resp.Body.Close()

It should be fine for the caller to reuse req after resp.Body.Close() because the request is complete. But, if instead, the request is a POST with a non-empty body:

req, _ := http.NewRequest("POST", url, body)
resp, _ := http.DefaultTransport.Roundtrip(req)
resp.Body.Close()

We cannot guarantee that req can be reused after resp.Body.Close() because the transport might not have finished uploading the request body. This problem applies to both HTTP/1 and HTTP/2, although in HTTP/1, the semantics are a bit murkier while HTTP/2 is very clear that streams are fully bidirectional and each end can be half-closed independently of the other end. Additionally, the example from this comment does not work when req contains a non-nil Body, since two RoundTrip calls cannot read from the body concurrently.

So, I think this may deserve clarification, in two respects:

  1. It is illegal to pass the same req to two concurrent RoundTrip calls if req has a non-nil Body.

  2. If req has a non-nil Body, it's not clear if it's safe to reuse req in a sequence of non-concurrent RoundTrip calls. This is because we have no way to signal when the first RoundTrip call is done reading req.Body.

The simplest resolution is to say: A Request cannot be reused if it has a non-nil Body. If the Request has a nil Body, then the Request should not be modified until either RoundTrip returns an error or Respond.Body.Close is called. (This covers both of the above corner cases.)

An even simpler resolution is to forbid reusing Requests in any cases, but I suspect this would break callers.

WDYT?

@tombergan tombergan reopened this Nov 2, 2017

@tombergan tombergan modified the milestones: Go1.9, Go1.10 Nov 2, 2017

@bradfitz bradfitz assigned tombergan and unassigned bradfitz Nov 2, 2017

@bradfitz

This comment has been minimized.

Copy link
Member

commented Nov 2, 2017

Or we just say it's only safe to reuse a Request if Body is nil or the the Request.Body has been read to EOF. (the caller could wrap the Body in their own wrapper which notes EOF if they wanted).

@gopherbot

This comment has been minimized.

Copy link

commented Nov 2, 2017

Change https://golang.org/cl/75671 mentions this issue: net/http: clarify when it is safe to reuse a request

@gopherbot gopherbot closed this in 3039bff Nov 2, 2017

c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018

http2: fix transport data race on reused *http.Request objects
Based on golang/go#19653, it should be possible to reuse an http.Request
object after the outstanding request has completed. This CL fixes a race
in the http/2 library that occurs when a caller tries to reuse an
http.Request just after the request completed.

The new test failed with -race before this CL and passes after this CL.
Verified with -count 10000.

Updates golang/go#21316

Change-Id: I014cf9cefd0dd21f6f41763ba554d23ddc7fca40
Reviewed-on: https://go-review.googlesource.com/75530
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

brandur-stripe pushed a commit to stripe/stripe-go that referenced this issue Aug 4, 2018

Set request body before every retry
This patch changes the interfaces of `NewRequest` and `Do` around a
little so that we can set a new request body with every request.

In the era of HTTP (1), it was safe to reuse a `Request` object, but
with the addition of HTTP/2, it's now only sometimes safe. Reusing a
`Request` with a body will break.

See some more information here:

golang/go#19653 (comment)

Fixes #642.

brandur-stripe pushed a commit to stripe/stripe-go that referenced this issue Aug 4, 2018

Set request body before every retry
This patch changes the interfaces of `NewRequest` and `Do` around a
little so that we can set a new request body with every request.

In the era of HTTP (1), it was safe to reuse a `Request` object, but
with the addition of HTTP/2, it's now only sometimes safe. Reusing a
`Request` with a body will break.

See some more information here:

golang/go#19653 (comment)

Fixes #642.

brandur-stripe pushed a commit to stripe/stripe-go that referenced this issue Aug 4, 2018

Set request body before every retry
This patch changes the interfaces of `NewRequest` and `Do` around a
little so that we can set a new request body with every request.

In the era of HTTP (1), it was safe to reuse a `Request` object, but
with the addition of HTTP/2, it's now only sometimes safe. Reusing a
`Request` with a body will break.

See some more information here:

golang/go#19653 (comment)

Fixes #642.

brandur-stripe pushed a commit to stripe/stripe-go that referenced this issue Aug 4, 2018

Set request body before every retry
This patch changes the interfaces of `NewRequest` and `Do` around a
little so that we can set a new request body with every request.

In the era of HTTP (1), it was safe to reuse a `Request` object, but
with the addition of HTTP/2, it's now only sometimes safe. Reusing a
`Request` with a body will break.

See some more information here:

golang/go#19653 (comment)

Fixes #642.

@golang golang locked and limited conversation to collaborators Nov 2, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.