From: Bryan C. Mills <bcmills@google.com>
Date: Tue, 25 Aug 2020 18:01:27 +0000 (+0000)
Subject: Revert "net/http: fix data race due to writeLoop goroutine left running"
X-Git-Tag: go1.16beta1~1212
X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=8d31ca255bd6b00d04f1673d26110b702e96662b;p=gostls13.git

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 05ff3ba1c2..d37b52b13d 100644
--- 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 29d1ec3f46..2d9ca10bf0 100644
--- 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)
-	}
-}