While no guarantees are made about the safety of repeated or
concurrent closes of a request body, HTTP/1 request bodies are
concurrency-safe and HTTP/2 ones should be as well.
Fixesgolang/go#51197
Change-Id: Id6527dc2932579cabc9cbe921c6e0b3b4a3d472c
Reviewed-on: https://go-review.googlesource.com/c/net/+/386495
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
This failure mode has been observed on openbsd/mips64 and
openbsd/arm64.
I have not diagnosed the precise root cause, but I suspect a platform
bug — perhaps a bad interaction with the relatively weakly-ordered
memory models on these CPUs.
For golang/go#52208.
Change-Id: I0ab0285cc395d22742ced8f28d5c9c3280fcd1e3
Reviewed-on: https://go-review.googlesource.com/c/net/+/398794
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Closing a *tls.Conn can write to the network, blocking indefinitely.
Don't hold cc.mu while closing Conns. If closing a *tls.Conn takes too
long (arbitrarily set at 250ms), try to acquire the underlying net.Conn
(Go 1.18+) and close it instead.
Change-Id: Ib4e4a26595108eba9cd767231c727fd9412658b5
Reviewed-on: https://go-review.googlesource.com/c/net/+/359636
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Trust: Brad Fitzpatrick <bradfitz@golang.org>
This test constructed a new serverTester for each subtest, but reused
a Transport across all of the tests. I don't fully understand why, but
for some reason that occasionally led to "connection reset by peer"
failures when the port was reused.
Scoping the Transport to the lifetime of the serverTester seems to
resolve the errors: I tested with
go test ./http2 -run=TestContentEncodingNoSniffing -httptest.serve=127.0.0.1:8080 -count=10000
and got lots of failures prior to this change and none after.
Fixesgolang/go#46762
(I hope.)
Change-Id: I385c077c1d8627e42ce4d2db041878aacb5452fb
Reviewed-on: https://go-review.googlesource.com/c/net/+/380154
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
This test makes assumptions about the internal structure of *clientConnPool,
as well as assuming that a goroutine will schedule and run within one
second. The former assumption isn't necessary, and the latter causes
flakiness.
Refactor the test to count dial and close calls, which is all it needs
to test the desired behavior (one pending dial per destination).
Fixesgolang/go#43176.
Change-Id: I125b110f196e29f303960c6851089118f8fb5d38
Reviewed-on: https://go-review.googlesource.com/c/net/+/370175
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
The http2 random write scheduler should not queue RST_STREAM
frames with the DATA frames, and instead treat them as control frames.
There can be deadlock situations if data frames block the queue,
because if the sender wants to close the stream it sends an RST frame,
but if the client is not draining the queue, the RST frame is stuck
and the sender is not able to finish.
Fixesgolang/go#49741
Change-Id: I0940a76d1aad95f1c4d3856e4d79cf5ce2a78ff2
Reviewed-on: https://go-review.googlesource.com/c/net/+/367154
Trust: Dave Cheney <dave@cheney.net>
Reviewed-by: Damien Neil <dneil@google.com>
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Avoid reusing connections after receiving a response to a request
for which cc.Close is set or a "Connection: close" header is present.
Adjust the tests for connection reuse to test both the case where
new conns are created by the http2 package and when they are
created by the net/http package.
Fixesgolang/go#49375
Change-Id: I58594972c832a20b66a3910c17acb51a98a9f7a5
Reviewed-on: https://go-review.googlesource.com/c/net/+/361498
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>
When a request has a deadline set, the net/http package can cancel
the request context after reading an error (including io.EOF) from
the response body. Avoid returning a spurious error from
transportResponseBody if the request context is canceled.
For golang/go#49366.
Change-Id: I645466a1d6e405bdf17b3cc087204e4622a140eb
Reviewed-on: https://go-review.googlesource.com/c/net/+/362354
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>
Deflake TestClientConnCloseAtBody, which assumes all streams
have been fully cleaned up after ClientConn.Close returns.
Only count active streams, not ones in the process of aborting.
Perhaps ClientConn.Close should block until all streams have
been cleaned up, although this could result in Close blocking
indefinitely if a stream is blocked reading from the request body.
Fixesgolang/go#49335.
Change-Id: I172e0d3f10875191d29b24598de0abbdeb35e434
Reviewed-on: https://go-review.googlesource.com/c/net/+/361536
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
When the client gets an END_STREAM from the peer, it causes the
response body to return io.EOF and closes cs.peerClosed.
It is possible for the caller of RoundTrip to read io.EOF from the
response body and end the request by calling Response.Body.Close
before we close cs.peerClosed. In this case, we send a spurious
RST_STREAM to the peer.
Closing cs.peerClosed first reverses the race: This can cause the
response body to return "request canceled" rather than io.EOF.
Close both streams with the connection mutex held.
Fixesgolang/go#49314
Change-Id: I75557670497c96dd6ed1b566bb4f0f3106a0c9f9
Reviewed-on: https://go-review.googlesource.com/c/net/+/361267
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>
We close the request body after receiving an error status code.
This is supposed to cause cs.writeRequestBody to return
errStopReqBodyWrite. Ensure that it does so if it gets an error
reading from the post-close body, rather than returning an
error which causes the entire request to be aborted.
Change-Id: I7c51928cb678f5baf37148f0df6ab196518d39d4
Reviewed-on: https://go-review.googlesource.com/c/net/+/356969
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>
During this test:
-> client sends req1
<- server sends req1 headers
-> client sends req2
<- server resets req2
<- server sends END_STREAM for req1
-> client closes the *server* connection
The flake occurs when the last two steps happen out of order: The
client processes the reset for req2, returns from RoundTrip, and
the test closes the connection before the server writes the last
DATA frame for req1. This can be demonstrated by inserting a 1ms
sleep between WriteRSTStream and WriteData on the server side of
the test.
It isn't clear why the client side of the test is closing the server
side of the connection; it isn't essential to the test in any way.
Remove it.
Fixesgolang/go#48994.
Change-Id: If074e45a76205ebd245617ef1b068967a6674b94
Reviewed-on: https://go-review.googlesource.com/c/net/+/356329
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Don't send the END_STREAM flag from the server until the client
sends its END_STREAM. Avoids a flaky failure when the client
sends the END_STREAM in a zero-length DATA frame:
- The server reads bodySize bytes and half-closes the stream.
- The client's Response.Body returns EOF.
- The test calls Response.Body.Close.
- The client resets the stream.
Avoid hanging forever on the client side of the test if the
server side exits with an error.
Fixesgolang/go#48970
Change-Id: Ic921a3f60149abbb5434ec7a53985becea7b23c3
Reviewed-on: https://go-review.googlesource.com/c/net/+/355649
Trust: Damien Neil <dneil@google.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
After RoundTrip returns, closing the Response's Body should
interrupt any ongoing write of the request body. Close the
Request's Body to unblock the body writer.
Take additional care around the use of a Request after
its Response's Body has been closed. The RoundTripper contract
permits the caller to modify the request after the Response's
body has been closed.
Updates golang/go#48908.
Change-Id: I261e08eb5d70016b49942d72833f46b2ae83962a
Reviewed-on: https://go-review.googlesource.com/c/net/+/355491
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
buildRootHuffmanNode() was allocating a new node for every leaf of the huffman decoding tree.
This PR allocates an array of nodes for all the leaf nodes, and reuses them.
As buildRootHuffmanNode is only called via sync.Once, it's not significant performance wise, but still seems an unnecessary number of allocations.
benchmark old ns/op new ns/op delta
BenchmarkRoot-4 162847 17911 -89.00%
benchmark old allocs new allocs delta
BenchmarkRoot-4 3852 31 -99.20%
benchmark old bytes new bytes delta
BenchmarkRoot-4 92112 35056 -61.94%
Change-Id: I7c1eae13fb6130090610eec1eb0347d5d1fef20c
GitHub-Last-Rev: f40a4e8530
GitHub-Pull-Request: golang/net#112
Reviewed-on: https://go-review.googlesource.com/c/net/+/346649
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>
We start the PingTimeout timer before writing a PING frame.
However, writing the frame can block indefinitely (either
acquiring cc.wmu or writing to the conn), and the timer is
not checked until after the frame is written.
Move PING writes into a separate goroutine, so we can detect
write-blocked connections.
Fixesgolang/go#48810.
Change-Id: Ifd67fa23799e750d02754e1fe5d32719f60faed4
Reviewed-on: https://go-review.googlesource.com/c/net/+/354389
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
This test sends a request with a 10MiB body, reads a 403 response
while the body is still being written, and closes the response body.
It then verifies that the full request body was not written, since
reading a response code >=300 interrupts body writes.
This can be racy: We process the status code and interrupt the body
write in RoundTrip, but it is possible for the body write to complete
before RoundTrip looks at the response.
Adjust the test to have more control over the request body:
Only provide half the Request.Body until after the response headers
have been received.
Fixesgolang/go#48792.
Change-Id: Id4802b04a50f34f6af28f4eb93e37ef70a33a068
Reviewed-on: https://go-review.googlesource.com/c/net/+/354130
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
CL 352469 inverts the case in shouldTraceGetConn: We want to call GetConn
for connections that have been previously used, but it calls GetConn
only on approximately the first use. "Approximately", because it uses
cc.nextStreamID to determine if the connection has been used, which
is racy.
Restructure the decision to call GetConn to track a per-ClientConn bool
indicating whether GetConn has already been called for this connection.
Set this bool for connections received from net/http, clear it after the
first use of the connection.
Fixes net/http's TestTransportEventTrace_h2.
Change-Id: I8e3dbba7cfbce9acd3612e39b6b6ee558bbfc864
Reviewed-on: https://go-review.googlesource.com/c/net/+/353875
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>
Move the entire request write into a new writeRequest function,
which runs as its own goroutine.
The writeRequest function handles all indefintely-blocking
operations (in particular, network writes), as well as all
post-request cleanup: Closing the request body, sending a
RST_STREAM when necessary, releasing the concurrency slot
held by the stream, etc.
Consolidates several goroutines used to wait for stream
slots, write the body, and close response bodies.
Ensures that RoundTrip does not block past request cancelation.
Change-Id: Iaf8bb3e17de89384b031ec4f324918b5720f5877
Reviewed-on: https://go-review.googlesource.com/c/net/+/353390
Trust: Damien Neil <dneil@google.com>
Trust: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
