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

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

Comments

Projects
None yet
6 participants
@tombergan
Copy link
Contributor

commented Jan 11, 2017

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

This comment has been minimized.

Copy link
Member

commented Jan 11, 2017

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 label Jan 11, 2017

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2017

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

@gopherbot

This comment has been minimized.

Copy link

commented Jan 18, 2017

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

@flowblok

This comment has been minimized.

Copy link

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)

@dsnet

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link

commented Dec 1, 2017

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

@gopherbot gopherbot closed this in f3b24b9 Dec 1, 2017

@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.
You can’t perform that action at this time.