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

testing: data race on common.done #28169

Closed
mikioh opened this issue Oct 12, 2018 · 7 comments

Comments

Projects
None yet
5 participants
@mikioh
Copy link
Contributor

commented Oct 12, 2018

See https://storage.googleapis.com/go-build-log/91f20a3f/linux-amd64-race_651331f3.log

ok  	os	9.834s
ok  	os/exec	24.172s
==================
WARNING: DATA RACE
Read at 0x00c0000ba443 by goroutine 43:
  testing.(*common).logDepth()
      /workdir/go/src/testing/testing.go:612 +0x94
  testing.(*common).log()
      /workdir/go/src/testing/testing.go:602 +0x55
  testing.(*common).Logf()
      /workdir/go/src/testing/testing.go:634 +0x86
  os/signal_test.TestTerminalSignal.func1()
      /workdir/go/src/os/signal/signal_cgo_test.go:140 +0x378

Previous write at 0x00c0000ba443 by goroutine 40:
  testing.tRunner.func1()
      /workdir/go/src/testing/testing.go:841 +0x337
  testing.tRunner()
      /workdir/go/src/testing/testing.go:854 +0x17e

Goroutine 43 (running) created at:
  os/signal_test.TestTerminalSignal()
      /workdir/go/src/os/signal/signal_cgo_test.go:116 +0x86f
  testing.tRunner()
      /workdir/go/src/testing/testing.go:850 +0x162

Goroutine 40 (running) created at:
  testing.(*T).Run()
      /workdir/go/src/testing/testing.go:901 +0x64e
  testing.runTests.func1()
      /workdir/go/src/testing/testing.go:1142 +0xa8
  testing.tRunner()
      /workdir/go/src/testing/testing.go:850 +0x162
  testing.runTests()
      /workdir/go/src/testing/testing.go:1140 +0x4ee
  testing.(*M).Run()
      /workdir/go/src/testing/testing.go:1057 +0x2ef
  main.main()
      _testmain.go:64 +0x221
==================
FAIL
FAIL	os/signal	18.658s

Looks like https://go-review.googlesource.com/c/127596 introduces this data race.

@bradfitz bradfitz added the NeedsFix label Oct 13, 2018

@bradfitz bradfitz added this to the Go1.12 milestone Oct 13, 2018

@bradfitz

This comment has been minimized.

Copy link
Member

commented Oct 13, 2018

@wselwood, could you look into this?

@wselwood

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2018

I'll take a look.

@wselwood

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2018

Hi @bradfitz

I've taken a look I think I understand whats going on here, but I am not sure of the right way to go about fixing it. One side of the race condition is explicitly not locking, according to a comment above it.

This appears to be handled the same way in the t.Fail() path.

Any ideas or hints on the right way to solve this would be welcomed. I am happy to try and implement it if you can point me in the right direction, or to a person who is more likely to know the best way to solve this.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

I didn't follow the original CL. Maybe @ianlancetaylor or @mpvl have that code in their head at the moment.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2018

I don't think there is anything wrong with https://golang.org/cl/127596 . The testing package arranges for the race detector to report an error if there is an unsynchronized goroutine. CL 127596 introduces a new way for the race detector to notice an unsynchronized goroutine. Previously the race detector would notice an unsynchronized goroutine that calls t.Fail (or t.Error, etc.). Now the race detector will additionally notice an unsynchronized goroutine that calls t.Log. So I think we just need to fix this in the os/signal package.

@gopherbot

This comment has been minimized.

Copy link

commented Oct 17, 2018

Change https://golang.org/cl/142889 mentions this issue: os/signal: wait for goroutine in TestTerminalSignal

@gopherbot gopherbot closed this in dc75744 Oct 17, 2018

@wselwood

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2018

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