Skip to content

Commit

Permalink
[internal-branch.go1.17-vendor] http2: avoid spurious RoundTrip error…
Browse files Browse the repository at this point in the history
… when server closes and resets stream

Avoid a race condition between RoundTrip and the read loop when the
read loop reads a response followed by an immediate stream reset.

For golang/go#49645
Fixes golang/go#49662

Change-Id: Ifb34e2dcb8cc443d3ff5d562cc032edf09da5307
Reviewed-on: https://go-review.googlesource.com/c/net/+/364834
Trust: Damien Neil <[email protected]>
Run-TryBot: Damien Neil <[email protected]>
Reviewed-by: Brad Fitzpatrick <[email protected]>
(cherry picked from commit 6a13c67c3ce400be1b91076053a994c2d1ebf01b)
Reviewed-on: https://go-review.googlesource.com/c/net/+/368379
Reviewed-by: Michael Knyszek <[email protected]>
  • Loading branch information
neild authored and mknyszek committed Dec 1, 2021
1 parent 2bb3bba commit 64ac61a
Showing 1 changed file with 39 additions and 26 deletions.
65 changes: 39 additions & 26 deletions transport.go
Original file line number Diff line number Diff line change
Expand Up @@ -1033,36 +1033,49 @@ func (cc *ClientConn) RoundTrip(req *http.Request) (*http.Response, error) {
}
}

handleResponseHeaders := func() (*http.Response, error) {
res := cs.res
if res.StatusCode > 299 {
// On error or status code 3xx, 4xx, 5xx, etc abort any
// ongoing write, assuming that the server doesn't care
// about our request body. If the server replied with 1xx or
// 2xx, however, then assume the server DOES potentially
// want our body (e.g. full-duplex streaming:
// golang.org/issue/13444). If it turns out the server
// doesn't, they'll RST_STREAM us soon enough. This is a
// heuristic to avoid adding knobs to Transport. Hopefully
// we can keep it.
cs.abortRequestBodyWrite()
}
res.Request = req
res.TLS = cc.tlsState
if res.Body == noBody && actualContentLength(req) == 0 {
// If there isn't a request or response body still being
// written, then wait for the stream to be closed before
// RoundTrip returns.
if err := waitDone(); err != nil {
return nil, err
}
}
return res, nil
}

for {
select {
case <-cs.respHeaderRecv:
res := cs.res
if res.StatusCode > 299 {
// On error or status code 3xx, 4xx, 5xx, etc abort any
// ongoing write, assuming that the server doesn't care
// about our request body. If the server replied with 1xx or
// 2xx, however, then assume the server DOES potentially
// want our body (e.g. full-duplex streaming:
// golang.org/issue/13444). If it turns out the server
// doesn't, they'll RST_STREAM us soon enough. This is a
// heuristic to avoid adding knobs to Transport. Hopefully
// we can keep it.
cs.abortRequestBodyWrite()
}
res.Request = req
res.TLS = cc.tlsState
if res.Body == noBody && actualContentLength(req) == 0 {
// If there isn't a request or response body still being
// written, then wait for the stream to be closed before
// RoundTrip returns.
if err := waitDone(); err != nil {
return nil, err
}
}
return res, nil
return handleResponseHeaders()
case <-cs.abort:
waitDone()
return nil, cs.abortErr
select {
case <-cs.respHeaderRecv:
// If both cs.respHeaderRecv and cs.abort are signaling,
// pick respHeaderRecv. The server probably wrote the
// response and immediately reset the stream.
// golang.org/issue/49645
return handleResponseHeaders()
default:
waitDone()
return nil, cs.abortErr
}
case <-ctx.Done():
err := ctx.Err()
cs.abortStream(err)
Expand Down

0 comments on commit 64ac61a

Please sign in to comment.