Repository navigation
Conversation
A Mock created with the outer t and used inside synctest.Test could hang the test until go test's timeout. A goroutine the Mock starts in the bubble, such as the run loop from Ticker.Reset on a stopped ticker, belongs to the bubble but waits on channels created outside it. synctest does not count that as durably blocked, so it neither reports a deadlock nor returns: the goroutine exits only in NewMock's cleanup, which runs after synctest.Test returns. NewMock now records whether it ran in a bubble, and every call that would start a goroutine panics if it runs in a bubble and the Mock was created outside one. The bubble is detected by time.Now having no monotonic reading, which is runtime behavior rather than API.
QF1009 suggests Time.Equal, but Equal ignores the monotonic reading that inBubble tests for, so it would report true everywhere and the check would never panic.
Checking only calls that start goroutines left a rule users could not predict, and a recovered panic could leave state behind: TickerFunc, NewTicker and Ticker.Reset added an event before panicking, and Timer.Reset(0) removed one, which changed how far Advance could go. matchCallLocked now checks first, before any caller changes state, and the entries that bypass it check at their top: Advance, Set, AdvanceNext, Peek, Trap, WithLogger, Trap.Wait, Trap.Close, Call.Release and AdvanceWaiter.Done, which Wait and MustWait now use. The checks next to goroutine starts are gone.
time.Now has no monotonic reading when the wall clock is outside 1885 to 2157, and the rejected second signal needs one reason only.
Done dereferenced the Mock for the bubble check, so Done and Wait on a zero AdvanceWaiter panicked instead of returning nil and ctx.Err(). The checkBubble comment now names the two public methods that do not check.
Keep NewMock's existing paragraph as it was, state the rule for test writers in one paragraph, and give maintainers the hang mechanism and the state invariant without listing exceptions or rejected alternatives.
mafredri
marked this pull request as ready for review
November 5, 2026 15:20
Member
Author
|
@spikecurtis LMK if this is too controversial (also see question above). Thought it would be a nice way to help protect against mis-use. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #41.
A
*Mockcreated with the outertand used insidesynctest.Testcan hang the test untilgo testtimes out: a goroutine it starts in the bubble waits on channels created outside and exits only inNewMock's cleanup, aftersynctest.Testreturns.In a bubble, call
NewMockwith thetthatsynctest.Testpasses in. A Mock created outside the bubble now panics when used inside it:With this change, the public methods of the Mock and of the timers, tickers, traps, calls and waiters it returns check first (
Trap()andTrap.Stringchange nothing and do not), so a recovered panic leaves no state behind. Misuse that passed by chance now panics, such asAfterFunc(0)orAdvancewith nothing due.Detection relies on undocumented runtime behavior: in a bubble,
time.Nowhas no monotonic reading. Outside one, every port'stime.Nowhas one, from the runtime's monotonic clock (Plan 9 kernels without it fall back to Unix time, which is also nonzero), between the years 1885 and 2157. If Go changes this, the check stops firing; it cannot panic unless the Mock was created outside a bubble and used inside one. Run on linux/amd64 and linux/386; other ports checked by reading the runtime source.The check costs about 50 ns per Mock call; allocations are unchanged, and
Nowwith the default logger is about 4% slower.Question for Spike
Should there be a way to turn the check off?
NewMockoption that disables it: saves the cost, but adds API and lets the hang back.Follow-ups
go child.checkParent()is covered; the context methods and cancel function need the check, added by whichever PR lands second.Resetof a stopped ticker now panics.t: now true only for a Mock created inside with the outert.Behavior on #41: Mock created outside, call made inside the bubble
"Crashes" means the test binary dies with a synctest deadlock panic or a runtime fatal error such as
receive on synctest channel from outside bubble.TickerFuncDonechannel was created outside the bubble, crashes if inside; works with a context from the bubbleNewTicker,NewTimer(0)AfterFunc(0),Now,Peek,WithLogger,Trap().Now(),Trap.Close,AdvanceWaiter.MustWaitAdvance,Set,AdvanceNextwith an event dueAfterFunctimers or outside tickers fireAdvancewith nothing dueTimer.Reset(0)AfterFunctimerTicker.ResetafterStopTrap.WaitthenCall.Releasesync: WaitGroup.Add called from inside and outside synctest bubblecoder/coder's only test with a Mock in a bubble (
agent/x/agentacp) creates it inside and passes.When squash merging, please replace the commit body with this description.