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.

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

Jump to bottom

x/net/http2: backport critical fixes [1.16 backport] #49076

Closed
neild opened this issue Oct 19, 2021 · 33 comments
Closed

x/net/http2: backport critical fixes [1.16 backport] #49076

neild opened this issue Oct 19, 2021 · 33 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Milestone
Go1.16.10

Comments

@neild
Copy link
Contributor

neild commented Oct 19, 2021

A number of fixes in x/net/http2 haven't been backported to 1.16. This is an umbrella issue to figure out which changes should be backported and doing it.
@neild neild added the CherryPickCandidate Used during the release process for point releases label Oct 19, 2021
@neild neild self-assigned this Oct 19, 2021
@gopherbot gopherbot added this to the Go1.16.10 milestone Oct 19, 2021
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/356980 mentions this issue: [internal-branch.go1.16-vendor] http2: close request body after early RoundTrip failures

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/356972 mentions this issue: [internal-branch.go1.16-vendor] http2: use (*tls.Dialer).DialContext in dialTLS

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/356979 mentions this issue: [internal-branch.go1.16-vendor] http2: limit client initial MAX_CONCURRENT_STREAMS

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/356988 mentions this issue: [internal-branch.go1.16-vendor] http2: refactor request write flow

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/356986 mentions this issue: [internal-branch.go1.16-vendor] http2: fix Transport connection pool TOCTOU max concurrent stream bug

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/356976 mentions this issue: [internal-branch.go1.16-vendor] http2: close the request body if needed

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/357093 mentions this issue: [internal-branch.go1.16-vendor] http2: don't rely on system TCP buffer sizes in TestServer_MaxQueuedControlFrames

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/357098 mentions this issue: [internal-branch.go1.16-vendor] http2: don't abort half-closed streams on server connection close

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/356975 mentions this issue: [internal-branch.go1.16-vendor] http2: reduce frameScratchBuffer caching aggressiveness

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/357089 mentions this issue: [internal-branch.go1.16-vendor] http2: avoid extra GetConn trace call

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/356973 mentions this issue: [internal-branch.go1.16-vendor] http2: switch to ASCII equivalents of string functions

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/357097 mentions this issue: [internal-branch.go1.16-vendor] http2: on write errors, close ClientConn before returning from RoundTrip

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/357095 mentions this issue: [internal-branch.go1.16-vendor] http2: close the Request's Body when aborting a stream

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/356971 mentions this issue: [internal-branch.go1.16-vendor] http2: discard DATA frames with higher stream IDs during graceful shutdown

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/357096 mentions this issue: [internal-branch.go1.16-vendor] http2: deflake TestTransportReqBodyAfterResponse_200

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/356970 mentions this issue: [internal-branch.go1.16-vendor] http2: rework Ping test to rely less on timing

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/356982 mentions this issue: [internal-branch.go1.16-vendor] http2: avoid blocking while holding ClientConn.mu

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/357090 mentions this issue: [internal-branch.go1.16-vendor] http2: avoid clientConnPool panic when NewClientConn fails

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/356978 mentions this issue: [internal-branch.go1.16-vendor] http2: make Transport not reuse conns after a stream protocol error

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/356974 mentions this issue: [internal-branch.go1.16-vendor] http2: also set "http/1.1" ALPN in ConfigureServer

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/357094 mentions this issue: [internal-branch.go1.16-vendor] http2: return unexpected eof on empty response with non-zero content length

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/356983 mentions this issue: [internal-branch.go1.16-vendor] http: fix race in DATA frame padding refund

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/357091 mentions this issue: [internal-branch.go1.16-vendor] http2: avoid race in TestTransportReqBodyAfterResponse_403.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/356985 mentions this issue: [internal-branch.go1.16-vendor] http2: shut down idle Transport connections after protocol errors

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/356977 mentions this issue: [internal-branch.go1.16-vendor] http2: accept zero-length block fragments in HEADERS frames

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/356981 mentions this issue: [internal-branch.go1.16-vendor] http2: fix off-by-one error in client check for max concurrent streams

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/357092 mentions this issue: [internal-branch.go1.16-vendor] http2: detect write-blocked PING frames

gopherbot pushed a commit to golang/net that referenced this issue Oct 29, 2021
@fraenkel @dmitshur
[internal-branch.go1.16-vendor] http2: close the request body if needed
0e5043f 
As per client.Do and Request.Body, the transport is responsible to close
the request Body.
If there was an error or non 1xx/2xx status code, the transport will
wait for the body writer to complete. If there is no data available to
read, the body writer will block indefinitely. To prevent this, the body
will be closed if it hasn't already.
If there was a 1xx/2xx status code, the body will be closed eventually.

Updates golang/go#49076

Change-Id: I9a4a5f13658122c562baf915e2c0c8992a023278
Reviewed-on: https://go-review.googlesource.com/c/net/+/323689
Reviewed-by: Damien Neil <dneil@google.com>
Trust: Damien Neil <dneil@google.com>
Trust: Alexander Rakoczy <alex@golang.org>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/net/+/356976
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit to golang/net that referenced this issue Oct 29, 2021
@neild @dmitshur
[internal-branch.go1.16-vendor] http2: accept zero-length block fragm…
0fe5f8a 
…ents in HEADERS frames

Fix a fencepost error which rejects HEADERS frames with neither
payload nor padding, but accepts ones with padding and no payload.

Updates golang/go#49076

Change-Id: Ic6ed65571366e86980a5ec69e7f9e6ab958f3b07
Reviewed-on: https://go-review.googlesource.com/c/net/+/344029
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/net/+/356977
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit to golang/net that referenced this issue Oct 29, 2021
@bradfitz @dmitshur
[internal-branch.go1.16-vendor] http2: make Transport not reuse conns…
524fcad 
… after a stream protocol error

If a server sends a stream error of type "protocol error" to a client,
that's the server saying "you're speaking http2 wrong". At that point,
regardless of whether we're in the right or not (that is, regardless of
whether the Transport is bug-free), clearly there's some confusion and
one of the two parties is either wrong or confused. There's no point
pushing on and trying to use the connection and potentially exacerbating
the confusion (as we saw in golang/go#47635).

Instead, make the client "poison" the connection by setting a new "do
not reuse" bit on it. Existing streams can finish up but new requests
won't pick that connection.

Also, make those requests as retryable without the caller getting an
error.

Given that golang/go#42777 existed, there are HTTP/2 servers in the
wild that incorrectly set RST_STREAM PROTOCOL_ERROR codes. But even
once those go away, this is still a reasonable fix for preventing
a broken connection from being stuck in the connection pool that fails
all future requests if a similar bug happens in another HTTP/2 server.

Updates golang/go#49076

Change-Id: I3f89ecd1d3710e49f7219ccb846e016eb269515b
Reviewed-on: https://go-review.googlesource.com/c/net/+/347033
Trust: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-on: https://go-review.googlesource.com/c/net/+/356978
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit to golang/net that referenced this issue Oct 29, 2021
@stevenh @dmitshur
[internal-branch.go1.16-vendor] http2: limit client initial MAX_CONCU…
59c0c25 
…RRENT_STREAMS

Prevent the client trying to establish more streams than the server is willing to accept during the initial life time of a connection by limiting `maxConcurrentStreams` to `100`, the http2 specifications
recommended minimum, until we've received the initial `SETTINGS` frame from the server.

After a `SETTINGS` frame has been received use the servers `MAX_CONCURRENT_STREAMS`, if present, otherwise use `1000` as a reasonable value.

For normal consumers this will have very little impact, allowing a decent level of concurrency from the start, and for highly concurrent consumers or large bursts it will prevent significant number of rejected streams being attempted hence actually increasing performance.

Updates golang/go#49076

Change-Id: I35fecd501ca39cd059c7afd1d44090b023f16e1e
GitHub-Last-Rev: 0d1114d
GitHub-Pull-Request: #73
Reviewed-on: https://go-review.googlesource.com/c/net/+/236497
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Trust: Brad Fitzpatrick <bradfitz@golang.org>
Trust: Joe Tsai <joetsai@digital-static.net>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/net/+/356979
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit to golang/net that referenced this issue Oct 29, 2021
@neild @dmitshur
[internal-branch.go1.16-vendor] http2: close request body after early…
828651b 
… RoundTrip failures

The RoundTrip contract requires that the request Body be closed,
even when an error occurs sending the request.

Fix several cases where the body was not closed by hoisting the
Close call to Transport.RoundTripOpt. Now ClientConn.roundTrip
takes responsibility for closing the body once the body write
begins; otherwise, the caller does so.

Fix the case where a new body is acquired via Request.GetBody
to close the previous body, matching net/http's behavior.

Updates golang/go#49076

Change-Id: Id9dc682d4d86a1c255c7c0d864208ff76ed53eb2
Reviewed-on: https://go-review.googlesource.com/c/net/+/349489
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/net/+/356980
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit to golang/net that referenced this issue Oct 29, 2021
@neild @dmitshur
[internal-branch.go1.16-vendor] http2: fix off-by-one error in client…
78e8d65 
… check for max concurrent streams

Updates golang/go#49076

Change-Id: Ib4eb93702b32ae7d03cad17ca0b997d5e6a58ad7
Reviewed-on: https://go-review.googlesource.com/c/net/+/349490
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/net/+/356981
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit to golang/net that referenced this issue Oct 29, 2021
@neild @dmitshur
[internal-branch.go1.16-vendor] http2: avoid blocking while holding C…
d7eefc9 
…lientConn.mu

Operations which examine the state of a ClientConn--notably,
the connection pool's check to see if a conn is available to
take a new request--need to acquire mu. Blocking while holding mu,
such as when writing to the network, blocks these operations.

Remove blocking operations from the mutex.
Perform network writes with only ClientConn.wmu held.
Clarify that wmu guards the per-conn HPACK encoder and buffer.

Add a new mutex guarding request creation, covering the critical
section starting with allocating a new stream ID and continuing
until the stream is created.

Fix a locking issue where trailers were written from the HPACK
buffer with only wmu held, but headers were encoded into the buffer
with only mu held. (Now both encoding and writes occur with wmu
held.)

Updates golang/go#49076

Change-Id: Ibb313424ed2f32c1aeac4645b76aedf227b597a3
Reviewed-on: https://go-review.googlesource.com/c/net/+/349594
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/net/+/356982
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit to golang/net that referenced this issue Oct 29, 2021
@neild @dmitshur
[internal-branch.go1.16-vendor] http2: fix race in DATA frame padding…
2df4c53 
… refund

Move flow adjustment back under cc.mu.

Updates golang/go#49076

Change-Id: Idb762091cfeb55c18bc74389e62193f81438624f
Reviewed-on: https://go-review.googlesource.com/c/net/+/351950
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/net/+/356983
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit to golang/net that referenced this issue Oct 29, 2021
@neild @dmitshur
[internal-branch.go1.16-vendor] http2: remove check for read-after-cl…
3741e47 
…ose of request bodies

Aborting a request currently races with writes of the request
body, so abortRequestBodyWrite can close the body before writeRequestBody
reads from it.

Updates golang/go#49076

Change-Id: I5362283f4066611aeecbc48b400d79cfa0b4b284
Reviewed-on: https://go-review.googlesource.com/c/net/+/351972
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/net/+/356984
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit to golang/net that referenced this issue Oct 29, 2021
@bradfitz @dmitshur
[internal-branch.go1.16-vendor] http2: shut down idle Transport conne…
ab1d67c 
…ctions after protocol errors

Updates golang/go#49076

Change-Id: Ic4e85cdc75b4baef7cd61a65b1b09f430a2ffc4b
Reviewed-on: https://go-review.googlesource.com/c/net/+/352449
Trust: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-on: https://go-review.googlesource.com/c/net/+/356985
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit to golang/net that referenced this issue Oct 29, 2021
@bradfitz @dmitshur
[internal-branch.go1.16-vendor] http2: fix Transport connection pool …
b843c7d 
…TOCTOU max concurrent stream bug

Updates golang/go#49076

Change-Id: I3e02072403f2f40ade4ef931058bbb5892776754
Reviewed-on: https://go-review.googlesource.com/c/net/+/352469
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
Trust: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/net/+/356986
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit to golang/net that referenced this issue Oct 29, 2021
@fraenkel @dmitshur
[internal-branch.go1.16-vendor] http2: remove PingTimeout from TestTr…
6e87631 
…ansportPingWhenReading

Use the default PingTimeout since it has no bearing on the test. A small
value can cause a failure on slower machines. Rely on the deadline to
determine a sufficient amount of time to complete.

Updates golang/go#49076

Change-Id: I9389777fa00ed5193f1fc7ae04d2e2134114c675
Reviewed-on: https://go-review.googlesource.com/c/net/+/352970
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Trust: Bryan C. Mills <bcmills@google.com>
Trust: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/net/+/356987
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit to golang/net that referenced this issue Oct 29, 2021
@neild @dmitshur
[internal-branch.go1.16-vendor] http2: refactor request write flow
1760f31 
Move the entire request write into a new writeRequest function,
which runs as its own goroutine.

The writeRequest function handles all indefintely-blocking
operations (in particular, network writes), as well as all
post-request cleanup: Closing the request body, sending a
RST_STREAM when necessary, releasing the concurrency slot
held by the stream, etc.

Consolidates several goroutines used to wait for stream
slots, write the body, and close response bodies.

Ensures that RoundTrip does not block past request cancelation.

Updates golang/go#49076

Change-Id: Iaf8bb3e17de89384b031ec4f324918b5720f5877
Reviewed-on: https://go-review.googlesource.com/c/net/+/353390
Trust: Damien Neil <dneil@google.com>
Trust: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/net/+/356988
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit to golang/net that referenced this issue Oct 29, 2021
@neild @dmitshur
[internal-branch.go1.16-vendor] http2: avoid extra GetConn trace call
d06dfc7 
CL 352469 inverts the case in shouldTraceGetConn: We want to call GetConn
for connections that have been previously used, but it calls GetConn
only on approximately the first use. "Approximately", because it uses
cc.nextStreamID to determine if the connection has been used, which
is racy.

Restructure the decision to call GetConn to track a per-ClientConn bool
indicating whether GetConn has already been called for this connection.
Set this bool for connections received from net/http, clear it after the
first use of the connection.

Updates golang/go#49076

Change-Id: I8e3dbba7cfbce9acd3612e39b6b6ee558bbfc864
Reviewed-on: https://go-review.googlesource.com/c/net/+/353875
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/net/+/357089
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit to golang/net that referenced this issue Oct 29, 2021
@neild @dmitshur
[internal-branch.go1.16-vendor] http2: avoid clientConnPool panic whe…
d585ef0 
…n NewClientConn fails

Updates golang/go#49076

Change-Id: Id5c1c40f9d8a07b7e6d399cc4e9f60ebe10ccf49
Reviewed-on: https://go-review.googlesource.com/c/net/+/353881
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/net/+/357090
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
gopherbot pushed a commit to golang/net that referenced this issue Oct 29, 2021
@neild @dmitshur
[internal-branch.go1.16-vendor] http2: avoid race in TestTransportReq…
20ed279 
…BodyAfterResponse_403.

This test sends a request with a 10MiB body, reads a 403 response
while the body is still being written, and closes the response body.
It then verifies that the full request body was not written, since
reading a response code >=300 interrupts body writes.

This can be racy: We process the status code and interrupt the body
write in RoundTrip, but it is possible for the body write to complete
before RoundTrip looks at the response.

Adjust the test to have more control over the request body:
Only provide half the Request.Body until after the response headers
have been received.

Updates golang/go#49076

Change-Id: Id4802b04a50f34f6af28f4eb93e37ef70a33a068
Reviewed-on: https://go-review.googlesource.com/c/net/+/354130
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/net/+/357091
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
gopherbot pushed a commit to golang/net that referenced this issue Oct 29, 2021
@neild @dmitshur
[internal-branch.go1.16-vendor] http2: detect write-blocked PING frames
198b78c 
We start the PingTimeout timer before writing a PING frame.
However, writing the frame can block indefinitely (either
acquiring cc.wmu or writing to the conn), and the timer is
not checked until after the frame is written.

Move PING writes into a separate goroutine, so we can detect
write-blocked connections.

Updates golang/go#49076

Change-Id: Ifd67fa23799e750d02754e1fe5d32719f60faed4
Reviewed-on: https://go-review.googlesource.com/c/net/+/354389
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/net/+/357092
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
gopherbot pushed a commit to golang/net that referenced this issue Oct 29, 2021
@neild @dmitshur
[internal-branch.go1.16-vendor] http2: don't rely on system TCP buffe…
640e170 
…r sizes in TestServer_MaxQueuedControlFrames

This test relies on filling up a TCP write buffer, but buffer sizes aren't
under our control and can be large. Use a net.Conn wrapper that blocks instead.

Updates golang/go#49076

Change-Id: I72471ef293f906b33f2a0fd81d69a3dd57f32fde
Reviewed-on: https://go-review.googlesource.com/c/net/+/349932
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/net/+/357093
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit to golang/net that referenced this issue Oct 29, 2021
@AlexanderYastrebov @dmitshur
[internal-branch.go1.16-vendor] http2: return unexpected eof on empty…
e5dd05d 
… response with non-zero content length

Updates golang/go#49076

Change-Id: I8b8262ba8e28a129f6aacfca43b7b8d9e605a0ce
GitHub-Last-Rev: 11b92cc
GitHub-Pull-Request: #114
Reviewed-on: https://go-review.googlesource.com/c/net/+/354141
Reviewed-by: Damien Neil <dneil@google.com>
Trust: Damien Neil <dneil@google.com>
Trust: Alexander Rakoczy <alex@golang.org>
Run-TryBot: Alexander Rakoczy <alex@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/net/+/357094
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit to golang/net that referenced this issue Oct 29, 2021
@neild @dmitshur
[internal-branch.go1.16-vendor] http2: close the Request's Body when …
d26011a 
…aborting a stream

After RoundTrip returns, closing the Response's Body should
interrupt any ongoing write of the request body. Close the
Request's Body to unblock the body writer.

Take additional care around the use of a Request after
its Response's Body has been closed. The RoundTripper contract
permits the caller to modify the request after the Response's
body has been closed.

Updates golang/go#49076

Change-Id: I261e08eb5d70016b49942d72833f46b2ae83962a
Reviewed-on: https://go-review.googlesource.com/c/net/+/355491
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/net/+/357095
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit to golang/net that referenced this issue Oct 29, 2021
@neild @dmitshur
[internal-branch.go1.16-vendor] http2: deflake TestTransportReqBodyAf…
275be3f 
…terResponse_200

Don't send the END_STREAM flag from the server until the client
sends its END_STREAM. Avoids a flaky failure when the client
sends the END_STREAM in a zero-length DATA frame:

  - The server reads bodySize bytes and half-closes the stream.
  - The client's Response.Body returns EOF.
  - The test calls Response.Body.Close.
  - The client resets the stream.

Avoid hanging forever on the client side of the test if the
server side exits with an error.

Updates golang/go#49076

Change-Id: Ic921a3f60149abbb5434ec7a53985becea7b23c3
Reviewed-on: https://go-review.googlesource.com/c/net/+/355649
Trust: Damien Neil <dneil@google.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-on: https://go-review.googlesource.com/c/net/+/357096
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
gopherbot pushed a commit to golang/net that referenced this issue Oct 29, 2021
@neild @dmitshur
[internal-branch.go1.16-vendor] http2: on write errors, close ClientC…
2f744fa 
…onn before returning from RoundTrip

Deflakes TestTransportRoundtripCloseOnWriteError.

Updates golang/go#49076

Change-Id: I4384d9091d55307d15fbd44b1b8137dcc8939c86
Reviewed-on: https://go-review.googlesource.com/c/net/+/356029
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/net/+/357097
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit to golang/net that referenced this issue Oct 29, 2021
@neild @dmitshur
[internal-branch.go1.16-vendor] http2: don't abort half-closed stream…
c4031f5 
…s on server connection close

If the server sends an END_STREAM for a stream and then closes the
connection, allow the stream to complete normally.

Updates golang/go#49076

Change-Id: Ia876e6e6e7eb4a0563db74c931c03a44620ece08
Reviewed-on: https://go-review.googlesource.com/c/net/+/356030
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/net/+/357098
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/359774 mentions this issue: [release-branch.go1.16] net/http: update bundled golang.org/x/net/http2

@gopherbot
Copy link
Contributor

Closed by merging 631b567 to release-branch.go1.16.

gopherbot pushed a commit that referenced this issue Nov 1, 2021
@dmitshur
[release-branch.go1.16] net/http: update bundled golang.org/x/net/http2
631b567 
Pull in approved backports to golang.org/x/net/http2:

	d8c3cde set ContentLength to -1 for HEAD response with no Content-Length
	7b24c0a set Response.ContentLength to 0 when headers end stream
	c4031f5 don't abort half-closed streams on server connection close
	2f744fa on write errors, close ClientConn before returning from RoundTrip
	275be3f deflake TestTransportReqBodyAfterResponse_200
	d26011a close the Request's Body when aborting a stream
	e5dd05d return unexpected eof on empty response with non-zero content length
	640e170 don't rely on system TCP buffer sizes in TestServer_MaxQueuedControlFrames
	198b78c detect write-blocked PING frames
	20ed279 avoid race in TestTransportReqBodyAfterResponse_403.
	d585ef0 avoid clientConnPool panic when NewClientConn fails
	d06dfc7 avoid extra GetConn trace call
	1760f31 refactor request write flow
	6e87631 remove PingTimeout from TestTransportPingWhenReading
	b843c7d fix Transport connection pool TOCTOU max concurrent stream bug
	ab1d67c shut down idle Transport connections after protocol errors
	3741e47 remove check for read-after-close of request bodies
	2df4c53 fix race in DATA frame padding refund
	d7eefc9 avoid blocking while holding ClientConn.mu
	78e8d65 fix off-by-one error in client check for max concurrent streams
	828651b close request body after early RoundTrip failures
	59c0c25 limit client initial MAX_CONCURRENT_STREAMS
	524fcad make Transport not reuse conns after a stream protocol error
	0fe5f8a accept zero-length block fragments in HEADERS frames
	0e5043f close the request body if needed
	bb4ce86 reduce frameScratchBuffer caching aggressiveness
	3112343 also set "http/1.1" ALPN in ConfigureServer
	63939f4 switch to ASCII equivalents of string functions
	54161af use (*tls.Dialer).DialContext in dialTLS
	75b906f discard DATA frames with higher stream IDs during graceful shutdown
	1dfe517 rework Ping test to rely less on timing

By doing:

	$ go get -d golang.org/x/net@internal-branch.go1.16-vendor
	go get: upgraded golang.org/x/net v0.0.0-20210901185431-d2e9a4ea682f => v0.0.0-20211101194150-d8c3cde3c676
	$ go mod tidy
	$ go mod vendor
	$ go generate -run=bundle std

Fixes #49076.
Fixes #48822.
Fixes #48649.

Change-Id: Ie17f327eef2b6e6a9a1ac7635c5c4daef792e893
Reviewed-on: https://go-review.googlesource.com/c/go/+/359774
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
@rsc rsc unassigned neild Jun 23, 2022
@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Projects
None yet
Milestone
Go1.16.10
Development

No branches or pull requests
3 participants
@neild @gopherbot @cherrymui