Replace ad-hoc pre-synctest synchronization with testing/synctest.
Many of the http2 package tests use a custom-built fake time
implementation and a idleness-detection mechanism based on parsing
goroutine stack dumps. Experience with this approach to testing
eventually led to the development of the testing/synctest package.
Switch over to testing/synctest.
The synctest package became available as an experiment in Go 1.24
(only when GOEXPERIMENT=synctest is set), and was fully released
with some API changes in Go 1.25.
- Use the released synctest API on Go 1.25.
- Use the experimental API (synctest.Run) on Go 1.24 when
GOEXPERIMENT=synctest is set. (Note that we set this on trybots.)
- Skip tests on Go 1.24 when GOEXPERIMENT=synctest is not set.
The x/net module requires go1.24, so older versions
can be disregarded.
Change-Id: Ifc13bfdd9bdada6c016730a78bd5972a5193ee30
Reviewed-on: https://go-review.googlesource.com/c/net/+/700996
Reviewed-by: Nicholas Husin <husin@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Damien Neil <dneil@google.com>
Reviewed-by: Nicholas Husin <nsh@golang.org>
FrameType is a dense integer range, so we can store the frameParsers
in an array instead of a map. This should be a very small performance
win on all Go http2 servers. For high QPS gRPC services, this function
is visible in the Go profiler. For example, it shows up as 0.16% of
all CPU time on one production service at Datadog.
Change FrameType.String() to use the same pattern.
Add a test for testFrameType with unknown FrameTypes.
Fixesgolang/go#73613
Change-Id: I5f5b523e011a99d6b428cbdbfd97415e488169d1
Reviewed-on: https://go-review.googlesource.com/c/net/+/670415
Reviewed-by: Sean Liao <sean@liao.dev>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
If for some reason the server sends an HTTP/1.1 response
starting with "HTTP/1.1 " (9 bytes), http2 transport interprets that as
a valid frame header for an expected payload of ~4MB, and fails with
non-descriptive messages like "unexpected EOF".
This could happen, for example, if ALPN is misconfigured on the server's
load balancer.
This change attempts to improve that feedback by noting in the error
messages whenever the frame header was exactly the "HTTP/1.1 " bytes.
Change-Id: I7bf9ed2ee7f299b939b9004386f5bfa30a4e9032
GitHub-Last-Rev: d6e410daa3
GitHub-Pull-Request: golang/net#224
Reviewed-on: https://go-review.googlesource.com/c/net/+/623155
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: David Chase <drchase@google.com>
Auto-Submit: Sean Liao <sean@liao.dev>
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Sean Liao <sean@liao.dev>
The http2 package uses a precursor to the experimental
testing/synctest package, parsing runtime.Stack output
to determine when goroutines are idle.
When GOOS=js, some tests which use t.Log are flaky.
t.Log blocks in the syscall package writing to stdout.
The GOOS=js implementation of the syscall leaves the goroutine
blocked on a channel operation, which synctest interprets
as the goroutine being "durably blocked".
Fix the http2 synctest to treat any goroutine blocked in the
syscall package as not being durably blocked.
Making this fix reveals another bug when GOOS=js: Looping
while calling runtime.Gosched does not appear to permit
syscalls to make progress. Add a few time.Sleep(1) calls
while waiting for idleness to work around the problem.
While changing things in here, change http2's synctest
to not treat goroutines blocked on mutex operations as
durably blocked. This matches the behavior of testing/synctest.
(This would all be simpler if we just used testing/synctest,
but we don't want to make the http2 package depend on an
experimental API.)
Fixesgolang/go#67693
Change-Id: I889834e97e4a33f4ef232278b1a78af00d52d261
Reviewed-on: https://go-review.googlesource.com/c/net/+/653696
Reviewed-by: Jonathan Amsterdam <jba@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Damien Neil <dneil@google.com>
HTTP/2 requires that pseudo-headers (which start with : and are
used to pass information other than the regular request headers)
be encoded before all regular headers.
The x/net/http2 Transport's extended CONNECT support is enabled by
the user setting a ":protocol" header in the Request. This header
matches the pseudo-header that will be sent on the wire.
Ensure that the :protocol pseudo-header is sent before any regular
headers.
For golang/go#70728
Change-Id: I70de7ad524ab9457d6dfb61cb3fabe3d53c6b39b
Reviewed-on: https://go-review.googlesource.com/c/net/+/641476
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Damien Neil <dneil@google.com>
Reviewed-by: Antonio Ojea <aojea@google.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Browsers interpret a server advertising extended CONNECT support as
indicating the server supports WebSockets-over-HTTP/2.
However, WebSocket-over-HTTP/2 requires support from both the HTTP
implementation and the WebSocket implementation, and existing
Go WebSocket packages don't support HTTP/2.
Disable extended CONNECT support by default, since advertising it
is a non-backwards-compatible change.
For golang/go#71128
For golang/go#49918
Change-Id: Ie7d3ee2cd48124836a00bad320752e78719ffc46
Reviewed-on: https://go-review.googlesource.com/c/net/+/641475
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>
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>
