diff options
author | Steven Hartland <steven.hartland@multiplay.co.uk> | 2020-05-07 21:12:21 +0000 |
---|---|---|
committer | Bryan C. Mills <bcmills@google.com> | 2020-08-25 17:37:38 +0000 |
commit | 5e1e8c4c9f99da52419c99e618425794102c9769 (patch) | |
tree | 8c5c7f091b900732981d34304fee4a23f11baa32 /src/net/http/transport_test.go | |
parent | 91a52de5274a13fcaab68c0a78115eff632f68fc (diff) | |
download | go-5e1e8c4c9f99da52419c99e618425794102c9769.tar.gz go-5e1e8c4c9f99da52419c99e618425794102c9769.zip |
net/http: fix data race due to writeLoop goroutine left running
Fix a data race for clients that mutate requests after receiving a
response error which is caused by the writeLoop goroutine left
running, this can be seen on canceled requests.
Fixes #37669
Change-Id: I0e0e4fd63266326b32587d8596456760bf848b13
Reviewed-on: https://go-review.googlesource.com/c/go/+/232799
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Diffstat (limited to 'src/net/http/transport_test.go')
-rw-r--r-- | src/net/http/transport_test.go | 92 |
1 files changed, 92 insertions, 0 deletions
diff --git a/src/net/http/transport_test.go b/src/net/http/transport_test.go index 2d9ca10bf0..29d1ec3f46 100644 --- a/src/net/http/transport_test.go +++ b/src/net/http/transport_test.go @@ -25,6 +25,7 @@ import ( "io" "io/ioutil" "log" + mrand "math/rand" "net" . "net/http" "net/http/httptest" @@ -6284,3 +6285,94 @@ func TestTransportRejectsSignInContentLength(t *testing.T) { t.Fatalf("Error mismatch\nGot: %q\nWanted substring: %q", got, want) } } + +// dumpConn is a net.Conn which writes to Writer and reads from Reader +type dumpConn struct { + io.Writer + io.Reader +} + +func (c *dumpConn) Close() error { return nil } +func (c *dumpConn) LocalAddr() net.Addr { return nil } +func (c *dumpConn) RemoteAddr() net.Addr { return nil } +func (c *dumpConn) SetDeadline(t time.Time) error { return nil } +func (c *dumpConn) SetReadDeadline(t time.Time) error { return nil } +func (c *dumpConn) SetWriteDeadline(t time.Time) error { return nil } + +// delegateReader is a reader that delegates to another reader, +// once it arrives on a channel. +type delegateReader struct { + c chan io.Reader + r io.Reader // nil until received from c +} + +func (r *delegateReader) Read(p []byte) (int, error) { + if r.r == nil { + r.r = <-r.c + } + return r.r.Read(p) +} + +func testTransportRace(req *Request) { + save := req.Body + pr, pw := io.Pipe() + defer pr.Close() + defer pw.Close() + dr := &delegateReader{c: make(chan io.Reader)} + + t := &Transport{ + Dial: func(net, addr string) (net.Conn, error) { + return &dumpConn{pw, dr}, nil + }, + } + defer t.CloseIdleConnections() + + quitReadCh := make(chan struct{}) + // Wait for the request before replying with a dummy response: + go func() { + defer close(quitReadCh) + + req, err := ReadRequest(bufio.NewReader(pr)) + if err == nil { + // Ensure all the body is read; otherwise + // we'll get a partial dump. + io.Copy(ioutil.Discard, req.Body) + req.Body.Close() + } + select { + case dr.c <- strings.NewReader("HTTP/1.1 204 No Content\r\nConnection: close\r\n\r\n"): + case quitReadCh <- struct{}{}: + } + }() + + t.RoundTrip(req) + + // Ensure the reader returns before we reset req.Body to prevent + // a data race on req.Body. + pw.Close() + <-quitReadCh + + req.Body = save +} + +// Issue 37669 +// Test that a cancellation doesn't result in a data race due to the writeLoop +// goroutine being left running, if the caller mutates the processed Request +// upon completion. +func TestErrorWriteLoopRace(t *testing.T) { + for i := 0; i < 1000; i++ { + ctx, cancel := context.WithCancel(context.Background()) + r := bytes.NewBuffer(make([]byte, 10000)) + delay := time.Duration(mrand.Intn(5)) * time.Millisecond + go func() { + time.Sleep(delay) + cancel() + }() + req, err := NewRequestWithContext(ctx, MethodPost, "http://example.com", r) + if err != nil { + t.Fatal(err) + } + + testTransportRace(req) + } +} |