testing/synctest: add convenience function for Sleep then Wait

[rittneje]

Proposal Details

In our experimentation with testing/synctest (which is really helpful!), we have found that a surprisingly common mistake is to call time.Sleep in the test function but forget to call synctest.Wait afterwards, which causes the test to be racy. These tests are similar in nature to this example.

To that end, I would like to propose a helper function (I don't care much about the exact name) that internally makes these two calls.

package synctest

func Sleep(d time.Duration) {
    time.Sleep(d)
    Wait()
}

Obviously this function isn't strictly needed, and we could of course make that wrapper ourselves. The hope is that by having it within synctest and replacing two calls with one, the likelihood of the aforementioned mistake will reduce significantly.

[gabyhelp]

Related Issues

• testing/synctest: new package for testing concurrent code #67434 (closed)
• proposal: testing: less syntactic overhead to synctest tests #76607
• testing/synctest: experimental package for testing concurrent code #69687 (closed)
• testing/synctest: replace Run with Test #73567 (closed)
• proposal: context: add Sleep #39483 (closed)
• proposal: testing/synctest: create bubbles with Start rather than Run #73062 (closed)
• testing/synctest: improve interactions between Wait, time, and the race detector #74352
• proposal: context: Sleep(ctx, dur) #73295 (closed)

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

[ianlancetaylor]

CC @neild

[aclements]

Notes from proposal committee:

• We dropped this from the original synctest proposal because it was so trivial. "On the one hand, it's incredibly trivial. On the other hand, it's incredibly trivial."
• @neild says he's written this function in almost every test that uses synctest.
• The documentation would have to be clear that (unlike the rest of synctest!) there's absolutely no magic to this function. It's just there because in a test you almost certainly don't want to just time.Sleep().

[nicholashusin]

In a similar vein, I've noticed that we've ended up with this convenience function in both x/net/http2 and x/net/internal/http3:

func synctestSubtest(t *testing.T, name string, f func(t *testing.T)) {
	t.Run(name, func(t *testing.T) {
		synctest.Test(t, f)
	})
}

I suspect this is probably not going to be used as much as the proposed synctest.Sleep, but it does wrap something that is less trivial to write (and can prevent an additional indentation!).

(To be clear, I don't intend to scope-creep this proposal. Just pointing out another similar instance to potentially help decide how far we want to go in terms of giving convenience within synctest.)

[aclements]

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 for the proposal review group

[apparentlymart]

One thing I noticed in some early experiments with synctest is that those who are less familiar with it tend to not immediately understand that sleeping in a synctest bubble only advances the virtual clock, without actually pausing. Of course they do quickly understand it once told, but it's caused me to end up always leaving comments somewhere nearby attempting to quickly explain how synctest time works so that future readers can more easily notice that something special is going on.

Having a special synctest-specific sleep function seems like it could help with that: seeing synctest.Sleep instead of time.Sleep on the page is an initial clue that something special is happening, and then the documentation of synctest.Sleep could be a good central place to briefly note that synctest has a virtual clock so that information doesn't need to be repeated directly in the test code.

(I did also think about suggesting a name that makes it even more explicit that the clock is being virtualized, such as synctest.FakeSleep, but I don't really like any of the potential alternative names I thought about. I think documentation associated with synctest.Sleep is probably sufficient.)

[aclements]

Proposal review discussion:

• Could we make time.Sleep imply a Wait? Not with the current definition of Wait, which cannot be called concurrently. We could redefine Wait to say it waits for all goroutines except those in Wait, but if you have two goroutines Sleep(1) and Wait, then it has to be arbitrary which one wakes up first, which means test code and system-under-test code would still race if they both sleep the same amount. Therefore, you really only want to sleep and wait in test driver code.
• Could we do this just for the "root" goroutine of the synctest bubble? We've avoided making any goroutine in the bubble special. Also, the test code can synchronously call into system-under-test code, and you only want the test code to do the Wait.
• If we add an automatic Wait, does that also apply to receiving from a timer channel? Or an AfterFunc?
• Even though synctest.Sleep would be a trivial function, it captures a common and important pattern and is almost always what you want in the test code. Capturing that pattern in a function makes it much more visible.

[aclements]

Based on the discussion above, this proposal seems like a likely accept.
— aclements for the proposal review group

The proposal is to add the following function to package synctest:

// Sleep advances the virtual clock by d,
// allowing other goroutines in this synctest bubble to run,
// and then waits until all goroutines in this bubble
// are durably blocked.
//
// This is exactly equivalent to
//
//     time.Sleep(d)
//     synctest.Wait()
//
// In tests, this is often preferable to calling only [time.Sleep].
// If the test itself and another goroutine running the system under test
// sleeps for the exact same amount of time, it's unpredictable which
// of the two goroutines will run first. The test itself usually wants
// to wait for the system under test to "settle" after sleeping.
// This is what Sleep accomplishes.
func Sleep(d time.Duration) {
    time.Sleep(d)
    Wait()
}

We will also update the examples in the synctest package documentation to use this function as appropriate.