Repository navigation
Conversation
AdvanceWaiter.Wait and MustWait selected on the advance channel and ctx.Done() together. When both were ready, select picked one at random, so a completed advance failed about half the time with an expired context. The context only bounds how long to wait, so check for completion again before returning the context error. MustWait now calls Wait so both share that logic.
Trap.Wait and Call.Release had the same random select as AdvanceWaiter: with a trapped call ready, or a released call complete, an expired context could still win. Check the call channel again before returning the context error. The regression tests are internal because the public API cannot order these states before the context check: catch delivers each call from its own goroutine, and a call completes only after its last release.
The completion recheck added to AdvanceWaiter.Wait and Trap.Wait must not block: when the awaited work has not happened, an expired context still returns its error. Call.Release already has this coverage in Test_MultipleTrapsDeadlock.
The Release and AdvanceWaiter.Wait docs said they return nil when the work has completed even if the context has also expired. Read as "an already-expired context works", that is wrong for the common cases: call.Release(expired) right after trap.MustWait returned an error 1000/1000, because the call completes only after Release's own releases.Done(). Advance with a pending AfterFunc followed by Wait(expired) also failed 1000/1000, because triggered events run on other goroutines. Describe the guarantee as completion by the time the context expires.
"If a call is ready" reads as "if the code under test has made the call", but each trapped call reaches Wait from a separate catch goroutine. With a call trapped by two traps, trap0.MustWait proved the call was made, yet trap1.Wait(expired) still failed 997/1000 because trap1's catch goroutine had not sent yet. Use the same wording as Release and AdvanceWaiter.Wait.
If AdvanceWaiter.Wait or Trap.Wait ever blocked on an expired context, TestExpiredContext_WorkPending hung until the go test timeout. Run each Wait on a goroutine and bound it with a 10s context so a regression fails with a message. With both inner checks changed to block, each subtest now fails after 10s with "Wait() did not return after the context expired".
The fix makes AdvanceWaiter.Wait and Call.Release match what their original godoc already said, and the expired-context caveat does not change how normal test code uses them. Trap.Wait keeps only a one-line summary; users cannot observe when a trapped call arrives, so a caveat about arrival gives them nothing to act on.
MustWait now calls Wait, so the Wait subtest covers the behavior. The other Must* wrappers, Trap.MustWait and Call.MustRelease, have no separate test either.
Each method's expired-context coverage now lives under one name: TestAdvanceWaiter_ExpiredContext with Completed and Pending subtests, and TestTrapWait_ExpiredContextNoCall. The test docs state the contract instead of the regression that prompted them.
Preferring completion turned a wrong wait context into a flake. With that the same Advance expires failed 10000/10000 without this PR, but only 23/10000 with it (about 6000/10000 under -race). The same holds for t.Context() used in a cleanup and for a test timeout used up by time.Sleep in a synctest bubble. Wait now returns ctx.Err() whenever the context is done, so the mistake fails every time at the line that made it. Done still reports completion without a deadline. #30's mock contexts, mClock.Advance(time.Second).Wait(ctx) on a context
Like AdvanceWaiter.Wait, Trap.Wait now fails every time on an expired context instead of only when select happens to pick ctx.Done(). It checks the context before the select rather than after it: once Wait has received a call it must return it, because the code under test stays blocked until the call is released. A call that is not taken stays in the trap for the next Wait. This also makes a closed trap plus an expired context return the context error every time, where it used to return ErrTrapClosed or the context error at random.
Release released the call before waiting, but only decremented the trap's unreleased count after the call completed. When the wait failed, for example while another trap still held the call, Trap.Close later reported "Closed() with 1 unreleased calls" for a call the test had released. Test_MultipleTrapsDeadlock now uses an already-expired context instead of waiting two seconds, and checks that Close reports nothing.
Every failed Release asked "did more than one trap capture the call?", even when one trap caught it, so the hint pointed at a cause that could not apply. The mock now records how many traps caught each call. Release names that count when it is above one and otherwise only says the call did not complete after release.
AdvanceWaiter.Wait and Trap.Wait now return the context error whenever the context is done, and Release does not. The reason is not visible in the code: Release acts before it waits, and a second Release panics with "sync: negative WaitGroup counter", so its result is the test's only report of whether the call completed.
Release called releases.Done() and only then decremented the trap's unreleased count. Once Done runs, the code under test can return and the test can call Trap.Close, which reads the count. With Release on another goroutine, -race reported a data race between Close and callReleased on every run. On main, where the decrement came after the call completed, Close also reported "Closed() with 1 unreleased calls" in 4987 of 5000 runs of the same test. Test_UnreleasedCalls gets a subtest for this next to the existing unreleased-call case.
The trap's mu protects unreleasedCalls, but Close read it twice without mu: once to decide whether to fail and again for the message. On main that produced "Closed() with 0 unreleased calls" in 11 of 1000 runs of the Test_UnreleasedCalls release subtest, a failure whose message contradicts itself. Close now reads the count once under mu, so it never races with Trap.Wait or Release, whatever synchronization the test uses. Close holds Mock.mu when it takes mu. Nothing takes Mock.mu while holding mu, and the field comment now says to keep it that way.
A regression to the random select on main made each of these checks fail in only about half of the runs, so one CI run could miss it. Each check now runs 100 times per test. Against main's mock.go they fail 200 of 200 runs, and each test still takes under a millisecond.
Release preferred completion on the grounds that its result was the only report of whether the call completed. That never applied when Release was entered with an expired context: the call completes only after this Release's own releases.Done(), so the completion check could only win when the code under test returned before the select ran. Release(expired) returned nil in 2 of 10000 runs, 28 under -race. Release now reads the context before releasing and returns its error if it had already expired. It still releases the call first, so the code under test is not left blocked. The internal Release tests checked a completed call before release, a state the mock cannot reach. TestCallRelease_ExpiredContext replaces them and Test_MultipleTrapsDeadlock with a public table: one trap, two traps, and two traps with a context that expires while Release waits. Each row also checks that the call returns and that Close reports nothing.
The 100-iteration loops ran in every row, but only Completed and TrapClosed involve a select that can pick either case; Pending fails the same way every run. TrapWait_ExpiredContext/NoCall passed on main and under every mutation, because Trap.Wait returns before its select when the context has already expired. ExpiresWhileWaiting replaces it: the context is live when Wait is called, so it covers the select's ctx.Done case, which no test reached before. The ReleasedOnAnotherGoroutine comment now says what that subtest catches.
With an expired context, MustWait fails even when the advance has completed, and the message "context expired while waiting for clock to advance" then points at the wrong cause. It now says the advance completed and asks whether the context had already expired when MustWait was called. The two messages are in separate select branches, so a TestingT whose Fatalf returns sees one failure, not two. The multi-trap Release error now says each trap must release the call from its own goroutine, as the Release godoc requires. The godoc for Wait and MustWait says "the advance" instead of "timers and ticks", and keeps the first line from main. apiCall.traps is now trapCount, since it holds a number.
Release with an already-expired context returns without waiting, but its error said "timed out waiting for the call to complete" and, with two traps, told the user every trap must release the call. Neither is the cause: the call was released and may well complete. That branch now says the context had already expired when Release was called and that the call was released. The timeout message stays for a context that expires while Release waits. TwoTrapsWhileWaiting used a 10ms timeout, which now picks the message by timing: a slow run would see the already-expired one. It uses a test context that expires when Release first asks for Done, after the entry check, so the row no longer depends on the clock.
MustWait chose its "already expired when MustWait was called" hint by checking whether the advance had completed after Wait failed. That was wrong both ways: a context that expired during the wait just before the advance completed got the hint, and a context that had already expired when the advance was still pending did not. MustWait now checks ctx.Err() before it waits and gives the hint exactly then. Completed and Pending each call MustWait twice: with an already-expired context, which must give the hint, and with a context that expires once MustWait waits, which must not.
ExpiresWhileWaiting is there to reach the ctx.Done case of Trap.Wait's select. With a 10ms timeout it did so only when Wait checked the context within 10ms; on a slow run the entry check returned first and the row covered nothing new. The expiresOnWaitCtx fixture is live at the entry check and expires once Wait waits, so the row reaches the select every run and no longer waits 10ms.
With more than one trap, "the call was released" could read as "the call can now return", but it returns only after every trap releases it. The message now says this trap released the call.
Once Release returned early on an already-expired context, no row reached the one-trap "after release" error: a one-trap Now call completes as soon as it is released. The table now traps the TickerFunc's Wait, which does not return until the ticker stops, so a one-trap call stays pending while Release waits. OneTrapWhileWaiting uses that, and each row checks that only the two-trap timeout mentions other traps.
MustRelease failed the test with Release's error as is, so the message named Release, which the test never called, and did not say which call failed. It now reads "MustRelease of <call>: <error>", like Trap.MustWait names its trap.
TestAdvanceWaiter_ExpiredContext has also checked MustWait's messages since MustWait started picking them from the context on entry, but its doc comment described only Wait.
MustRelease formatted the call for its failure message after Release had let the code under test return. The call's Tags share the caller's variadic slice, so a caller that reused the slice raced with the formatting: -race reported it in 10 of 10 runs, and without -race the message showed the caller's later value in 32 of 2000 runs.
MustRelease prefixes Release's error, so "context already expired when Release was called" told MustRelease callers about a method they did not call. The error now says only that the context had already expired and that this trap released the call, which reads correctly both alone and as "MustRelease of <call>: ...". MustWait still names itself because its message has no prefix.
MustRelease's godoc did not say it also fails on a context that had already expired, or that the call is released either way. Release's godoc ran past 100 columns and added its expired-context sentence as a separate line. AdvanceWaiter.Wait's doc switched between "ctx" and "the context". The MustWait comment argued against an earlier design instead of saying why the entry check exists, and the trapCount comment did not say why reading it without a lock is safe.
Trap.MustWait with a context that had already expired failed with "context expired while waiting for <trap>", which points at a slow call instead of the test's context. Like AdvanceWaiter.MustWait, it now checks the context first and says it had already expired. The closed-trap message is unchanged.
expiresOnWaitCtx expired the first time a method asked for Done, so the tests passed only while the methods read Err before Done. Rewriting an entry check as a select on Done, which changes no behavior, failed them. The tests now start the wait on a goroutine, call synctest.Wait until it blocks, then cancel the context. That works whatever order the methods read the context in. The Trap.Wait ready-call test moves to the public API as TestTrapWait_ExpiredContext/CallReady: synctest.Wait returns once the trap's catch goroutine is blocked handing over the call, so mock_internal_test.go goes away. The Completed MustWait check for a context that expires during the wait is dropped: the advance has already completed, so no wait happens and nothing can expire during it. ReleasedOnAnotherGoroutine now fails after 10s instead of hanging if the call or Release never returns.
mafredri
force-pushed
the
mafredri/mustwait-done-first
branch
from
November 4, 2026 17:31
16c0dcb to
b989a40
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 #41.
On
main,AdvanceWaiter.Wait,Trap.WaitandCall.Releaseeachselecton the awaited work and onctx.Done(). When both are ready, Go picks one at random, so a test that waits with an expired context passes or fails by chance.Now, when the context has already expired,
WaitandReleasereturn its error, andMustWaitandMustReleasefail the test, even if the work has finished. An expired wait context is a mistake in the test, such ast.Context()used in a cleanup. An earlier version of this PR returned success when the work had finished. That made such a test pass in most runs and fail only now and then, so the mistake surfaced as a rare CI flake instead of a failure at the line that passed the context.semaphore.Acquirein x/sync makes the same choice.Releasestill releases the call when it returns an error. To check whether an advance has finished without a deadline, useAdvanceWaiter.Done().Trap.Closenow counts released calls correctly.Releasecounted a call only after it completed, soCloseright after a call released on another goroutine reported "Closed() with 1 unreleased calls", and-racereported a data race. A failedReleasealso left its call counted.Changed failure messages:
AdvanceWaiter.MustWaitandTrap.MustWait: "context already expired when MustWait was called" (for a trap: "… called for ") when the context had expired before the call.Call.Release: "context already expired; this trap released the call" when the context had expired before the call. If it expires whileReleasewaits: "timed out waiting for the call to complete after release", or with several traps, "…; N traps caught it, and each must release it from its own goroutine". This replaces "did more than one trap capture the call?".Call.MustRelease: "MustRelease of : ".Squash note: please replace the squash commit body with this description. The commit messages on this branch record earlier designs.