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: are T.Run and B.Run safe for concurrent use? #18603

Closed
tombergan opened this issue Jan 11, 2017 · 7 comments
Closed

testing: are T.Run and B.Run safe for concurrent use? #18603

tombergan opened this issue Jan 11, 2017 · 7 comments
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone
Go1.10

Comments

@tombergan
Copy link
Contributor

tombergan commented Jan 11, 2017 •
edited

I do not see any documentation specifying if T.Run is safe for concurrent use. I see the following doc for testing.T, which does not mention T.Run:

A test ends when its Test function returns or calls any of the methods FailNow, Fatal, Fatalf, SkipNow, Skip, or Skipf. Those methods, as well as the Parallel method, must be called only from the goroutine running the Test function.

The other reporting methods, such as the variations of Log and Error, may be called simultaneously from multiple goroutines.

There is a similar doc for testing.B.

I ask because I believe T.Run was safe for concurrent use in go 1.7. Then, this line was added in go 1.8, which made T.Run no longer safe for concurrent use. I don't particularly care whether T.Run is safe for concurrent use, but I think the semantics should be clarified.

Optimistically labeling this go1.8.

/cc @dsnet
@tombergan tombergan added this to the Go1.8 milestone Jan 11, 2017
@dsnet
Copy link
Member

dsnet commented Jan 11, 2017 •
edited

If T.Run was safe concurrently in Go1.7, even if it was ambiguous whether it should be, it's worth considering whether it should continue to be concurrent safe in Go1.8.

\cc @mpvl

(moving to Go1.8Maybe; I don't think this is a release blocker at this point)

@dsnet dsnet modified the milestones: Go1.8Maybe, Go1.8 Jan 11, 2017
@dsnet dsnet added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jan 11, 2017
@rsc
Copy link
Contributor

rsc commented Jan 18, 2017

Not sure whether @mpvl is around. Sent https://go-review.googlesource.com/35354.
/cc @bradfitz

@gopherbot
Copy link

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

@dsnet dsnet added NeedsFix The path to resolution is known, but the work has not been done. and removed Documentation NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jan 18, 2017
@flowblok
Copy link

flowblok commented Aug 2, 2017

CL https://golang.org/cl/35354 added this comment to B.Run():

// Run may be called simultaneously from multiple goroutines, but all such
// calls must happen before the outer benchmark function for b returns.

Which I don't think is true at all: calling B.Run() multiple times from multiple goroutines results in benchmarkLock.Unlock() being called multiple times before benchmarkLock.Lock(), which causes a crash.

(example code: https://play.golang.org/p/0u15AvArTK)

@ianlancetaylor ianlancetaylor reopened this Aug 2, 2017
@ianlancetaylor ianlancetaylor modified the milestones: Go1.10, Go1.8Maybe Aug 2, 2017
@dsnet
Copy link
Member

dsnet commented Aug 2, 2017

Is this a documentation issue or a implementation issue? cl/35354 does have a test for multiple goroutines using when using testing.T.Run, but lacks a test for testing.B.Run, which clearly seems broken.

Are you proposing we properly making testing.B.Run work also? or just remove the comment.

@flowblok
Copy link

flowblok commented Aug 3, 2017

I don't mind which way it gets resolved, but it's significantly less effort to just remove the comment.

@gopherbot
Copy link

Change https://golang.org/cl/80841 mentions this issue: testing: remove claim that b.Run is safe for concurrent use

@golang golang locked and limited conversation to collaborators Dec 1, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Milestone
Go1.10
Development

No branches or pull requests
6 participants
@flowblok @rsc @ianlancetaylor @dsnet @gopherbot @tombergan