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 CL adds RegisterServer and RegisterTransport, which are used to
plug our HTTP/3 implementation into net/http. For now, (inaccurately)
assume that RegisterTransport do not take any configs for simplicity.
Various exported symbols have also been changed to be unexported, since
HTTP/3 will be used only via net/http and will not support direct usage.
For golang/go#77440
Change-Id: I7b9528c193c18f5049da7f497d4259153b8770eb
Reviewed-on: https://go-review.googlesource.com/c/net/+/739881
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>
The existing implementation of the Name.unpack method does not check the
length of the domain name until parsing is complete. This allows a
malicious user to supply an unreasonably large name and wastle cycles
parsing. This change moves an equivalent check into the loop during
process to short-circuit if we've created too large of a name.
For golang/go#77540
Change-Id: I4c4bf20c0342825a09cefd9b0b3c0bdce0c80137
Reviewed-on: https://go-review.googlesource.com/c/net/+/750260
Reviewed-by: Roland Shoemaker <roland@golang.org>
Auto-Submit: Roland Shoemaker <roland@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
RoundTrip will now call httptrace.ClientTrace.Got1xxResponse, if any,
when receiving 1xx status response from a peer. This allows our client
and server to use HTTP 103 end-to-end.
Got100Continue and Wait100Continue have also been added to RoundTrip as
they are nearby. The rest of httptrace.ClientTrace will be added in the
future.
For golang/go#70914
Change-Id: Ia7ef7dd026a5390225149da3d76b06a2a372c009
Reviewed-on: https://go-review.googlesource.com/c/net/+/749265
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>
TestRoundTripRequestBodyErrors/read_error currently has a small chance
of failing due to a race condition.
Most of the time, within writeBodyAndTrailer which runs concurrently,
when a request body returns an error on Read, rt.abort will be called
with the Read error. Since rt.abort is idempotent, the Read error will
then be propagated and the test would pass as expected.
However, before rt.abort is called with the Read error,
rt.reqBodyWriter.Close is first called, which will close the write
direction of the QUIC stream. This creates a small chance where the main
goroutine which runs RoundTrip can encounter an EOF error due to the
closed stream, and subsequently call rt.abort with a different error
than expected, causing test flakiness.
To fix this, this change makes sure that rt.abort is immediately called
when a request body Read returns an error. This should be a no-op, since
a body Read error has always been prioritized over a potential error
from rt.reqBodyWriter.Close anyways.
Fixesgolang/go#77782
Change-Id: I9ec5acf82dd95ac91963ce5c8ddbf08b49af9508
Reviewed-on: https://go-review.googlesource.com/c/net/+/751280
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>
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>
This change implements support for Server to send trailer headers, and
for ClientConn to receive said trailer headers. This is just like
go.dev/cl/743600, but in the opposite direction.
The bulk of the implementation relies on the trailer header encoding and
decoding support that was added to bodyWriter and bodyReader
respectively in go.dev/cl/743600.
For golang/go#70914
Change-Id: I0efded4b1ac3e3c6b9479f18402e02e9e764d4a2
Reviewed-on: https://go-review.googlesource.com/c/net/+/744220
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>
This change adds support for ClientConn to send trailer headers, and for
Server to receive trailer headers. Support for the other direction will
be done in a follow-up change.
The encoding and decoding of trailer headers are done using bodyWriter
and bodyReader respectively, as these logic are agnostic to whether the
trailer header is sent by the client or the server.
For golang/go#70914
Change-Id: I646d193ae1bc44ddea69b8397d4473d3b11eddf2
Reviewed-on: https://go-review.googlesource.com/c/net/+/743600
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>
In HTTP/3, zero or more DATA frames can come after a HEADERS frame to
represent a request or response body. Our current implementation can
behave rather badly when zero DATA frame is sent.
ClientConn does not close the write direction of the stream when it has
no body to send. As a result, our Server can end up reading the next
frame after a HEADERS frame, only to hang infinitely until the timeout
is reached. To fix this, when there is no body to send, ClientConn now
closes the write direction of the stream as soon as it has finished
writing its HEADERS frame. Server will also prevent itself from reading
the stream if a Content-Length header with the value 0 is received.
In the opposite direction (client reading response from a server), a
similar problem also exists, with a slight variant. While our Server
reliably closes its write direction of the stream as soon as the server
handler exits, a problem can still occur when a client receives an empty
response body due to sending a HEAD request. In this case, if the client
decides to read the response body, bodyReader might throw an error due
to a mismatch between the Content-Length header given by the server and
the actual body length. This is fixed by making ClientConn aware that
HEAD requests will always result in an empty response body.
For golang/go#70914
Change-Id: I1e8970672e7076c9dbf84aec8808632d04bac807
Reviewed-on: https://go-review.googlesource.com/c/net/+/742960
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>
While running net/http tests against our HTTP/3 implementation locally,
some tests fail due to slight behavior differences in responseWriter
compared to other http.ResponseWriter implementations:
- responseWriter does not return a 200 OK response if a server handler
is completely empty.
- responseWriter does not have a Flush method, and therefore does not
implement http.Flusher.
There are surely more differences, but these are straightforward to fix
right now.
For golang/go#70914
Change-Id: Ieb729a4de4ccb55d670eac2369e73c240b9ac8f8
Reviewed-on: https://go-review.googlesource.com/c/net/+/741720
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>
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>
Currently, we are passing a very barebone http.Request to the server
handler: we only initialize an empty http.Request and put whatever info
we can get while decoding QPACK headers.
Unfortunately, this causes the Server to panic when parsing requests
whose headers are meant to be written to http.Request.URL, as
http.Request.URL was never initialized.
Therefore, make sure that http.Request.URL is initialized. Also,
populate other http.Request fields that we can easily figure out as of
now.
For golang/go#70914
Change-Id: Ie6552d6678b430fe4b51069616c0e366791c4e34
Reviewed-on: https://go-review.googlesource.com/c/net/+/738880
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>
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>
internal/http3 currently depends on internal/quic/quicwire only for its
SizeVarint when writing SETTINGS frame. This makes bundling
internal/http3 into std more complicated since it forces us to also
bundle internal/quic/quicwire and do import remapping. This CL breaks
this dependency for easier bundling.
Dependency on internal/quic/quicwire in test codes have been left alone
since they do not affect bundling into std, and uses more than just
SizeVarint.
Change-Id: I2b85c5487ae6cce422deb4509f68f932d3f0de6b
Reviewed-on: https://go-review.googlesource.com/c/net/+/738060
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>
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>
