Revert "net/http: fix data race due to writeLoop goroutine left running"
This reverts CL 232799.
Reason for revert: net/http test is failing on all longtest builders.
Change-Id: I4694e34f35419bab2d0b45fa6d8c3ac2aa1f51a0
Reviewed-on: https://go-review.googlesource.com/c/go/+/250597
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
diff --git a/src/net/http/transport.go b/src/net/http/transport.go
index 05ff3ba..d37b52b 100644
--- a/src/net/http/transport.go
+++ b/src/net/http/transport.go
@@ -1963,15 +1963,6 @@
return nil
}
- // Wait for the writeLoop goroutine to terminate to avoid data
- // races on callers who mutate the request on failure.
- //
- // When resc in pc.roundTrip and hence rc.ch receives a responseAndError
- // with a non-nil error it implies that the persistConn is either closed
- // or closing. Waiting on pc.writeLoopDone is hence safe as all callers
- // close closech which in turn ensures writeLoop returns.
- <-pc.writeLoopDone
-
// If the request was canceled, that's better than network
// failures that were likely the result of tearing down the
// connection.
@@ -1997,6 +1988,7 @@
return err
}
if pc.isBroken() {
+ <-pc.writeLoopDone
if pc.nwrite == startBytesWritten {
return nothingWrittenError{err}
}
diff --git a/src/net/http/transport_test.go b/src/net/http/transport_test.go
index 29d1ec3..2d9ca10 100644
--- a/src/net/http/transport_test.go
+++ b/src/net/http/transport_test.go
@@ -25,7 +25,6 @@
"io"
"io/ioutil"
"log"
- mrand "math/rand"
"net"
. "net/http"
"net/http/httptest"
@@ -6285,94 +6284,3 @@
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)
- }
-}
