This CL is the x/net counterpart to CL 755320.
This test contains a race condition in the server handler:
inHandler <- streamID
<-leaveHandler
We assume that all requests queue reading from leaveHandler in order,
but it is possible for the second request (stream id 3) to arrive at
leaveHandler before the first (stream id 1).
We could fix the race with a judicious synctest.Wait, but rewrite
the test to use serverHandlerCall to manipulate server handlers,
which permits us to precisely pick which request to unblock.
Fixes#69670
Change-Id: I9507d1dba07f7d62bcdc6c9bb67c47466a6a6964
Reviewed-on: https://go-review.googlesource.com/c/net/+/755081
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Nicholas Husin <husin@google.com>
Reviewed-by: Nicholas Husin <nsh@golang.org>
Reviewed-by: Nicholas Husin <husin@google.com>
This change makes it easier to move x/net/http2 into std.
Moving the http2 package into std and importing it from net/http
(rather than bundling it as net/http/h2_bundle.go) requires
removing the http2->net/http dependency. Moving tests into
the http2_test package allows them to continue importing net/http
without creating a cycle.
Change-Id: If0799a94a6d2c90f02d7f391e352e14e6a6a6964
Reviewed-on: https://go-review.googlesource.com/c/net/+/749280
Auto-Submit: Damien Neil <dneil@google.com>
Reviewed-by: Nicholas Husin <husin@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Nicholas Husin <nsh@golang.org>
Years back, our HTTP/2 server implementation had a stream accounting bug
that would cause it to improperly report a PROTOCOL_ERROR. In response
to this, we modified our Transport to retry RoundTrip when a RST_STREAM
with PROTOCOL_ERROR was received from a peer.
At this point, this retry logic had outlived its usefulness. Instead, it
might cause issues, e.g. a client that sends a malformed request will
keep retrying repeatedly, despite there being zero chance for the
request to actually succeed.
Fixesgolang/go#77843
Change-Id: Ic043723e3535f68f91db33d8f6bcd7fc2dbce856
Reviewed-on: https://go-review.googlesource.com/c/net/+/750720
Reviewed-by: Nicholas Husin <husin@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
The addition of FramePriorityUpdate (0x10) in RFC 9218 introduced a gap
in the frameParsers array indices (0x0a-0x0f). These indices were
initialized to nil, causing a panic when typeFrameParser accessed them
for unassigned frame types (e.g., ALTSVC 0x0a).
This change adds a nil check in typeFrameParser to safely fallback to
parseUnknownFrame for these unassigned types, preventing the crash.
Fixesgolang/go#77652
Change-Id: I14d7ad85afc1eafabc46417a9fff10f9e0a22446
Reviewed-on: https://go-review.googlesource.com/c/net/+/746180
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
Auto-Submit: Damien Neil <dneil@google.com>
Reviewed-by: Mark Freeman <markfreeman@google.com>
Transport currently deadlocks when receiving a WINDOW_UPDATE for a
non-zero stream that increments its window beyond the 2^31-1 bytes
limit.
This is because endStreamError is called to end the non-zero stream,
which tries to lock an already-locked mutex. Therefore, create and use
endStreamErrorLocked instead, which assumes the mutex is already locked.
Fixesgolang/go#77331
Change-Id: Iea212f49a1f305d1bddefb8831dbaca00840870c
Reviewed-on: https://go-review.googlesource.com/c/net/+/739700
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Nicholas Husin <husin@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
As part of adding support for HTTP/2 stream prioritization, a
DisableClientPriority field will be added to http.Server to allow users
to completely disable client prioritization if desired. When
DisableClientPriority is set to true, HTTP/2 server will revert back to
the old behavior where streams are processed in a round-robin manner.
For golang/go#75500
Change-Id: Ida083b3ac17a953e5ddb3ad7ab8a81f9cde2bfc1
Reviewed-on: https://go-review.googlesource.com/c/net/+/737521
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Nicholas Husin <husin@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
To make sure that clients do not unnecessarily send RFC 7540 priority
signals when it would be treated as a no-op, this change makes it so
that our server always sends SETTINGS_NO_RFC7540_PRIORITIES in our
SETTINGS frame when our write scheduler is set to anything other than
the RFC 7540 write scheduler.
For golang/go#75500
Change-Id: I7a54251022087319999deda7efb663f8b251aa95
Reviewed-on: https://go-review.googlesource.com/c/net/+/729141
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Nicholas Husin <husin@google.com>
In RFC 9218, streams are non-incremental by default, meaning that they
are processed one-by-one to completion. This behavior is the opposite of
our current default of handling streams in a round-robin manner.
This might cause a surprising behavior change once we make the RFC 9218
priority scheduler the default write scheduler for most users (we assume
that most users will not be sending RFC 9218 priority signals, at least
initially). To avoid surprising users with such a behavior change, this
CL makes it so that the streams are only made non-incremental once there
has been a clear signal that the end-user is aware of RFC 9218.
For golang/go#75500
Change-Id: Ibd22cb279c43de0190962904c3809007447a5fe3
Reviewed-on: https://go-review.googlesource.com/c/net/+/729140
Reviewed-by: Michael Knyszek <mknyszek@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Nicholas Husin <husin@google.com>
RFC 9218 allows HTTP/2 stream priority to be set in two ways: via
PRIORITY_UPDATE frame and via header field. This change adds support for
the latter method.
As part of supporting priority adjustment via header field, this CL
also makes sure to look for the existence of an intermediary. If an
intermediary exists, default priority will be used for all streams to
ensure fairness between multiple clients who could be using the same
intermediary.
For golang/go#75500
For golang/go#75936
Change-Id: I6dc409b650fd52fa192d771a16b7a4ac5e51c9aa
Reviewed-on: https://go-review.googlesource.com/c/net/+/729120
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Nicholas Husin <husin@google.com>
This change adds initial support for the PRIORITY_UPDATE frame
introduced in RFC 9218.
Clients can now use a new exported function to write PRIORITY_UPDATE
frames easily. However, sending PRIORITY_UPDATE frames to the server
does not currently cause any behavior changes: we only use
PRIORITY_UPDATE frames to adjust stream priority when the RFC 9218 write
scheduler is being used for a particular connection. However, this
scheduler is not currently usable yet from any configuration surfaces
exposed to the user.
For golang/go#75500
Change-Id: Ie2c821cb0d2faa6e942e209e11638f190fc98e2b
Reviewed-on: https://go-review.googlesource.com/c/net/+/705917
Reviewed-by: Nicholas Husin <husin@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
When sending a RST_STREAM for a canceled request, we sometimes send
a PING frame along with the reset to confirm that the server is responsive
and has received the reset.
Sending too many PINGs trips denial-of-service detection on some servers,
causing them to close a connection with an ENHANCE_YOUR_CALM error.
Do not send a PING frame along with an RST_STREAM if the connection
has displayed signs of life since the canceled request began.
Specifically, if we've received any stream-related frames since the
request was sent, assume the server is responsive and do not send a PING.
We still send a PING if a request is canceled and no stream-related
frames have been received from the server since the request was first
sent.
For golang/go#76296
Change-Id: I1be3532febf9ac99d65e9cd35346c02306db5f9d
Reviewed-on: https://go-review.googlesource.com/c/net/+/720300
Reviewed-by: Nicholas Husin <husin@google.com>
Reviewed-by: Nicholas Husin <nsh@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Permit net/http to create new HTTP/2 client connections.
We do this by adding a NewClientConn method to the type the HTTP/2 client
registers with net/http.Transport.RegisterProtocol, which creates a
persistent connection from a net.Conn.
No tests in this CL. Tests will be in net/http, and will cover
both the HTTP/1 and HTTP/2 paths for NewClientConn.
For golang/go#75772
Change-Id: Ib1a06b4d13fdd6008e5db9a090c6e9632029a2a4
Reviewed-on: https://go-review.googlesource.com/c/net/+/722200
Reviewed-by: Nicholas Husin <husin@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Nicholas Husin <nsh@golang.org>
We use uint8 (0-255, inclusive) to represent the RFC 7540 priorities
weight (1-256, inclusive). To account for the difference, we add 1 to
the uint8 weight value within sortPriorityNodeSiblingsRFC7540.
However, the addition was done before converting the uint8 type to
float. As a result, when provided a maximum weight value, overflow will
happen and will cause the scheduler to treat the maximum weight as a
minimum weight instead.
This CL fixes the issue by making sure the addition happens after the
type conversion.
Change-Id: I404e87e5ad85fa06d5fa49cda613c93ac8847bdc
Reviewed-on: https://go-review.googlesource.com/c/net/+/714742
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Nicholas Husin <husin@google.com>
This is a copy of the CL 510255 with the difference that
it preserves use of fs.ErrClosed when reading after close.
goos: darwin
goarch: arm64
pkg: golang.org/x/net/http2
cpu: Apple M4
│ HEAD~1 │ HEAD │
│ sec/op │ sec/op vs base │
ClientGzip-10 752.8µ ± 1% 750.7µ ± 2% ~ (p=0.393 n=10)
│ HEAD~1 │ HEAD │
│ B/op │ B/op vs base │
ClientGzip-10 75.49Ki ± 0% 33.98Ki ± 3% -54.99% (p=0.000 n=10)
│ HEAD~1 │ HEAD │
│ allocs/op │ allocs/op vs base │
ClientGzip-10 705.0 ± 0% 698.5 ± 0% -0.92% (p=0.000 n=10)
Updates golang/go#61353
Change-Id: I0fdc0c0a5947d27dcc615e5bcf4d4620c2c95d9e
GitHub-Last-Rev: 4d04dc93f3
GitHub-Pull-Request: golang/net#239
Reviewed-on: https://go-review.googlesource.com/c/net/+/710235
Reviewed-by: Sean Liao <sean@liao.dev>
Reviewed-by: Michael Pratt <mpratt@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
Auto-Submit: Damien Neil <dneil@google.com>
This change exports two new methods on the Framer, ReadFrameHeader and
ReadFrameBodyForHeader, which split the functionality of the existing
ReadFrame method.
This provides more granular control, allowing callers to inspect the
frame header before deciding whether or how to read the frame body.
This is useful for applications that may need to make decisions based on frame
type.
Fixesgolang/go#73560
Change-Id: I60b42d2889095fac8e243022886740bc6dd94012
Reviewed-on: https://go-review.googlesource.com/c/net/+/710515
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
Auto-Submit: Damien Neil <dneil@google.com>
TestGoroutineLock sets DebugGoroutines = true.
When a previous test leaves a server running after exiting,
this write to DebugGoroutines can race with reads from the server.
Obviously tests shouldn't leave goroutines around after they exit,
but it happens and when it does it can show up here as a rare
and hard-to-debug flake.
DebugGoroutines is always true in tests, so there's no need to set
it here. Just leave it alone.
Fixesgolang/go#75811
Change-Id: Iebeab2a22642cbd6867b9f4f5a171c91ea697b17
Reviewed-on: https://go-review.googlesource.com/c/net/+/710675
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Nicholas Husin <nsh@golang.org>
Reviewed-by: Nicholas Husin <husin@google.com>
Auto-Submit: Damien Neil <dneil@google.com>
Reviewed-by: Nicholas Husin <nsh@golang.org>
Our previous implementation of writeQueue relies on one
[]FrameWriteRequests, forcing us to copy the rest of the slice's content
whenever we remove an item from the front.
This change remedies this problem by implementing writeQueue using
two-stage queues, similar to Okasaki's purely functional queue.
With 25 frames per stream, we are observing the following performance
improvement:
goos: linux
goarch: amd64
pkg: golang.org/x/net/http2
cpu: AMD EPYC 7B13
│ /tmp/old │ /tmp/new │
│ sec/op │ sec/op vs base │
WriteQueue-64 508.3n ± 3% 305.7n ± 3% -39.86% (p=0.000 n=10)
│ /tmp/old │ /tmp/new │
│ B/op │ B/op vs base │
WriteQueue-64 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
¹ all samples are equal
│ /tmp/old │ /tmp/new │
│ allocs/op │ allocs/op vs base │
WriteQueue-64 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
¹ all samples are equal
As the number of frames increases, the performance difference becomes
more stark as the old implementation does a quadratic amount of copying
in total to be able to fully consume a queue.
Change-Id: Ide816ebdd89a41275b5829683c0f10d48321af50
Reviewed-on: https://go-review.googlesource.com/c/net/+/710635
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Nicholas Husin <husin@google.com>
Previously, the RFC 9218 write scheduler had a bug where AdjustStream()
did not update the stream's metadata after adjusting its priority. This
results in the function not being idempotent, where repeated calls to it
for the same stream can instead remove an unrelated stream from our
scheduler, and duplicate the stream whose priority is being updated.
For go/golang#75500
Change-Id: Iaf3dd819d02839bc6cff65027c4916f9f2fa3e5b
Reviewed-on: https://go-review.googlesource.com/c/net/+/709477
Reviewed-by: Nicholas Husin <husin@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
This change introduces a new write scheduler that prioritizes writes
based on RFC 9218. Eventually, this scheduler will be used to replace
the existing priority scheduler based on RFC 7540, which has been
deprecated in RFC 9113.
No behavioral changes has been introduced as this scheduler is not used
anywhere yet.
goos: linux
goarch: amd64
pkg: golang.org/x/net/http2
cpu: AMD EPYC 7B13
BenchmarkWriteSchedulerThroughputRoundRobin-64 100000 140884 ns/op 139201 B/op 2900 allocs/op
BenchmarkWriteSchedulerLifetimeRoundRobin-64 100000 149632 ns/op 139202 B/op 2900 allocs/op
BenchmarkWriteSchedulerThroughputRandom-64 100000 218311 ns/op 139201 B/op 2900 allocs/op
BenchmarkWriteSchedulerLifetimeRandom-64 100000 216559 ns/op 139203 B/op 2900 allocs/op
BenchmarkWriteSchedulerThroughputPriorityRFC7540-64 100000 587625 ns/op 139201 B/op 2900 allocs/op
BenchmarkWriteSchedulerThroughputPriorityRFC9218Incremental-64 100000 149563 ns/op 139200 B/op 2900 allocs/op
BenchmarkWriteSchedulerLifetimePriorityRFC9218Incremental-64 100000 163697 ns/op 139201 B/op 2900 allocs/op
BenchmarkWriteSchedulerThroughputPriorityRFC9218NonIncremental-64 100000 145364 ns/op 139201 B/op 2900 allocs/op
BenchmarkWriteSchedulerLifetimePriorityRFC9218NonIncremental-64 100000 159316 ns/op 139203 B/op 2900 allocs/op
For golang/go#75500
Change-Id: Id5db195f6f75970f9cc3c7b7a292df96a139de8b
Reviewed-on: https://go-review.googlesource.com/c/net/+/704758
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Nicholas Husin <husin@google.com>
This change renames the file for the RFC 7540 priority write scheduler
to writesched_priority_rfc7540.go file. Existing symbols have also been
renamed to make it explicit that they are only used for the RFC 7540
priority implementation.
This is done so that when we introduce the new RFC 9218 priority write
scheduler, we will not cause confusion with regards to which symbols are
used for which scheduler.
This CL only renames and moves symbols, no behavior changes have been
introduced.
For golang/go#75500
Change-Id: I5c31bd51bc0d25415ff72909cc0b8f9fef44c052
Reviewed-on: https://go-review.googlesource.com/c/net/+/704757
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Split testClientConnClose into four separate tests,
rather than having one big function which performs
different tests depending on its parameter.
Use the more modern testClientConn in these tests,
for simplicity.
Drop the activeStreams function, which pokes a bit too
far into implementation internals and isn't necessary
for the new tests. (It had one use outside of
testClientConnClose, which provides little useful
signal and can also be dropped.)
Change-Id: Id8d1c7feab59c1f041bc2d1cf0398e8b1e230c69
Reviewed-on: https://go-review.googlesource.com/c/net/+/701005
Reviewed-by: Nicholas Husin <nsh@golang.org>
Auto-Submit: Damien Neil <dneil@google.com>
Reviewed-by: Nicholas Husin <husin@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This is a transport test. Use a testClientConn rather than a
Transport attached to a test server, for better control over
the frames sent to the test transport.
Drop one section of the test which pokes into the response
body's transportResponseBody type, as being too coupled to the
implementation internals.
Change-Id: I7bc7c7c756fd0c596424fab9a892dda8d9e89d1c
Reviewed-on: https://go-review.googlesource.com/c/net/+/701004
Reviewed-by: Nicholas Husin <nsh@golang.org>
Reviewed-by: Nicholas Husin <husin@google.com>
Auto-Submit: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Rewrite the slowest test in the http2 package.
This test was added in CL 23812 to verify that a Transport does not
provide flow control window updates until the user consumes data
from a stream.
Rewrite the test to use the more modern testClientConn, which permits
precise examination of when the transport sends WINDOW_UPDATE frames.
Change-Id: Ibcde492549cad6363ce0e1a5ba169da7a4427d85
Reviewed-on: https://go-review.googlesource.com/c/net/+/700923
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Nicholas Husin <nsh@golang.org>
Reviewed-by: Nicholas Husin <husin@google.com>
Auto-Submit: Damien Neil <dneil@google.com>
Rewrite the slowest test in the http2 package.
This test was added in CL 23287 to verify two changes:
- A server handler calling req.Body.Close does not
kill the request stream.
- A Transport does not leak a goroutine if a request body is
still being written when the request stream is closed.
Split the test into two individual tests, one for the
server behavior and one for the transport.
Change-Id: I211f458e1001df435d00c2e1ebd7f3072e053c89
Reviewed-on: https://go-review.googlesource.com/c/net/+/700922
Reviewed-by: Nicholas Husin <nsh@golang.org>
Reviewed-by: Nicholas Husin <husin@google.com>
Auto-Submit: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
It is often useful in server tests to orchestrate a sequence
of actions that involve both a server connection and request handler.
For example, we might want to have the request handler read from
the request body at a precise point in the test.
Add support for this to serverTester (used for most server tests).
Pass a nil handler to serverTester, and it will provide synchronous
access to the handler:
call := st.nextHandlerCall()
call.do(func(w http.ResponseWriter, r *http.Request) {
// this executes in the handler goroutine
})
Replace the existing handlerPuppet type, which provided a
similar mechanism but only worked on a single call at a time.
Change-Id: I023e032084f911ab4f9b803c393e4a55b12af87f
Reviewed-on: https://go-review.googlesource.com/c/net/+/701002
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Nicholas Husin <husin@google.com>
Auto-Submit: Nicholas Husin <nsh@golang.org>
Reviewed-by: Nicholas Husin <nsh@golang.org>
requestBody.Read avoids panicking when requestBody.conn is unset
and we are in tests. This seems like an excellent way to hide a
non-test panic. Drop the exceptional path.
This path was introduced in golang.org/cl/31636 to support a
test verifying that concurrently closing and reading from a
Request.Body does not race. Rewrite this test to use a real
server handler.
Change-Id: I778e78ff9ab45e248769557fff94d17940eb7a18
Reviewed-on: https://go-review.googlesource.com/c/net/+/701000
Auto-Submit: Nicholas Husin <nsh@golang.org>
Reviewed-by: Nicholas Husin <husin@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Nicholas Husin <nsh@golang.org>
The test-only function serverTester.wantFlowControlConsumed
purports to check for the amount of connection or stream level
flow control consumed. Calling it with a streamID of 0
checks the connection-level flow control tokens.
However, the stream-level flow control path is unimplemented and unused.
Calling this function with a non-zero streamID doesn't work,
and (fortunately) all tests that use it only check connection-level
flow control.
Rename the function to wantConnFlowControlConsumed.
Change-Id: I1d934e8b46b4c43d393d102f1b2621329a7472aa
Reviewed-on: https://go-review.googlesource.com/c/net/+/700920
Auto-Submit: Nicholas Husin <nsh@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Nicholas Husin <husin@google.com>
Reviewed-by: Nicholas Husin <nsh@golang.org>
