Repository navigation
Conversation
mafredri
changed the title
mafredri
marked this pull request as ready for review
November 3, 2026 20:29
mafredri
marked this pull request as draft
November 3, 2026 21:03
A ticker restarted after its cleanup had run, by Reset or by NewTicker on a finished test, started a runLoop that nothing ended. It leaked outside a synctest bubble and made synctest panic inside one. runLoop now also returns when Mock.done closes, as timer writes do. fire selects on done too, because interrupt stays set when runLoop returns that way and the send to internalTicks would block with the lock held. On 5eaccd7 the new TickerResetAfterTestEnds subtest leaks runLoop and the leak check fails.
done now ends every runLoop, so the cleanup NewTicker registered only ended runLoop before NewMock's cleanup does. A ticker now ticks until NewMock's cleanup runs, including during cleanups registered between NewMock and NewTicker, as timers do. Without the done case in runLoop's inner select, FiredDuringTest now leaks the ticker's runLoop. Without the one in the outer select, TestNewTicker does.
With the per-ticker cleanup gone, a ticker stops when NewMock's cleanup closes done, and cleanups registered after NewMock still receive ticks. The old text said ticks stop at the end of the test and stay suppressed after it; Mock.done already notes that a reader waiting on C can still get a write.
The comment said what a non-nil interrupt means but not why the send needs the done case: once runLoop has returned on done, nothing receives from internalTicks, and a plain send blocks with the lock held.
TimerFiredAfterTestEnds and TickerStartedAfterTestEnds set up the same way, so AfterTestEnds now runs both as rows. ResetWithoutStop becomes TickerResetAfterTick. Advancing inside Inner leaves the run loop parked in its select with an unread tick, so closing done ends it and the later Advance finds no receiver. With a plain send in fire this row fails 20/20; before, fire's done case was only covered when the run loop happened to return first.
Mock.done was created by NewMock, outside any synctest bubble, and every runLoop selected on it. When a Mock created with the outer test's t had a ticker created inside a bubble, runLoop waited on done, which synctest does not count as durably blocked, so the test hung until go test's timeout instead of panicking at once as on main. Timer writes stopped using done for the same reason. NewMock's cleanup now stops each running runLoop by receiving on its interrupt channel, as Stop does. The Mock tracks tickers with a running runLoop, and Stop removes them. After the cleanup, Reset and NewTicker start no runLoop, so interrupt is set only while runLoop runs and fire no longer needs a done case.
When a reader was waiting on C and Stop or NewMock's cleanup was waiting on interrupt by the time runLoop reached its select, select picked between them at random. In a synctest bubble, with a reader waiting and the ticker fired as the test's last step, the reader lost the tick in 967 of 2000 runs, and in 971 of 2000 with Stop right after the Advance. main loses it about as often. runLoop now tries a non-blocking send first, as timer writes do, and the reader got the tick in all 2000 runs of both.
The test becomes TestReaderWaitingAtMockCleanup with a ticker row that fires as the test's last step, so runLoop often reaches its send only after NewMock's cleanup waits on interrupt. Without runLoop's first send, TickerFiredBeforeCleanup fails 100 of 100 runs, and 46 of 100 with -race. There is no ticker row for firing after the cleanup, because a ticker sends no ticks then.
TestTimer_ReadableDuringCleanup becomes TestReadableDuringCleanup, with a Timer and a Ticker row. Its server cleanup is registered before NewTicker, as when a server creates the ticker. With the ticker code before this PR, NewTicker's own cleanup ran first and stopped runLoop, so the worker never got the tick and the Ticker row fails.
Tickers stop when NewMock's cleanup runs, as pending timer writes end, so a cleanup registered before NewMock that waits for code reading a ticker hangs in the same way. The NewMock doc now covers tickers too.
The comment above the rounds started with "When it does", which pointed at a row comment in the table above. It now names the send it means.
After NewMock's cleanup, a timer that fires still sends to a reader already waiting on C, but a ticker sent nothing: the cleanup had stopped runLoop and none starts after it. fire now tries one non-blocking send to C when no runLoop is running, which happens only after the cleanup, so tickers match timers and still start no goroutine. The new TickerFiredAfterCleanup row fails 100 of 100 runs without this, with and without -race. On main a ticker's own cleanup also dropped ticks for a waiting reader, so this changes what such a reader sees after the test.
A time.Ticker delivers no tick from the old schedule after Reset returns. The mock did, in two ways. runLoop kept a tick it was holding for a reader, because Reset left a running runLoop alone. And a fire that Advance started could get the lock after Reset and send its tick anyway: a Reset right after Advance returned got a stale tick in 2000 of 2000 runs. Reset now stops runLoop, which drops a held tick, and starts a new one. fire returns without effect when the ticker was stopped or nxt no longer matches the fire's time, as timer fires do. TestTicker_StopAndReset replaces TestTickerStop_Go123 with a table: ResetTickedUnread fails without the restart, and ResetDuringAdvance without the check in fire.
newTimer and newTicker were defined in two tests; they are now package functions. The row comment in TestReaderWaitingAtMockCleanup said all rows fire as the test's last step, which only the two FiredBeforeCleanup rows do. AfterTestEnds/TickerResetAfterTick reached the same Reset path as TickerResetAfterStop once fire no longer selected on done, so it is removed, and the rows no longer need a context. Two comment lines in TestReadableDuringCleanup were longer than 100 columns.
NewMock's doc says what happens to timer values and ticks when the test's cleanups run, so NewTicker now refers to it instead of repeating part of it. The comment in Ticker.Stop said runLoop is interrupted only if the ticker has fired, but Stop ends runLoop either way.
Ticker.Reset's doc says it panics when d is not positive, as time.Ticker.Reset and NewTicker do, but the mock accepted it. The next fire then looped forever advancing nxt by a zero or negative period while holding the Mock's lock: after Reset(0), Advance(0) never completed and a later Stop blocked forever.
The NewMock doc said what happens to values not received when its cleanup runs, but not to timers and tickers that fire after it: they send only to a reader that is already waiting on C, and nothing is kept for a later receive.
Reset stopped runLoop and started a new one to drop a tick held for a reader. The new runLoop belongs to the caller's synctest bubble, so a ticker created outside a bubble and Reset inside one left a goroutine in the bubble waiting on channels created outside it, and synctest.Test hung until go test's timeout. Before the restart, Reset of a running ticker started no goroutine and that test passed. Reset now sends on a channel that both of runLoop's selects receive from, and runLoop drops a held tick when it gets it. A stopped ticker still gets a new runLoop, as before this PR. ResetTickedUnread fails if runLoop keeps the tick, and the tests hang if the outer select does not receive.
"A timer or ticker that fires sends only to a reader already waiting on C" did not hold for AfterFunc timers, which have no C, or for TickerFunc, which keeps calling its function after NewMock's cleanup until its context ends. The doc now names NewTimer and NewTicker, and says the same holds for a ticker reset after the cleanup: Reset starts no runLoop then, and a later reader gets nothing.
A Reset that started a new runLoop left a goroutine in the caller's synctest bubble waiting on the Mock's channels, so synctest.Test never returned. With 08e4a02's ticker.go both rows of the new TestTicker_ResetInBubble hang until go test -timeout, which stops the run with their synctest.Test call in the stack, as deadlock tests in this repo already rely on. The TickedUnread row also advances to the new schedule and checks that the first tick received is on it, which fails when runLoop keeps the old tick. The rows do not fit TestTicker_StopAndReset, which creates the Mock in the bubble and checks with synctest.Wait, which cannot see goroutines outside the bubble.
The paragraph now starts with what the caller does, calling NewMock before cleanups that wait for code reading its timers or tickers, and then gives the reason. "also after Reset" is gone: "after that cleanup" already covers a ticker reset then.
The Reset comment explained why a running ticker is signalled instead of getting a new runLoop, but not that a stopped ticker still gets one, so a stopped ticker created outside a synctest bubble and reset inside it still keeps synctest.Test from returning.
The TickedUnread row checked for a stale tick after Reset, which TestTicker_StopAndReset/ResetTickedUnread already covers, so the test is now the Running case alone. Its receive now gives up when the context ends, so a missing tick fails the test instead of waiting for go test -timeout. With 697264f's ticker.go, which starts a new runLoop on Reset, the test still hangs until go test -timeout with its synctest.Test call in the stack.
NewMock's cleanup ends every ticker's runLoop, so the Stop at the end of TestTicker_ResetInBubble and of each TestTicker_StopAndReset row did nothing that the leak check in TestMain would miss.
It now follows TestTicker_StopAndReset, so the tests of a ticker's Stop and Reset results come before the test of Reset's argument check.
The sentence named the stopped ticker twice. It now says once that the hang needs Reset called inside a bubble on a stopped ticker created outside that bubble.
mafredri
added a commit
that referenced
this pull request
Nov 4, 2026
The NewMock doc now says first what a cleanup registered after NewMock can rely on, then what NewMock's cleanup drops and what a later fire sends, and points to NewTicker for ticks. NewTicker no longer describes a Reset after its cleanup: #38 removes that cleanup, so the sentence would only be churn.
mafredri
added a commit
that referenced
this pull request
Nov 4, 2026
On this branch a Reset after the ticker's own cleanup starts its runLoop again, and ticks then arrive until Stop, also after NewMock's cleanup. 09613b7 dropped that from the NewTicker doc because #38 removes the per-ticker cleanup, but this branch can merge first, so its docs must hold without #38. NewMock's pointer to NewTicker now names the exception too.
mafredri
force-pushed
the
mafredri/timer-fire-cleanup
branch
from
November 4, 2026 18:16
5eaccd7 to
65aed39
Compare
mafredri
force-pushed
the
mafredri/ticker-reset-leak
branch
from
November 4, 2026 18:16
910d21d to
95527a4
Compare
mafredri
changed the title
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 #37.
On #37, tickers still have the shutdown hang that #37 fixes for timers.
NewTickerregistered its own cleanup that stopped the ticker, so when the test registeredt.Cleanup(srv.Close)before the server created the ticker, the ticker stopped first and a worker readingtkr.CduringClosenever got a tick. That cleanup also missed run loops started after it had run, byTicker.Resetor byNewTickeron a finished test: they leaked, and inside atesting/synctestbubble the test panicked withdeadlock: main bubble goroutine has exited but blocked goroutines remain.Now
NewMock's cleanup ends each ticker's run loop, as #37 does for timer writes, and the per-ticker cleanup is gone. TheNewMockdoc covers ticks too.Behavior changes:
NewMockbut beforeNewTickergets ticks untilNewMock's cleanup runs, as with timers. Before, the ticker's own cleanup stopped it first.NewMock's cleanup,ResetandNewTickerstart no run loop, and a ticker that fires sends only to a reader already waiting onC, as a timer does. Before, those ticks were dropped.Cgets the tick even whenStoporNewMock's cleanup comes right after the fire.Resetreturns, as withtime.Ticker.Reset. Before, a tick the run loop held for a reader, or one from anAdvancewhose fire got the lock afterReset, arrived after it in every run.Ticker.Resetwith a zero or negative duration panics, as its doc says and astime.Ticker.ResetandNewTickerdo. Before, the next fire looped forever with theMock's lock held.Resetof a running ticker from outside the synctest bubble that created it is now fatal (fatal error: send on synctest channel from outside bubble), asStopandAdvancealready were.For a running ticker,
Resettells the run loop to drop its held tick instead of starting a new run loop, because a run loop started inside a synctest bubble waits on channels created outside it and keepssynctest.Testfrom returning:TestTicker_ResetInBubblepasses on #37 and hangs with a new run loop.Tests that fail on #37:
TestReadableDuringCleanup/Ticker: the shutdown hang.TestNoLeak_Go123/AfterTestEnds/TickerResetAfterStopandNewTicker: the leak check inTestMainreports the run loop.TestReaderWaitingAtMockCleanup/TickerFiredBeforeCleanupandTickerFiredAfterCleanup: the waiting reader gets no tick.TestTicker_StopAndReset/ResetTickedUnreadandResetDuringAdvance: a stale tick afterReset.TestTickerReset_NonPositiveDuration:Resetdoes not panic.TestTimer_ReadableDuringCleanupandTestTimer_ReaderWaitingAtMockCleanupdrop theTimer_prefix and gain ticker rows.TestTicker_StopAndResetreplacesTestTickerStop_Go123, whose check is now theStopTickedUnreadrow.Not fixed:
Advance(...).MustWaitmisses the tick in about 1% to 2% of runs, and in about a quarter of runs with-race, because the run loop may not be waiting to send yet.Resetof a stopped ticker still starts a run loop, as onmain. With a ticker created outside a synctest bubble and reset inside one,synctest.Testthen hangs untilgo test's timeout.When squash merging, please replace the commit body with this description.