Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net/http: data race in test #11136

Closed
bradfitz opened this issue Jun 10, 2015 · 4 comments

Comments

Projects
None yet
4 participants
@bradfitz
Copy link
Member

commented Jun 10, 2015

Test-only data race I saw once in a trybot:

==================
WARNING: DATA RACE
Write by goroutine 127:
  net/http_test.TestTransportConcurrency()
      /tmp/workdir/go/src/net/http/transport_test.go:1104 +0x345
  testing.tRunner()
      /tmp/workdir/go/src/testing/testing.go:455 +0xdc

Previous read by goroutine 146:
  net/http.(*Transport).getConn.func2.1()
      /tmp/workdir/go/src/net/http/transport.go:534 +0xa0

Goroutine 127 (running) created at:
  testing.RunTests()
      /tmp/workdir/go/src/testing/testing.go:563 +0xcac
  testing.(*M).Run()
      /tmp/workdir/go/src/testing/testing.go:493 +0xe4
  net/http_test.TestMain()
      /tmp/workdir/go/src/net/http/main_test.go:19 +0x2e
  main.main()
      net/http/_test/_testmain.go:538 +0x209

Goroutine 146 (finished) created at:
  net/http.(*Transport).getConn.func2()
      /tmp/workdir/go/src/net/http/transport.go:537 +0x9c
  net/http.(*Transport).getConn()
      /tmp/workdir/go/src/net/http/transport.go:559 +0x45e
  net/http.(*Transport).RoundTrip()
      /tmp/workdir/go/src/net/http/transport.go:228 +0x62a
  net/http.send()
      /tmp/workdir/go/src/net/http/client.go:220 +0x6e4
  net/http.(*Client).send()
      /tmp/workdir/go/src/net/http/client.go:143 +0x1f7
  net/http.(*Client).doFollowingRedirects()
      /tmp/workdir/go/src/net/http/client.go:380 +0x1007
  net/http.(*Client).Get()
      /tmp/workdir/go/src/net/http/client.go:306 +0xc8
  net/http_test.TestChunkedNoContent()
      /tmp/workdir/go/src/net/http/transport_test.go:1071 +0x32a
  testing.tRunner()
      /tmp/workdir/go/src/testing/testing.go:455 +0xdc
==================
PASS
Found 1 data race(s)
FAIL    net/http    13.506s

Looks like TestChunkedNoContent isn't waiting for its Transport connections to die before a later test changes hooks.

@bradfitz bradfitz self-assigned this Jun 10, 2015

@bradfitz bradfitz added this to the Go1.5 milestone Jun 10, 2015

@bradfitz bradfitz added the RaceReport label Jun 10, 2015

@tw4452852

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2015

I think race condition is between the goroutine in the handlePendingDial and the next new transport. We didn't wait goroutine in the handlePendingDial to exit.
I'm sorry I can't access go.googlesource.com so I just upload an alternative patch here:

diff --git a/src/net/http/transport.go b/src/net/http/transport.go
index 5de5d94..30e6469 100644
--- a/src/net/http/transport.go
+++ b/src/net/http/transport.go
@@ -523,7 +523,7 @@ func (t *Transport) getConn(req *Request, cm connectMethod) (*persistConn, error
    }
    dialc := make(chan dialRes)

-   handlePendingDial := func() {
+   handlePendingDial := func(prePendingDial, postPendingDial func()) {
        if prePendingDial != nil {
            prePendingDial()
        }
@@ -556,10 +556,10 @@ func (t *Transport) getConn(req *Request, cm connectMethod) (*persistConn, error
        // else's dial that they didn't use.
        // But our dial is still going, so give it away
        // when it finishes:
-       handlePendingDial()
+       handlePendingDial(prePendingDial, postPendingDial)
        return pc, nil
    case <-cancelc:
-       handlePendingDial()
+       handlePendingDial(prePendingDial, postPendingDial)
        return nil, errors.New("net/http: request canceled while waiting for connection")
    }
 }
@bradfitz

This comment has been minimized.

Copy link
Member Author

commented Jun 10, 2015

I wish I could reproduce this.

@tzneal

This comment has been minimized.

Copy link
Member

commented Jun 18, 2015

I can reproduce it reliably with this change:

diff --git a/src/net/http/transport.go b/src/net/http/transport.go
index 5de5d94..0c305cf 100644
--- a/src/net/http/transport.go
+++ b/src/net/http/transport.go
@@ -531,6 +531,11 @@ func (t *Transport) getConn(req *Request, cm connectMethod) (*persistConn, error
            if v := <-dialc; v.err == nil {
                t.putIdleConn(v.pc)
            }
+           for i := 0; i < 10; i++ {
+               prePendingDial, postPendingDial = postPendingDial, prePendingDial
+               time.Sleep(10 * time.Millisecond)
+               prePendingDial, postPendingDial = postPendingDial, prePendingDial
+           }
            if postPendingDial != nil {
                postPendingDial()
            }

https://go-review.googlesource.com/#/c/11250/

@gopherbot

This comment has been minimized.

Copy link

commented Jun 18, 2015

CL https://golang.org/cl/11250 mentions this issue.

@bradfitz bradfitz added the Testing label Jun 28, 2015

@bradfitz bradfitz closed this in 7511806 Jun 28, 2015

@golang golang locked and limited conversation to collaborators Jun 27, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.