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.)
Fixesgolang/go#32388.
Fixesgolang/go#48340.
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>
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.
Fixesgolang/go#48341.
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>
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.
Fixesgolang/go#39389
Change-Id: I35fecd501ca39cd059c7afd1d44090b023f16e1e
GitHub-Last-Rev: 0d1114d3a5
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>
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#47635
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>
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#43989
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>
Add golang-dev as the Autocert notification email so Let's Encrypt can
send us emails. golang-dev is not an ideal choice, but we need something
publicly accessible and there isn't an obvious better option. My
understanding is we should expect essentially no emails so I don't want
to worry too much about it.
Updates golang/go#47108.
Change-Id: Ic3b5b7554d516ea2840bb56499eb3b8f35bf2304
Reviewed-on: https://go-review.googlesource.com/c/net/+/334929
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
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#46310
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>
When using h2c.NewHandler, the *http.Request object for h2c requests has a
.Context() that doesn't inherit from the *http.Server's BaseContext. This
is surprising for users of vanilla net/http, and is surprising to users of
http2.ConfigureServer; HTTP/1 requests inherit from that BaseContext, and
TLS h2 requests inherit from that BaseContext, but cleartext h2c requests
don't.
So, modify h2c.NewHander to respect that base Context, by way of the
hijacked Request's .Context().
Change-Id: Ibc24ae6e2fb153d9561827ad791329487dae8e5a
GitHub-Last-Rev: 821b2070f7
GitHub-Pull-Request: golang/net#88
Reviewed-on: https://go-review.googlesource.com/c/net/+/278972
Reviewed-by: Liam Haworth <liam@haworth.id.au>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Trust: Brad Fitzpatrick <bradfitz@golang.org>
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
If the server sends a GOAWAY at the same time that a client sends
HEADERS+DATA, the server will discard the HEADERS, but error on the DATA
frame. Luckily, the server doesn't turn this into a connection error
because it's already in a GOAWAY state. It just logs the PROTOCOL_ERROR,
but that produces a confusing log message.
This change effectively suppresses the log message by discarding the
DATA frame rather than erroring on it. Also, we now discard any frames for
streams with identifiers higher than the identified last stream. This is
done as per section 6.8 of the HTTP2 spec.
I also updated some stale documentation while I was trying to understand
the logic.
This is CL 188360 with a test
Fixesgolang/go#32112
Co-authored-by: Yunchi Luo <mightyguava@gmail.com>
Co-authored-by: Michael Fraenkel <michael.fraenkel@gmail.com>
Change-Id: I612c2bd82e37551e813af0961b16a98d14e77c38
Reviewed-on: https://go-review.googlesource.com/c/net/+/237957
Run-TryBot: Agniva De Sarker <agniva.quicksilver@gmail.com>
Run-TryBot: Emmanuel Odeke <emmanuel@orijtech.com>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Damien Neil <dneil@google.com>
Once a request body is scheduled to be written, a result of the write is always
expected. If the body writer is cancelled, and the write was never started,
send a successful result.
The test included is a modified version of the TestNoSniffExpectRequestBody_h2 found
in net/http.
Updates golang/go#42498
Change-Id: If3f23993170bdf10e9ae4244ec13ae269bd3877a
Reviewed-on: https://go-review.googlesource.com/c/net/+/269058
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Trust: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
When executing the clienttester.run(), it would exit when both
the client and server completed successfully or if either side failed.
If one side fails, the other side may not have finished. If an error was
reported, the goroutine was caught executing after the test completed.
The expectation is always that both sides complete successfully. In the
case where one side fails, we still need to wait for the other side to
complete.
Close both client and server connections on error to expedite the
completion.
Fixesgolang/go#41299
Change-Id: If47fbe61de42495bb2b1327bd5b03d6c295670dc
Reviewed-on: https://go-review.googlesource.com/c/net/+/267760
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Dmitri Shuralyov <dmitshur@golang.org>
The ConfigureTransport function doesn't provide any way to get at the
http2 Transport it creates, making it impossible to configure transport
parameters such as ReadIdleTimeout.
Add a ConfigureTransports function which returns the http2 Transport.
The very similar names are unfortunate, but they'll sort next to each
other in godoc and the pluralized ConfigureTransports hints at its purpose:
it lets you configure both the http and http2 transports.
Fixesgolang/go#40201.
Updates golang/go#41721.
Change-Id: I97aa345f369f49462c41d3f60d35660c06c51287
Reviewed-on: https://go-review.googlesource.com/c/net/+/264017
Trust: Damien Neil <dneil@google.com>
Trust: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
When a new connection is created, and a write error occurs during the
initial exchange, the connection must be closed. There is no guarantee
that the caller will close the connection.
When a connection with an existing write error is used or being used, it
will stay in use until its read loop completes. Requests will continue
to use this connection and fail when writing its header. These
connections should be closed to force the cleanup in its readLoop.
Fixesgolang/go#39337
Change-Id: I45e1293309e40629531f4cbb69387864f4f71bc2
Reviewed-on: https://go-review.googlesource.com/c/net/+/240337
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Brad Fitzpatrick <bradfitz@golang.org>
Trust: Damien Neil <dneil@google.com>
When the body.Write fails during processData, the connection flow
control must be updated to account for the data received. The connection's
WINDOW_UPDATE should reflect the amount of data that was not successfully
written. The stream is about to be closed, so no update is required.
Fixesgolang/go#40423
Change-Id: I546597cedf3715e6617babcb3b62140bf1857a27
Reviewed-on: https://go-review.googlesource.com/c/net/+/245158
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Emmanuel Odeke <emm.odeke@gmail.com>
Continuing the work of CL 234817, we enforce the same for HTTP/2
connections; that Content-Length header values with a sign (such as
"+5") are rejected or ignored. In each place, the handling of such
incorrect headers follows the approach already in place.
Fixesgolang/go#39017
Change-Id: Ie4854962dd0091795cb861d1cb3a78ce753fd3a8
Reviewed-on: https://go-review.googlesource.com/c/net/+/236098
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
