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

x/net/http2: backport critical fixes [1.17 backport] #49077

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

x/net/http2: backport critical fixes [1.17 backport] #49077

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

Comments

@neild
Copy link
Contributor

neild commented Oct 19, 2021

A number of fixes in x/net/http2 haven't been backported to 1.17. This is an umbrella issue to figure out which changes should be backported and doing it.

@neild neild self-assigned this Oct 19, 2021
@gopherbot gopherbot added this to the Go1.17.3 milestone Oct 19, 2021
@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/357679 mentions this issue: [internal-branch.go1.17-vendor] http2: remove check for read-after-close of request bodies

@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/357682 mentions this issue: [internal-branch.go1.17-vendor] http2: remove PingTimeout from TestTransportPingWhenReading

@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

gopherbot pushed a commit that referenced this issue Nov 1, 2021
Pull in approved backports to golang.org/x/net/http2:

	95aca89 set ContentLength to -1 for HEAD response with no Content-Length
	bd5b1b8 set Response.ContentLength to 0 when headers end stream
	27001ec don't abort half-closed streams on server connection close
	f0a8156 on write errors, close ClientConn before returning from RoundTrip
	9a182eb deflake TestTransportReqBodyAfterResponse_200
	821db7b close the Request's Body when aborting a stream
	028e125 return unexpected eof on empty response with non-zero content length
	5388f2f don't rely on system TCP buffer sizes in TestServer_MaxQueuedControlFrames
	fc298ce detect write-blocked PING frames
	e96ad84 avoid race in TestTransportReqBodyAfterResponse_403.
	7f15435 avoid clientConnPool panic when NewClientConn fails
	9572bae avoid extra GetConn trace call
	b04064c refactor request write flow
	7e165c9 remove PingTimeout from TestTransportPingWhenReading
	ef976fc fix Transport connection pool TOCTOU max concurrent stream bug
	1d9597c shut down idle Transport connections after protocol errors
	c173d09 remove check for read-after-close of request bodies
	466a463 fix race in DATA frame padding refund
	4028c5f avoid blocking while holding ClientConn.mu
	b91f72d fix off-by-one error in client check for max concurrent streams
	21e6c63 close request body after early RoundTrip failures
	e79adf9 limit client initial MAX_CONCURRENT_STREAMS
	c0c2bc5 make Transport not reuse conns after a stream protocol error
	14c0235 accept zero-length block fragments in HEADERS frames
	0d2c43c close the request body if needed
	5627bb0 reduce frameScratchBuffer caching aggressiveness
	c9f4fb0 also set "http/1.1" ALPN in ConfigureServer

By doing:

	$ go get -d golang.org/x/net@internal-branch.go1.17-vendor
	go get: upgraded golang.org/x/net v0.0.0-20210901185426-6d2eada6345e => v0.0.0-20211101194204-95aca89e93de
	$ go mod tidy
	$ go mod vendor
	$ go generate -run=bundle std

Fixes #49077.
Fixes #48823.
Fixes #48650.

Change-Id: Idb972ba5313080626b60b4111d52b80197364ff4
Reviewed-on: https://go-review.googlesource.com/c/go/+/359776
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
lezh added a commit to GoogleCloudPlatform/gcsfuse that referenced this issue Dec 2, 2021
The old version has a bug in net/http package causing deadlocks when
the number of open files exceeds the max connections per host. The new
version [fixes it](golang/go#49077).
fedosgad pushed a commit to fedosgad/oohttp that referenced this issue Jun 22, 2022
…nfigureServer

With CL 289209, we started enforcing ALPN protocol overlap when both
sides support ALPN. This means that an "http/1.1"-only ALPN-aware client
will fail to connect to a server with only "h2" in NextProtos.
Unfortunately, that's how ConfigureServer was setting up the tls.Config.

The new behavior mirrors ConfigureTransport on the client side (but not
Transport.newTLSConfig because the latter is used to make HTTP/2-only
connections).

Updates golang/go#49077

Change-Id: Idbab7a6f873f3be78104df6f2908895f4d399bd3
Reviewed-on: https://go-review.googlesource.com/c/net/+/325611
Trust: Filippo Valsorda <filippo@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/net/+/357669
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
fedosgad pushed a commit to fedosgad/oohttp that referenced this issue Jun 22, 2022
…ing aggressiveness

Use a sync.Pool, not per ClientConn.

Co-authored with dneil@google.com

Updates golang/go#49077

Change-Id: I93f06b19857ab495ffcf15d7ed2aaa2a6cb31515
Reviewed-on: https://go-review.googlesource.com/c/net/+/325169
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Trust: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/net/+/357670
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
fedosgad pushed a commit to fedosgad/oohttp that referenced this issue Jun 22, 2022
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#49077

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/+/357671
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
fedosgad pushed a commit to fedosgad/oohttp that referenced this issue Jun 22, 2022
…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#49077

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/+/357672
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
fedosgad pushed a commit to fedosgad/oohttp that referenced this issue Jun 22, 2022
… 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#49077

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/+/357673
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
fedosgad pushed a commit to fedosgad/oohttp that referenced this issue Jun 22, 2022
…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#49077

Change-Id: I35fecd501ca39cd059c7afd1d44090b023f16e1e
GitHub-Last-Rev: 0d1114d3a558cefed17008aba3e4a4d7b2ad3866
GitHub-Pull-Request: golang/net#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/+/357674
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>
fedosgad pushed a commit to fedosgad/oohttp that referenced this issue Jun 22, 2022
… 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#49077

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/+/357675
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
fedosgad pushed a commit to fedosgad/oohttp that referenced this issue Jun 22, 2022
… check for max concurrent streams

Updates golang/go#49077

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/+/357676
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
fedosgad pushed a commit to fedosgad/oohttp that referenced this issue Jun 22, 2022
…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#49077

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/+/357677
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
fedosgad pushed a commit to fedosgad/oohttp that referenced this issue Jun 22, 2022
… refund

Move flow adjustment back under cc.mu.

Updates golang/go#49077

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/+/357678
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
fedosgad pushed a commit to fedosgad/oohttp that referenced this issue Jun 22, 2022
…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#49077

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/+/357679
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
fedosgad pushed a commit to fedosgad/oohttp that referenced this issue Jun 22, 2022
…ctions after protocol errors

Updates golang/go#49077

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/+/357680
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
fedosgad pushed a commit to fedosgad/oohttp that referenced this issue Jun 22, 2022
…TOCTOU max concurrent stream bug

Updates golang/go#49077

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/+/357681
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
fedosgad pushed a commit to fedosgad/oohttp that referenced this issue Jun 22, 2022
…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#49077

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/+/357682
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
fedosgad pushed a commit to fedosgad/oohttp that referenced this issue Jun 22, 2022
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#49077

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/+/357683
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
fedosgad pushed a commit to fedosgad/oohttp that referenced this issue Jun 22, 2022
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#49077

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/+/357684
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
fedosgad pushed a commit to fedosgad/oohttp that referenced this issue Jun 22, 2022
…n NewClientConn fails

Updates golang/go#49077

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/+/357685
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
fedosgad pushed a commit to fedosgad/oohttp that referenced this issue Jun 22, 2022
…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#49077

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/+/357686
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
fedosgad pushed a commit to fedosgad/oohttp that referenced this issue Jun 22, 2022
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#49077

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/+/357687
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
fedosgad pushed a commit to fedosgad/oohttp that referenced this issue Jun 22, 2022
…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#49077

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/+/357688
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
fedosgad pushed a commit to fedosgad/oohttp that referenced this issue Jun 22, 2022
… response with non-zero content length

Updates golang/go#49077

Change-Id: I8b8262ba8e28a129f6aacfca43b7b8d9e605a0ce
GitHub-Last-Rev: 11b92cc8ba6e1d08716aac816d33b659563a893f
GitHub-Pull-Request: golang/net#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/+/357689
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
fedosgad pushed a commit to fedosgad/oohttp that referenced this issue Jun 22, 2022
…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#49077

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/+/357690
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
fedosgad pushed a commit to fedosgad/oohttp that referenced this issue Jun 22, 2022
…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#49077

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/+/357691
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
fedosgad pushed a commit to fedosgad/oohttp that referenced this issue Jun 22, 2022
…onn before returning from RoundTrip

Deflakes TestTransportRoundtripCloseOnWriteError.

Updates golang/go#49077

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/+/357692
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
fedosgad pushed a commit to fedosgad/oohttp that referenced this issue Jun 22, 2022
…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#49077

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/+/357693
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@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.
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Projects
None yet
Development

No branches or pull requests

3 participants