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

testing: data race on common.done #28169

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

testing: data race on common.done #28169

mikioh opened this issue Oct 12, 2018 · 7 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.12

Comments

@mikioh
Copy link
Contributor

mikioh 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 The path to resolution is known, but the work has not been done. label Oct 13, 2018
@bradfitz bradfitz added this to the Go1.12 milestone Oct 13, 2018
@bradfitz
Copy link
Contributor

@wselwood, could you look into this?

@emilyselwood
Copy link
Contributor

I'll take a look.

@emilyselwood
Copy link
Contributor

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

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

@ianlancetaylor
Copy link
Contributor

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

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

@emilyselwood
Copy link
Contributor

Thanks @ianlancetaylor

@emilyselwood emilyselwood mentioned this issue Jan 14, 2019
Closed
@ploxiln ploxiln mentioned this issue May 10, 2019
Closed
@ronnno ronnno mentioned this issue Sep 16, 2019
Closed
@golang golang locked and limited conversation to collaborators Oct 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.12
Development

No branches or pull requests
5 participants
@bradfitz @mikioh @emilyselwood @ianlancetaylor @gopherbot