Skip to content

Commit fc0d9a4

Browse files
Bryan C. Millsgopherbot
Bryan C. Mills
authored andcommitted
net/http: reject client-side retries in server timeout tests
This breaks an unbounded client-side retry loop if the server's timeout happens to fire during its final read of the TLS handshake. The retry loop was observed on wasm platforms at CL 557437. I was also able to reproduce chains of dozens of retries on my linux/amd64 workstation by adjusting some timeouts and adding a couple of sleeps, as in this patch: https://gist.github.com/bcmills/d0a0a57e5f64eebc24e8211d8ea502b3 However, on linux/amd64 on my workstation the test always eventually breaks out of the retry loop due to timing jitter. I couldn't find a retry-specific hook in the http.Client, http.Transport, or tls.Config structs, so I have instead abused the Transport.Proxy hook for this purpose. Separately, we may want to consider adding a retry-specific hook, or changing the net/http implementation to avoid transparently retrying in this case. Fixes #65410. Updates #65178. Change-Id: I0e43c039615fe815f0a4ba99a8813c48b1fdc7e6 Reviewed-on: https://go-review.googlesource.com/c/go/+/559835 Reviewed-by: Damien Neil <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> Auto-Submit: Bryan Mills <[email protected]> Reviewed-by: Michael Pratt <[email protected]>
1 parent f8b4653 commit fc0d9a4

File tree

1 file changed

+46
-0
lines changed

1 file changed

+46
-0
lines changed

src/net/http/serve_test.go

+46
Original file line numberDiff line numberDiff line change
@@ -764,7 +764,17 @@ func testServerReadTimeout(t *testing.T, mode testMode) {
764764
}), func(ts *httptest.Server) {
765765
ts.Config.ReadHeaderTimeout = -1 // don't time out while reading headers
766766
ts.Config.ReadTimeout = timeout
767+
t.Logf("Server.Config.ReadTimeout = %v", timeout)
767768
})
769+
770+
var retries atomic.Int32
771+
cst.c.Transport.(*Transport).Proxy = func(*Request) (*url.URL, error) {
772+
if retries.Add(1) != 1 {
773+
return nil, errors.New("too many retries")
774+
}
775+
return nil, nil
776+
}
777+
768778
pr, pw := io.Pipe()
769779
res, err := cst.c.Post(cst.ts.URL, "text/apocryphal", pr)
770780
if err != nil {
@@ -792,7 +802,34 @@ func testServerWriteTimeout(t *testing.T, mode testMode) {
792802
errc <- err
793803
}), func(ts *httptest.Server) {
794804
ts.Config.WriteTimeout = timeout
805+
t.Logf("Server.Config.WriteTimeout = %v", timeout)
795806
})
807+
808+
// The server's WriteTimeout parameter also applies to reads during the TLS
809+
// handshake. The client makes the last write during the handshake, and if
810+
// the server happens to time out during the read of that write, the client
811+
// may think that the connection was accepted even though the server thinks
812+
// it timed out.
813+
//
814+
// The client only notices that the server connection is gone when it goes
815+
// to actually write the request — and when that fails, it retries
816+
// internally (the same as if the server had closed the connection due to a
817+
// racing idle-timeout).
818+
//
819+
// With unlucky and very stable scheduling (as may be the case with the fake wasm
820+
// net stack), this can result in an infinite retry loop that doesn't
821+
// propagate the error up far enough for us to adjust the WriteTimeout.
822+
//
823+
// To avoid that problem, we explicitly forbid internal retries by rejecting
824+
// them in a Proxy hook in the transport.
825+
var retries atomic.Int32
826+
cst.c.Transport.(*Transport).Proxy = func(*Request) (*url.URL, error) {
827+
if retries.Add(1) != 1 {
828+
return nil, errors.New("too many retries")
829+
}
830+
return nil, nil
831+
}
832+
796833
res, err := cst.c.Get(cst.ts.URL)
797834
if err != nil {
798835
// Probably caused by the write timeout expiring before the handler runs.
@@ -5778,10 +5815,19 @@ func testServerCancelsReadTimeoutWhenIdle(t *testing.T, mode testMode) {
57785815
}
57795816
}), func(ts *httptest.Server) {
57805817
ts.Config.ReadTimeout = timeout
5818+
t.Logf("Server.Config.ReadTimeout = %v", timeout)
57815819
})
57825820
defer cst.close()
57835821
ts := cst.ts
57845822

5823+
var retries atomic.Int32
5824+
cst.c.Transport.(*Transport).Proxy = func(*Request) (*url.URL, error) {
5825+
if retries.Add(1) != 1 {
5826+
return nil, errors.New("too many retries")
5827+
}
5828+
return nil, nil
5829+
}
5830+
57855831
c := ts.Client()
57865832

57875833
res, err := c.Get(ts.URL)

0 commit comments

Comments
 (0)