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

Enable race detection on test run, fix race condition #232

Merged
merged 3 commits into from Mar 8, 2019

Conversation

@bobheadxi
Copy link
Contributor

commented Mar 7, 2019

Please feel free to close this if avoiding race conditions aren't a priority, but I noticed my -race-enabled tests would fail on what seemed like hercules code.

If the build for this PR fails (it does locally on my machine) I'll open a ticket about resolving concurrency issues.

Here's what I get when I run hercules' tests on my machine:

WARNING: DATA RACE
Write at 0x00c0001f2928 by goroutine 42:
  gopkg.in/src-d/hercules.v9/internal/plumbing.(*RenameAnalysis).Consume.func1.1()
      /Users/robertlin/go/src/gopkg.in/src-d/hercules.v9/internal/plumbing/renames.go:215 +0x38
  gopkg.in/src-d/hercules.v9/internal/plumbing.(*RenameAnalysis).Consume.func1()
      /Users/robertlin/go/src/gopkg.in/src-d/hercules.v9/internal/plumbing/renames.go:267 +0x1036
  gopkg.in/src-d/hercules.v9/internal/plumbing.(*RenameAnalysis).Consume.func3()
      /Users/robertlin/go/src/gopkg.in/src-d/hercules.v9/internal/plumbing/renames.go:327 +0x38

Previous write at 0x00c0001f2928 by goroutine 43:
  gopkg.in/src-d/hercules.v9/internal/plumbing.(*RenameAnalysis).Consume.func2.1()
      /Users/robertlin/go/src/gopkg.in/src-d/hercules.v9/internal/plumbing/renames.go:271 +0x38
  gopkg.in/src-d/hercules.v9/internal/plumbing.(*RenameAnalysis).Consume.func2()
      /Users/robertlin/go/src/gopkg.in/src-d/hercules.v9/internal/plumbing/renames.go:322 +0x103b
  gopkg.in/src-d/hercules.v9/internal/plumbing.(*RenameAnalysis).Consume.func4()
      /Users/robertlin/go/src/gopkg.in/src-d/hercules.v9/internal/plumbing/renames.go:328 +0x38

Goroutine 42 (running) created at:
  gopkg.in/src-d/hercules.v9/internal/plumbing.(*RenameAnalysis).Consume()
      /Users/robertlin/go/src/gopkg.in/src-d/hercules.v9/internal/plumbing/renames.go:327 +0x2526
  gopkg.in/src-d/hercules.v9/internal/plumbing.TestRenameAnalysisConsume()
      /Users/robertlin/go/src/gopkg.in/src-d/hercules.v9/internal/plumbing/renames_test.go:126 +0xb26
  testing.tRunner()
      /usr/local/Cellar/go/1.12/libexec/src/testing/testing.go:865 +0x163

Goroutine 43 (finished) created at:
  gopkg.in/src-d/hercules.v9/internal/plumbing.(*RenameAnalysis).Consume()
      /Users/robertlin/go/src/gopkg.in/src-d/hercules.v9/internal/plumbing/renames.go:328 +0x2558
  gopkg.in/src-d/hercules.v9/internal/plumbing.TestRenameAnalysisConsume()
      /Users/robertlin/go/src/gopkg.in/src-d/hercules.v9/internal/plumbing/renames_test.go:126 +0xb26
  testing.tRunner()
      /usr/local/Cellar/go/1.12/libexec/src/testing/testing.go:865 +0x163
==================
==================
WARNING: DATA RACE
Write at 0x00c00015d080 by goroutine 42:
  gopkg.in/src-d/hercules.v9/internal/plumbing.(*RenameAnalysis).Consume.func3()
      /Users/robertlin/go/src/gopkg.in/src-d/hercules.v9/internal/plumbing/renames.go:327 +0x59

Previous write at 0x00c00015d080 by goroutine 43:
  gopkg.in/src-d/hercules.v9/internal/plumbing.(*RenameAnalysis).Consume.func4()
      /Users/robertlin/go/src/gopkg.in/src-d/hercules.v9/internal/plumbing/renames.go:328 +0x59

Goroutine 42 (running) created at:
  gopkg.in/src-d/hercules.v9/internal/plumbing.(*RenameAnalysis).Consume()
      /Users/robertlin/go/src/gopkg.in/src-d/hercules.v9/internal/plumbing/renames.go:327 +0x2526
  gopkg.in/src-d/hercules.v9/internal/plumbing.TestRenameAnalysisConsume()
      /Users/robertlin/go/src/gopkg.in/src-d/hercules.v9/internal/plumbing/renames_test.go:126 +0xb26
  testing.tRunner()
      /usr/local/Cellar/go/1.12/libexec/src/testing/testing.go:865 +0x163

Goroutine 43 (finished) created at:
  gopkg.in/src-d/hercules.v9/internal/plumbing.(*RenameAnalysis).Consume()
      /Users/robertlin/go/src/gopkg.in/src-d/hercules.v9/internal/plumbing/renames.go:328 +0x2558
  gopkg.in/src-d/hercules.v9/internal/plumbing.TestRenameAnalysisConsume()
      /Users/robertlin/go/src/gopkg.in/src-d/hercules.v9/internal/plumbing/renames_test.go:126 +0xb26
  testing.tRunner()
      /usr/local/Cellar/go/1.12/libexec/src/testing/testing.go:865 +0x163
==================
==================
WARNING: DATA RACE
Read at 0x00c00015d080 by goroutine 41:
  gopkg.in/src-d/hercules.v9/internal/plumbing.(*RenameAnalysis).Consume()
      /Users/robertlin/go/src/gopkg.in/src-d/hercules.v9/internal/plumbing/renames.go:330 +0x257a
  gopkg.in/src-d/hercules.v9/internal/plumbing.TestRenameAnalysisConsume()
      /Users/robertlin/go/src/gopkg.in/src-d/hercules.v9/internal/plumbing/renames_test.go:126 +0xb26
  testing.tRunner()
      /usr/local/Cellar/go/1.12/libexec/src/testing/testing.go:865 +0x163

Previous write at 0x00c00015d080 by goroutine 43:
  gopkg.in/src-d/hercules.v9/internal/plumbing.(*RenameAnalysis).Consume.func4()
      /Users/robertlin/go/src/gopkg.in/src-d/hercules.v9/internal/plumbing/renames.go:328 +0x59

Goroutine 41 (running) created at:
  testing.(*T).Run()
      /usr/local/Cellar/go/1.12/libexec/src/testing/testing.go:916 +0x699
  testing.runTests.func1()
      /usr/local/Cellar/go/1.12/libexec/src/testing/testing.go:1157 +0xa8
  testing.tRunner()
      /usr/local/Cellar/go/1.12/libexec/src/testing/testing.go:865 +0x163
  testing.runTests()
      /usr/local/Cellar/go/1.12/libexec/src/testing/testing.go:1155 +0x523
  testing.(*M).Run()
      /usr/local/Cellar/go/1.12/libexec/src/testing/testing.go:1072 +0x2eb
  main.main()
      _testmain.go:170 +0x222

Goroutine 43 (finished) created at:
  gopkg.in/src-d/hercules.v9/internal/plumbing.(*RenameAnalysis).Consume()
      /Users/robertlin/go/src/gopkg.in/src-d/hercules.v9/internal/plumbing/renames.go:328 +0x2558
  gopkg.in/src-d/hercules.v9/internal/plumbing.TestRenameAnalysisConsume()
      /Users/robertlin/go/src/gopkg.in/src-d/hercules.v9/internal/plumbing/renames_test.go:126 +0xb26
  testing.tRunner()
      /usr/local/Cellar/go/1.12/libexec/src/testing/testing.go:865 +0x163
==================

update: travis ran into the same issue, so I've opened #233 with some more details

update 2: working on resolving race conditions, so hopefully this will close #233 as well

bobheadxi added 2 commits Mar 7, 2019
Signed-off-by: Robert Lin <robertlin1@gmail.com>
Signed-off-by: Robert Lin <robertlin1@gmail.com>
@@ -65,6 +65,7 @@ script:
- golint -set_exit_status $(go list ./... | grep -v /vendor/)
- flake8
- go test -coverpkg=all -v -coverprofile=coverage.txt -covermode=count gopkg.in/src-d/hercules.v9/... && sed -i '/cmd\/hercules\|core.go/d' coverage.txt
- go test -race gopkg.in/src-d/hercules.v9/... # run race test separately to preserve -covermode=count

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Mar 7, 2019

Author Contributor

note on this: it appears -race must be run with -covermode=atomic:

❯ go test -race -coverpkg=all -v -coverprofile=coverage.txt -covermode=count gopkg.in/src-d/hercules.v9/... && sed -i '/cmd\/hercules\|core.go/d' coverage.txt
-covermode must be "atomic", not "count", when -race is enabled

It's not great having the test suite run twice though, so I'm holding off on updating the appveyor config for now, but imo having the race detection is much more valuable than -covermode=count, let me know what you think and I can swap it over entirely to -covermode=atomic.

This comment has been minimized.

Copy link
@vmarkovtsev

vmarkovtsev Mar 8, 2019

Contributor

I am fine with -covermode=atomic as long as it does not break the profile stats, so yeah, let's swap.

…aces

Signed-off-by: Robert Lin <robertlin1@gmail.com>
@bobheadxi bobheadxi changed the title Enable race detection on test run Enable race detection on test run, fix race condition Mar 7, 2019
@bobheadxi

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

Not sure what happened with one of the build jobs: https://travis-ci.com/src-d/hercules/jobs/183188633

but the -race tests are passing in https://travis-ci.com/src-d/hercules/jobs/183188632

@vmarkovtsev

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

Hi @bobheadxi thanks so much for your contribution. That build failed because of an annoying problem in Travis environment related to networking:

grouped write of manifest, lock and vendor: error while writing out vendor tree: failed to write dep tree: failed to export golang.org/x/sys: failed to fetch source for https://go.googlesource.com/sys: unable to get repository: Cloning into '/home/travis/gopath/pkg/dep/sources/https---go.googlesource.com-sys'...
fatal: unable to access 'https://go.googlesource.com/sys/': The requested URL returned error: 502

I restarted that job. I have recently had to restart it 5 times before that error goes away, which is very annoying. We are thinking with @smola what can be done to resolve it.

@vmarkovtsev

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

It is 10pm here, so I will review this tomorrow 👍

@@ -65,6 +65,7 @@ script:
- golint -set_exit_status $(go list ./... | grep -v /vendor/)
- flake8
- go test -coverpkg=all -v -coverprofile=coverage.txt -covermode=count gopkg.in/src-d/hercules.v9/... && sed -i '/cmd\/hercules\|core.go/d' coverage.txt
- go test -race gopkg.in/src-d/hercules.v9/... # run race test separately to preserve -covermode=count

This comment has been minimized.

Copy link
@vmarkovtsev

vmarkovtsev Mar 8, 2019

Contributor

I am fine with -covermode=atomic as long as it does not break the profile stats, so yeah, let's swap.

@vmarkovtsev vmarkovtsev merged commit d6c5828 into src-d:master Mar 8, 2019
4 checks passed
4 checks passed
DCO DCO
Details
Travis CI - Pull Request Build Passed
Details
codecov/project 85.28% (+0.05%) compared to 7e55135
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.