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

Jump to bottom

net/http: data race in test #11136

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

net/http: data race in test #11136

bradfitz opened this issue Jun 10, 2015 · 4 comments
Labels
FrozenDueToAge Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Go1.5

Comments

@bradfitz
Copy link
Contributor

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
@tw4452852
Copy link
Contributor

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
Copy link
Contributor Author

I wish I could reproduce this.

@tzneal
Copy link
Member

tzneal 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
Copy link

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

@bradfitz bradfitz added the Testing An issue that has been verified to require only test changes, not just a test failure. label 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.
Assignees
No one assigned
Labels
FrozenDueToAge Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Go1.5
Development

No branches or pull requests
4 participants
@bradfitz @tzneal @tw4452852 @gopherbot