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

runtime/race: false positive in race detector #13096

Closed
bradfitz opened this issue Oct 29, 2015 · 3 comments
Closed

runtime/race: false positive in race detector #13096

bradfitz opened this issue Oct 29, 2015 · 3 comments
Labels
FrozenDueToAge
Milestone
Go1.6

Comments

@bradfitz
Copy link
Contributor

I'm seeing what appears to be a false positive in the race detector on Linux in the x/net/http2 tests:

=== RUN   TestServerWithCurl_LenientCipherSuites
==================
WARNING: DATA RACE
Write by goroutine 40:
  sync/atomic.StoreInt32()
      /home/bradfitz/go/src/runtime/race_amd64.s:215 +0xb
  golang.org/x/net/http2.testServerWithCurl.func2()
      /home/bradfitz/src/golang.org/x/net/http2/server_test.go:2228 +0x35
  golang.org/x/net/http2.ConfigureServer.func1()
      /home/bradfitz/src/golang.org/x/net/http2/server.go:191 +0x67
  net/http.(*conn).serve()
      /home/bradfitz/go/src/net/http/server.go:1323 +0xa95

Previous write by goroutine 31:
  golang.org/x/net/http2.testServerWithCurl()
      /home/bradfitz/src/golang.org/x/net/http2/server_test.go:2227 +0x343
  golang.org/x/net/http2.TestServerWithCurl_LenientCipherSuites()
      /home/bradfitz/src/golang.org/x/net/http2/server_test.go:2204 +0x33
  testing.tRunner()
      /home/bradfitz/go/src/testing/testing.go:456 +0xdc

Goroutine 40 (running) created at:
  net/http.(*Server).Serve()
      /home/bradfitz/go/src/net/http/server.go:1930 +0x533
  net/http/httptest.(*Server).goServe.func1()
      /home/bradfitz/go/src/net/http/httptest/server.go:215 +0xac

Goroutine 31 (running) created at:
  testing.RunTests()
      /home/bradfitz/go/src/testing/testing.go:561 +0xaaf
  testing.(*M).Run()
      /home/bradfitz/go/src/testing/testing.go:494 +0xe4
  main.main()
      golang.org/x/net/http2/_test/_testmain.go:272 +0x210
==================
--- PASS: TestServerWithCurl_LenientCipherSuites (0.78s)
        server_test.go:2230: Running test server for curl to hit at: https://127.0.0.1:37619
PASS

Except those two write lines, 2227 and 2228 are:

        var gotConn int32
        testHookOnConn = func() { atomic.StoreInt32(&gotConn, 1) }

Is the implicit zeroing of that variable counting as a write?

The only reference to that variable is at the end of the test:

        if atomic.LoadInt32(&gotConn) == 0 {
                t.Error("never saw an http2 connection")
        }

/cc @randall77
@bradfitz bradfitz added this to the Go1.6 milestone Oct 29, 2015
@dvyukov
Copy link
Member

dvyukov commented Nov 3, 2015

The test starts server and after that sets testHookOnConn. It is OK since curl is not started yet, but race detector does not see the synchronization behind starting curl and accepting a new connection. Can you just move setup of testHookOnConn above start of the server? It should get rid of the race. Can't test it as it requires some docker magic that does not work on my machine.

However, there seems to be a false negative there. If race detector sees a race here:

var gotConn int32
testHookOnConn = func() { atomic.StoreInt32(&gotConn, 1) }

then it probably should also see a race on testHookOnConn var.

@dvyukov
Copy link
Member

dvyukov commented Nov 3, 2015

Are you sure you don't see a race on testHookOnConn?
Race detector has similar tests and races on func vars are reported. I've modeled what [race detector thinks] happens in the test, and it detects races on both x and testf:

var testf func()
func TestRaceGlobalFuncVariableRW(t *testing.T) {
    ch := make(chan bool, 1)
    go func() {
        if testf != nil {
            testf()
        }
    }()
    var x int32
    testf = func() {
        atomic.AddInt32(&x, 1)
    }
    ch <- true
    <-ch
}

@rsc
Copy link
Contributor

rsc commented Dec 5, 2015

I agree with Dmitriy that this is an accurate race report. Move the setup above ConfigureServer.

I also agree that you should be seeing a race on testHookOnConn itself. Do you?

@rsc rsc closed this as completed Dec 5, 2015
@golang golang locked and limited conversation to collaborators Dec 14, 2016
@rsc rsc unassigned dvyukov Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.6
Development

No branches or pull requests
4 participants
@bradfitz @rsc @dvyukov @gopherbot