CL 586249 unified frame read/write functions used by client
and server tests, but inadvertently broke some benchmarks.
Fix those benchmarks.
This mostly restores the previous behavior of the affected
benchmarks (for example, testing only to see that a DATA frame
contains an END_STREAM marker, ignoring the number of bytes
in the frame).
Fixesgolang/go#70647
Change-Id: I2b0099c3513ac8754d11c4e37b7d63277a0fb0b1
Reviewed-on: https://go-review.googlesource.com/c/net/+/633055
Auto-Submit: Damien Neil <dneil@google.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Antonio Ojea <aojea@google.com>
gRPC has an unfortunate behavior of stictly rate limiting
the number of PING frames that it will receive. The default is
two PING frames every two hours when no requests are in flight;
two PING frames every five minutes when a request is in flight;
and the limit resets every time the gRPC endpoint sends a
HEADERS or DATA frame.
When sending a RST_STREAM frame, the Transport can bundle a PING
frame with it to confirm the server is responding. When canceling
several requests in succession, this can result in hitting the
gRPC ping limit.
Work around this gRPC behavior by sending at most one bundled
PING per HEADERS or DATA frame received. We already limit
ourselves to one PING in flight at a time; now, when we receive
a PING response, disable sending additional bundled PINGs
until we read a HEADERS/DATA frame.
This does not affect keep-alive pings.
Fixesgolang/go#70575.
Change-Id: I7c4003039bd2dc52106b2806ca31eeeee37b7e09
Reviewed-on: https://go-review.googlesource.com/c/net/+/632995
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Auto-Submit: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
When we create a new connection for a request, the request should
fail if the connection attempt fails.
There is a race condition which can cause this to not happen:
- net/http sends a request to a http2.Transport
- the http2.Transport returns ErrNoCachedConn
- net/http creates a new tls.Conn and passes it to the http2.Transport
- the http2.Transport adds the conn to its connection pool
- the connection immediately encounters an error
- the http2.Transport removes the conn from its connection pool
- net/http resends the request to the http2.Transport
- the http2.Transport returns ErrNoCachedConn, and the process repeates
If the request is sent to the http2.Transport before the connection
encounters an error, then the request fails. But otherwise, we get
stuck in an infinite loop of the http2.Transport asking for a new
connection, receiving one, and throwing it away.
To fix this, leave a dead connection in the pool for a short while
if it has never had a request sent to it. If a dead connection is
used for a new request, return an error and remove the connection
from the pool.
Change-Id: I64eb15a8f1512a6bda52db423072b945fab6f4b5
Reviewed-on: https://go-review.googlesource.com/c/net/+/625398
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Allow net/http to pass unencrypted net.Conns to Server/Transport.
We don't have an existing way to pass a conn other than a *tls.Conn
into this package, so (ab)use TLSNextProto to pass unencrypted
connections:
The http2 package adds an "unencrypted_http2" entry to the
TLSNextProto maps. The net/http package calls this function
with a *tls.Conn wrapping a net.Conn with an UnencryptedNetConn
method returning the underlying, unencrypted net.Conn.
For golang/go#67816
Change-Id: I31f9c1ba31a17c82c8ed651382bd94193acf09b9
Reviewed-on: https://go-review.googlesource.com/c/net/+/625175
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Consider the case of an unresponsive client connection, where
the server has stopped responding. We send an infinite sequence of
requests to the connection in sequence, each with a timeout.
Each request counts against the concurrency limit for the
connection while active, but when a request times out we send
a RST_STREAM and free up the concurrency slot it was using.
We continue to try to send requests to the connection forever (or
until the kernel closes the underlying TCP connection, or until
ReadIdleTimeout/WriteByteTimeout results in us closing the connection).
Defend against this scenario by counting a canceled request
against the connection concurrency limit until we confirm the
server is responding. Specifically:
Track the number of in-flight request cancellations in cc.pendingResets.
This total counts against the connection concurrency limit.
When sending a RST_STREAM for a canceled request, increment
cc.pendingResets. Send a PING frame to the server, unless a PING
is already in flight.
When receiving a PING response, set cc.pendingResets to 0.
A hung connection will be used for at most
SETTINGS_MAX_CONCURRENT_STREAMS requests.
When StrictMaxConcurrentStreams is false, we will create a
new connection after reaching the concurrency limit for a hung one.
When StrictMaxConcurrentStreams is true, we will continue to
wait for the existing connection until some timeout closes it
or it becomes responsive again.
For golang/go#59690
Change-Id: I0151f9a594af14b32bcb6005a239fa19eb103704
Reviewed-on: https://go-review.googlesource.com/c/net/+/617655
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Carlos Amedee <carlos@golang.org>
Replace Transport's limit of 5 1xx responses with a limit based
on the maximum header size: The total size of all 1xx response
headers must not exceed the limit we use on the size of the
final response headers.
(This differs slightly from the corresponding HTTP/1 change,
which imposes a limit on all 1xx response headers *plus* the
final response headers. The difference isn't substantial,
and this implementation fits better with the HTTP/2 framer.)
When the user is reading 1xx responses using a Got1xxResponse
client trace hook, disable the limit: Each 1xx response is
individually limited by the header size limit, but there
is no limit on the total number of responses. The user is
responsible for imposing a limit if they want one.
For golang/go#65035
Change-Id: I9c19dbf068e0f580789d952f63113b3d21ad86fc
Reviewed-on: https://go-review.googlesource.com/c/net/+/615295
Reviewed-by: Cherry Mui <cherryyz@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Damien Neil <dneil@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
CL 111835 changed Transport stream ID numbering to start at
stream 3 when AllowHTTP is set. This was based on a
misunderstanding:
When a connection upgrades an HTTP/1.1 request to HTTP/2,
the initial HTTP/1.1 request occupies stream 1.
However, Transport does not perform HTTP protocol upgrades.
When using a Transport to send unencrypted HTTP/2 requests,
the entire connection uses HTTP/2, the first request is
sent as HTTP/2, and there is no reason not to use stream 1
for this request.
Starting from stream 3 is mostly harmless,
but ClientConn.idleStateLocked assumes that client streams
start from 1. This causes it to misidentify new single-use
connections as having already sent a request (when AllowHTTP
is set), and therefore not suitable for use.
Revert to always starting stream IDs at 1.
Fixesgolang/go#67671
Change-Id: I97c89de4ae49623d916f9dbd200f8252d2fd4247
Reviewed-on: https://go-review.googlesource.com/c/net/+/591275
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Client and server tests both write frames to a test connection
and read frames back. Frame reads are usually paired with
test expectations.
Unify the API used for frame reads/writes in tests.
Introduce a testConnFramer type that implements a common set
of read/write methods, and embed it in both client and server
test types.
Change-Id: I6927c43459ba24f150a21c058a92797754f82bf1
Reviewed-on: https://go-review.googlesource.com/c/net/+/586249
Reviewed-by: Jonathan Amsterdam <jba@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
clientStream.requestedGzip is set from clientStream.writeRequest,
and examined by clientConn.readLoop. I'm not sure if there's
any possible way for an actual data race to happen here in
practice, since the read loop should only examine the field
after the request is sent by writeRequest, but it's enough
for the race detector to complain.
Set the field in ClientConn.roundTrip instead, before
the clientStream has become accessible to any other goroutines.
No test, but a following CL has race detector failures without
this change.
Change-Id: Id30f1b95bcfcc35c213440e0e47cce3feaaff06d
Reviewed-on: https://go-review.googlesource.com/c/net/+/586245
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
When run with -coverspec, tests track which portions of the
specification they cover and the TestSpecCoverage test produces
an error if any sections lack test coverage.
This is a lovely idea, and perhaps we should resurrect it at
some point, but there is currently exactly one coverage
annotation, dating back to the first commit of this package.
Change-Id: I5d2d8a1032b783d113ed0982f7e97a3bd1c07a33
Reviewed-on: https://go-review.googlesource.com/c/net/+/586243
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
newServerTester is used to create an HTTP/2 server for testing.
It returns a *serverTester, which includes a number of methods
for sending frames to and reading frames from a server connection
under test.
Many tests also use newServerTester to simply start an
*httptest.Server. These tests pass an "optOnlyServer" to
indicate that they don't need a client connection to interact with.
They interact with the *httptest.Server, and use no methods or
fields of *serverTester.
Make a clearer distinction between test types.
Add a newTestServer function which starts and returns
an *httptest.Server.
This function replaces use of newServerTester with optOnlyServer.
Change-Id: Ia590c9b4dcc23c17e530b0cc273b9120965da11a
Reviewed-on: https://go-review.googlesource.com/c/net/+/586155
Reviewed-by: Jonathan Amsterdam <jba@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Rewrite the synchronization used in Transport tests.
This replaces the explicit synchronization, which requires
annotating every point where a goroutine in the code under test
may block, with implicit syncronization based on parsing
goroutine stacks to identify when all goroutines of interest are
blocked.
Change-Id: I02646e2752c359ed1b08126370a48f3d3c1fde77
Reviewed-on: https://go-review.googlesource.com/c/net/+/584895
Reviewed-by: Jonathan Amsterdam <jba@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Drop a trivial "gate" type used in exactly one location,
replacing it with an equivalent channel operation.
We've been using a "gate" with a different definition in
the quic package; dropping this hapax legomenon type from
the http2 package frees up the name if we want to use the
same gate here.
Change-Id: Id9c7d05daf7ee920c38090df960822fcc1168a4d
Reviewed-on: https://go-review.googlesource.com/c/net/+/584896
Reviewed-by: Jonathan Amsterdam <jba@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
When a server sends a GOAWAY frame, it indicates the ID of the
last stream it processed. We use this to mark any requests after
that stream as being safe to retry on a new connection.
Change this to not retry the first request on a connection if we
get a GOAWAY with an error, even if the GOAWAY has a stream ID
of 0 indicating that it didn't process that request.
If we're getting an error as the first result on a new connection,
then there's either something wrong with the server or something
wrong with our request; either way, retrying isn't likely to be
productive and may be unsafe.
This matches the behavior of the HTTP/1 client, which
also avoids retrying the first request on a new connection.
For golang/go#66668Fixesgolang/go#60636
Change-Id: I90ea7cfce2974dd413f7cd8b78541678850376a5
Reviewed-on: https://go-review.googlesource.com/c/net/+/576895
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
When closing a connection because a stream contained a request we
didn't like (for example, because the request headers exceed
the maximum we will accept), set the LastStreamID in the GOAWAY
frame to include the offending stream. This informs the client
that retrying the request is unlikely to succeed, and avoids
retry loops.
This change requires passing the stream ID of the offending
stream from Framer.ReadFrame up to the caller. The most sensible
way to do this would probably be in the error. However,
ReadFrame currently returns a defined error type for
connection-ending errors (ConnectionError), and that type is a
uint32 with no place to put the stream ID. Rather than changing
the returned errors, ReadFrame now returns an error along with
a non-nil Frame containing the stream ID, when a stream is
responsible for a connection-ending error.
For golang/go#66668
Change-Id: Iba07ccbd70ab4939aa56903605474d01703ac6e4
Reviewed-on: https://go-review.googlesource.com/c/net/+/576756
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Auto-Submit: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This test causes the server to send a GOAWAY and close a connection.
The server GOAWAY path writes a GOAWAY frame asynchronously, and
closes the connection if the write doesn't complete within 1s.
This is causing failures on some builders, when the frame write
doesn't complete in time.
The important aspect of this test is that the connection be closed.
Drop the check for the GOAWAY frame.
Change-Id: I099413be9c4dfe71d8fe83d2c6242e82e282293e
Reviewed-on: https://go-review.googlesource.com/c/net/+/576235
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Many RoundTrip tests involve testing against a test-defined
server with specific behaviors. For example: Testing
RoundTrip's behavior when the server violates flow
control limits.
Existing tests mostly use the clientTester type, which
starts separate goroutines for the Transport and a fake
server. This results in tests where the control flow
bounces around the test function, and requires each
test to manage its own synchronization.
Introduce a new framework for writing RoundTrip tests.
testClientConn allows client tests to be written linearly,
with synchronization provided by the test framework.
For example, a testClientConn test can, as a linear
sequence of actions:
- start RoundTrip;
- check the request headers sent;
- provide data to the request body;
- check that a DATA frame is sent;
- send response headers from the server to the client;
- check that RoundTrip returns.
See TestTestClientConn at the top of clientconn_test.go
for a full example.
To enable synchronization with tests, this CL
instruments the RoundTrip path to record when
goroutines start, exit, and block waiting for events.
This adds a certain amount of noise and bookkeeping
to the client implementation, but (in my opinion)
this is more than repaid in improved testability.
The testClientConn also permits use of synthetic
time in tests. At the moment, this is limited to
the response header timeout, but extending it to
other timeouts (read, 100-continue) should be
straightforward.
This CL converts a number of existing clientTester tests
to use the new framework, but not all.
Change-Id: Ief963889969363ec8469cd3c3de0becb2fc548f9
Reviewed-on: https://go-review.googlesource.com/c/net/+/563540
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
