In golang/go#37319, TestPacketConnReadWriteMulticastUDP was observed
to occasionally fail with ENOBUFS on macOS.
This change adds a retry loop for that test function. There are some
other related test functions that may also wrap sendmsg, but I have
not observed any ENOBUFS failures for them — I suspect that some
difference in protocol or traffic class prevents this failure mode,
but we can always add more retry loops if we discover that they are
actually needed.
Fixesgolang/go#37319.
Change-Id: I99fce94ff10c6f3c09d493712eba782ec8707a58
Reviewed-on: https://go-review.googlesource.com/c/net/+/369742
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
These deadlines were added back in CL 21080043, apparently in an
attempt to increase code coverage numbers. However, nothing in the
tests actually exercises the deadline logic: the tests would pass even
if these methods were no-ops. Their only apparent effect is to make
the tests flaky on slower builders, and to destroy goroutine traces
if the test should ever happen to deadlock.
Updates golang/go#42064
For golang/go#37319
Change-Id: I530a8f3cb80d6d93d1625bc88f0ec7958d4ec35e
Reviewed-on: https://go-review.googlesource.com/c/net/+/366181
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
TestUDP/Messages and TestUDP/Messages-dialed occasionally failed because
the expected messages were not received in a single RecvMsgs call, or
because the messages were received out of order.
Assuming that both messages are returned immediately from a single
RecvMsgs call was a flawed expectation. Fixed by repeatedly invoking
RecvMsgs until all expected messages have been received.
While it certainly seems unusual that packets are reordered on a
loopback device, it does appear to happen occasionally (on linux-mips).
Fixed by sizing receive buffers such that messages in any order can be
received correctly, and by allowing either order for the reassembled
message.
Combine "Messages" and "Messages-dialed" subtests with a simple
table-driven test, to avoid the repetition. The same "Message" and
"Message-dialed".
Finally, make the test failure messages slightly more useful.
Fixesgolang/go#49385
Change-Id: I04463c6ffdf4865d2ccfb8662ab4660bda3b3cbf
GitHub-Last-Rev: d9df27b967
GitHub-Pull-Request: golang/net#119
Reviewed-on: https://go-review.googlesource.com/c/net/+/368094
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Instead of relying on syscall.Syscall, always reach into the syscall
package to call the appropriate functions on Unix systems. We were
already doing this on Darwin and AIX. We also have to do this on
OpenBSD, and it's simpler to do it on Linux 386 and s390x. Rather than
sometimes reach into syscall and sometimes not, just always reach in.
For golang/go#42064
Change-Id: I0adb1c7cc623f2c1247465b3852fefd8d09975d2
Reviewed-on: https://go-review.googlesource.com/c/net/+/366195
Trust: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Instead of relying on syscall.Syscall, always reach into the syscall
package to call the appropriate functions on Unix systems. We were
already doing this on Darwin. We also have to do this on
OpenBSD. Rather than sometimes reach into syscall and sometimes not,
just always reach in.
For golang/go#42064
Change-Id: Ie292a56766080d0c5ae6b6723d42b5475128290c
Reviewed-on: https://go-review.googlesource.com/c/net/+/366354
Trust: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
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>
