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/synctest: experimental package for testing concurrent code #69687

Closed
Tracked by #13091
neild opened this issue Sep 27, 2024 · 16 comments
Closed
Tracked by #13091

testing/synctest: experimental package for testing concurrent code #69687

neild opened this issue Sep 27, 2024 · 16 comments

Comments

@neild
Copy link
Contributor

neild commented Sep 27, 2024

Proposal Details

This is an offshoot of #67434. See that proposal for prior discussion and more details.

I propose adding a new testing/synctest package to aid in testing concurrent code. This package will be initially guarded by a GOEXPERIMENT flag: It will be present only in toolchains built with GOEXPERIMENT=synctest. After we've gained some experience with the experimental package, we will make the decision to drop the flag requirement and fully release it, abandon the experiment and remove it, or amend it.

The package API is as follows:

// Package synctest provides support for testing concurrent code.
package synctest

// Run executes f in a new goroutine.
//
// The new goroutine and any goroutines transitively started by it form
// an isolated "bubble".
// Run waits for all goroutines in the bubble to exit before returning.
//
// Goroutines in the bubble use a fake time implementation.
// The initial time is midnight UTC 2000-01-01.
//
// A goroutine in the bubble is idle if it is blocked on:
//   - a send or receive on a channel created within the bubble
//   - a select statement where every case is a channel within the bubble
//   - sync.Cond.Wait
//   - time.Sleep
//
// The above cases are the only times a goroutine is idle.
// In particular, a goroutine is NOT idle when blocked on:
//   - system calls
//   - cgo calls
//   - I/O, such as reading from a network connection with no data
//   - sync.Mutex.Lock or sync.Mutex.RLock
//
// Time advances when every goroutine in the bubble is idle.
// For example, a call to time.Sleep will block until all goroutines
// are idle, and return after the bubble's clock has advanced
// by the sleep duration.
//
// If every goroutine is idle and there are no timers scheduled,
// Run panics.
//
// Channels, time.Timers, and time.Tickers created within the bubble
// are associated with it. Operating on a bubbled channel, timer, or ticker
// from outside the bubble panics.
func Run(f func())

// Wait blocks until every goroutine within the current bubble,
// other than the current goroutine, is idle.
func Wait()
@neild neild added the Proposal label Sep 27, 2024
@gabyhelp
Copy link

Related Issues and Documentation

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@aclements
Copy link
Member

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.

@aclements
Copy link
Member

Putting on hold until we get enough implementation experience.

@mvdan
Copy link
Member

mvdan commented Oct 30, 2024

How would that experience happen over the coming months? Can we perhaps try an experimental implementation before this proposal gets accepted?

@aclements
Copy link
Member

This proposal already is for an experimental implementation. My understanding is that we're running into issues right off the bat even with that. Though perhaps that shouldn't block this from going through the proposal process.

@aclements aclements moved this from Active to Hold in Proposals Oct 30, 2024
@aclements
Copy link
Member

Placed on hold.

@neild
Copy link
Contributor Author

neild commented Oct 30, 2024

We've got one issue which I'm aware of, which is the question of how synctest bubbles interact with testing.T.Cleanup functions.

I think there are two clear answers to that question, which I presented in #67434 (comment):

  1. Proposal as written: T.Cleanup functions do not execute within a bubble. Tests need to gracefully handle this fact. Pro: No changes to proposal, API is entirely isolated to the testing/synctest package, the failure condition is an easily-detectable panic. Con: Tests that use synctest will need to be careful in how they use T.Cleanup.

  2. T.Cleanup functions execute within a bubble. Either we move the synctest.Run function to a method of testing.T, or make synctest.Run take a testing.TB parameter. Pro: Convenient. Con: Requires a change to this proposal.

My preference is to move the synctest API to the testing package, still behind a GOEXPERIMENT=synctest guard, but I'm fine with either of the above alternatives. Keeping the proposal-as-written also doesn't preclude us adding a testing.T method in the future if it turns out to be a good idea.

I don't think we're going to gain substantial further experience with synctest without an experimental implementation that people can try out.

@rsc
Copy link
Contributor

rsc commented Nov 13, 2024

It seems like this proposal only has to decide whether to create GOEXPERIMENT=synctest, not exactly what it means. (The point of the GOEXPERIMENT is to be able to check in the code and change what it means over time, in service of #67434.)

Does it make sense to approve the GOEXPERIMENT so that we can try some version of the code in Go 1.24 and find out if there are any problems besides t.Cleanup?

@rsc
Copy link
Contributor

rsc commented Nov 13, 2024

Damien also reports having converted some etcd tests that were using clockwork to just use synctest, and lots of testing things melted away and the tests just passed. So we do know that it works well enough for real usage, which seems to me to justify making it available as an experiment to get more real-world experimentation.

@aclements
Copy link
Member

You're right. I'm not quite sure what thought process led to me putting this one on hold. Maybe I meant to put #67434 on hold pending experience with the GOEXPERIMENT? Who even knows.

@cherrymui
Copy link
Member

// A goroutine in the bubble is idle if it is blocked on:
//   - a send or receive on a channel created within the bubble
//   - a select statement where every case is a channel within the bubble
//   - sync.Cond.Wait
//   - time.Sleep

I'm not sure about the word "idle". Could we just call it blocked? @neild also mentioned "durably blocked" at some point offline. How about that? Or, "bubbly blocked"?

@aclements aclements moved this from Hold to Likely Accept in Proposals Nov 13, 2024
@aclements
Copy link
Member

Based on the discussion above, this proposal seems like a likely accept.

The proposal is to add GOEXPERIMENT=synctest, which enables the testing/synctest package with the API given in the top comment.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/591997 mentions this issue: internal/synctest: new package for testing concurrent code

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/629735 mentions this issue: testing/synctest: add experimental synctest package

gopherbot pushed a commit that referenced this issue Nov 19, 2024
Add an internal (for now) implementation of testing/synctest.

The synctest.Run function executes a tree of goroutines in an
isolated environment using a fake clock. The synctest.Wait function
allows a test to wait for all other goroutines within the test
to reach a blocking point.

For #67434
For #69687

Change-Id: Icb39e54c54cece96517e58ef9cfb18bf68506cfc
Reviewed-on: https://go-review.googlesource.com/c/go/+/591997
Reviewed-by: Michael Pratt <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
@aclements
Copy link
Member

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.

The proposal is to add GOEXPERIMENT=synctest, which enables the testing/synctest package with the API given in the top comment.

@aclements aclements moved this from Likely Accept to Accepted in Proposals Nov 21, 2024
@aclements aclements changed the title proposal: testing/synctest: experimental package for testing concurrent code testing/synctest: experimental package for testing concurrent code Nov 21, 2024
@aclements aclements modified the milestones: Proposal, Backlog Nov 21, 2024
@dmitshur dmitshur modified the milestones: Backlog, Go1.24 Nov 22, 2024
@dmitshur
Copy link
Contributor

@neild This change (a package available only when an experiment is set) should be mentioned in Go 1.24 release notes, otherwise users might not know it exists and might not test it sufficiently, is that right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

No branches or pull requests

8 participants