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 05ff3ba1c2a84b13253d5e80ab6514ca5bfaa5be..d37b52b13d06006619d341692680bf13cc1a4d52 100644 (file)
--- a/src/net/http/transport.go
+++ b/src/net/http/transport.go
@@ -1963,15 +1963,6 @@ func (pc *persistConn) mapRoundTripError(req *transportRequest, startBytesWritte
                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 @@ func (pc *persistConn) mapRoundTripError(req *transportRequest, startBytesWritte
                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 29d1ec3f46bf570b23c37c999bcedae64585b295..2d9ca10bf0d1c1eab79ae1c439d74e9b390e882d 100644 (file)
--- a/src/net/http/transport_test.go
+++ b/src/net/http/transport_test.go
@@ -25,7 +25,6 @@ import (
        "io"
        "io/ioutil"
        "log"
-       mrand "math/rand"
        "net"
        . "net/http"
        "net/http/httptest"
@@ -6285,94 +6284,3 @@ 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)
-       }
-}