net/http: wait forever for write results in tests

After performing a round trip on a connection, the connection is
usually returned to the idle connection pool. If the write of the
request did not complete successfully, the connection is not
returned.

It is possible for the response to be read before the write
goroutine has finished signalling that its write has completed.
To allow for this, the check to see if the write completed successfully
waits for 50ms for the write goroutine to report the result of the
write.

See comments in persistConn.wroteRequest for more details.

On a slow builder, it is possible for the write goroutine to take
longer than 50ms to report the status of its write, leading to test
flakiness when successive requests unexpectedly use different connections.

Set the timeout for waiting for the writer to an effectively
infinite duration in tests.

Fixes #51147
Fixes #56275
Fixes #56419
Fixes #56577
Fixes #57375
Fixes #57417
Fixes #57476
Fixes #57604
Fixes #57605

Change-Id: I5e92ffd66b676f3f976d8832c0910f27456a6991
Reviewed-on: https://go-review.googlesource.com/c/go/+/483116
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>

diff --git a/src/net/http/export_test.go b/src/net/http/export_test.go
index 8a61e651dcd61b021aff2218925d7d8fc27b96e5..5d198f3f8947d1e33de2d2865e499d1bfdbcb3fd 100644 (file)
--- a/src/net/http/export_test.go
+++ b/src/net/http/export_test.go
@@ -36,7 +36,7 @@ var (
        Export_is408Message               = is408Message
 )
 
-const MaxWriteWaitBeforeConnReuse = maxWriteWaitBeforeConnReuse
+var MaxWriteWaitBeforeConnReuse = &maxWriteWaitBeforeConnReuse
 
 func init() {
        // We only want to pay for this cost during testing.

diff --git a/src/net/http/main_test.go b/src/net/http/main_test.go
index 1b2fa215ff702784fd08af7e3f0abf952ae725c4..1e83ca3c0ac5caf0fca9f8631f3c9741784a6a1e 100644 (file)
--- a/src/net/http/main_test.go
+++ b/src/net/http/main_test.go
@@ -20,6 +20,7 @@ import (
 var quietLog = log.New(io.Discard, "", 0)
 
 func TestMain(m *testing.M) {
+       *http.MaxWriteWaitBeforeConnReuse = 60 * time.Minute
        v := m.Run()
        if v == 0 && goroutineLeaked() {
                os.Exit(1)

diff --git a/src/net/http/transport.go b/src/net/http/transport.go
index 807cc8f0ebb098104845a0d87c05bfdfd4387a3f..8de63cdb8857d4eef7fa6338e321d2b317566549 100644 (file)
--- a/src/net/http/transport.go
+++ b/src/net/http/transport.go
@@ -2452,7 +2452,10 @@ func (pc *persistConn) writeLoop() {
 // maxWriteWaitBeforeConnReuse is how long the a Transport RoundTrip
 // will wait to see the Request's Body.Write result after getting a
 // response from the server. See comments in (*persistConn).wroteRequest.
-const maxWriteWaitBeforeConnReuse = 50 * time.Millisecond
+//
+// In tests, we set this to a large value to avoid flakiness from inconsistent
+// recycling of connections.
+var maxWriteWaitBeforeConnReuse = 50 * time.Millisecond
 
 // wroteRequest is a check before recycling a connection that the previous write
 // (from writeLoop above) happened and was successful.

diff --git a/src/net/http/transport_test.go b/src/net/http/transport_test.go
index 268b0a47761985ceb25b50c7edda964ee4efe571..6f57629eff892ac6b20a9e5defb913b4d3d4e255 100644 (file)
--- a/src/net/http/transport_test.go
+++ b/src/net/http/transport_test.go
@@ -3402,9 +3402,13 @@ func (c byteFromChanReader) Read(p []byte) (n int, err error) {
 // questionable state.
 // golang.org/issue/7569
 func TestTransportNoReuseAfterEarlyResponse(t *testing.T) {
-       run(t, testTransportNoReuseAfterEarlyResponse, []testMode{http1Mode})
+       run(t, testTransportNoReuseAfterEarlyResponse, []testMode{http1Mode}, testNotParallel)
 }
 func testTransportNoReuseAfterEarlyResponse(t *testing.T, mode testMode) {
+       defer func(d time.Duration) {
+               *MaxWriteWaitBeforeConnReuse = d
+       }(*MaxWriteWaitBeforeConnReuse)
+       *MaxWriteWaitBeforeConnReuse = 10 * time.Millisecond
        var sconn struct {
                sync.Mutex
                c net.Conn
@@ -3631,13 +3635,13 @@ func testRetryRequestsOnError(t *testing.T, mode testMode) {
                                req := tc.req()
                                res, err := c.Do(req)
                                if err != nil {
-                                       if time.Since(t0) < MaxWriteWaitBeforeConnReuse/2 {
+                                       if time.Since(t0) < *MaxWriteWaitBeforeConnReuse/2 {
                                                mu.Lock()
                                                got := logbuf.String()
                                                mu.Unlock()
                                                t.Fatalf("i=%d: Do = %v; log:\n%s", i, err, got)
                                        }
-                                       t.Skipf("connection likely wasn't recycled within %d, interfering with actual test; skipping", MaxWriteWaitBeforeConnReuse)
+                                       t.Skipf("connection likely wasn't recycled within %d, interfering with actual test; skipping", *MaxWriteWaitBeforeConnReuse)
                                }
                                res.Body.Close()
                                if res.Request != req {