Repository navigation
Conversation
mafredri
force-pushed
the
mafredri/context
branch
2 times, most recently
from
May 7, 2026 18:01
8122ed5 to
c7c5550
Compare
This change lets users create context timeouts that are scheduled on mock time by introducing four new methods on Clock. The parent context cancellation is respected, and its timeout may still run on wall time, which allows use cases like setting maximum runtime limits for tests and keeps the model simple as Quartz does not translate wall time to mock time. Closes #14. BREAKING CHANGE: Clock gained four new methods, which is technically a breakage of the public API. Impact should be minimal considering Clock is not meant to be faked outside of this package.
mafredri
commented
May 8, 2026
| } | ||
| } | ||
|
|
||
| func TestWithTimeout_CancelRace(t *testing.T) { |
Member
Author
There was a problem hiding this comment.
Note: While this test doesn't assert anything, its purpose is more-so to affirm that there are no data races when multiple parts are moving simultaneously (relies on race detector).
- The clock advancement triggers ctx timeout
- Manual ctx cancel (so as to not cause data race with timeout)
- Parent cancel (context.AfterFunc cancellation path)
- Checking .Err async doesn't cause data race
mafredri
commented
May 8, 2026
|
|
||
| mock *Mock | ||
|
|
||
| mu sync.Mutex // Protects the following fields. |
Member
Author
There was a problem hiding this comment.
Could switch to rw mutex here as there are r/o paths. 🤔 I also considered atomic values but single mutex design felt simpler.
spikecurtis
reviewed
May 20, 2026
|
|
||
| mock *Mock | ||
|
|
||
| mu sync.Mutex // Protects the following fields. |
Collaborator
There was a problem hiding this comment.
most of the other primitives use the mutex of the parent *Mock. Why is that inappropriate here?
Member
Author
There was a problem hiding this comment.
It felt a bit weird to guard the context state via *Mock since it's a bit of a standalone entity that reacts to events coming from outside the mock clock. I don't think there's anything that would make it inappropriate per-se. I can look into refactoring this.
|
|
||
| func (c *mockCtx) cancel(err, cause error) { | ||
| if innerErr := c.inner.Err(); innerErr != nil { | ||
| c.cancelFromInner(innerErr) |
Collaborator
There was a problem hiding this comment.
Smells like a time-of-check; time-of-use bug, where the inner context can be still active at the time the previous line executes, but then canceled before we call cancelOnce?
Also, why do we even need this check at all, given that we register an AfterFunc that calls cancelFromInner()on the inner context?
Member
Author
There was a problem hiding this comment.
I don't think we can eliminate the inherent race between two contexts cancelling simultaneously (inner via chain/cancel, and mockCtx via cancel or deadline). Given that, I don't see TOCTOU being a factor as it is best effort (the result doesn't really matter in a race). That said, you're right in that this shouldn't be needed due to our use of AfterFunc. I'll simplify.
| // Remove the scheduled deadline before closing Done, so callers | ||
| // waiting on Done can observe cancellation without seeing a stale | ||
| // event in Peek or AdvanceNext. | ||
| c.mock.removeEvent(c) |
Collaborator
There was a problem hiding this comment.
I feel like if we used the parent (*Mock).muinstead of having our own separate mutex, then we wouldn't have to release the lock while removing the event (call removeEventLocked() -- that would allow us to synchronize setting the error and closing c.done, and so we wouldn't need the extra doneClosed field at all.
| c.cancelFromInner(innerErr) | ||
| return | ||
| } | ||
| if cause == nil { |
Collaborator
There was a problem hiding this comment.
I found this confusing, so I think a comment here would be helpful.
Alternatively, it might be clearer to put the logic that selects the correct cause into the calling functions and always pass a non-nil cause. I realize that duplicates the logic, but my reading is that there are several different reasons why copying the error to the cause is correct.
| } | ||
|
|
||
| func (c *mockCtx) Deadline() (time.Time, bool) { | ||
| return c.deadline, true |
Collaborator
There was a problem hiding this comment.
This bit has me a little worried, because contexts are often passed across package boundaries, and so the downstream package might call Deadline() and then compare the returned time against the real wall clock. This is certainly what the default round-tripper in the HTTP package does.
Maybe it's worth the sharp edge, but I think we should describe this limitation and have a warning in the README.
Member
Author
There was a problem hiding this comment.
Fair callout. I did consider the scenario but I couldn't think of an alternative. Calling this out explicitly in godoc/README is a good idea. 👍🏻
| } | ||
| call.MustRelease(testCtx) | ||
| ctx := <-ctxs | ||
| cancel := <-cancels |
Collaborator
There was a problem hiding this comment.
these should be selected against the test timeout
Use mock.mu instead of a per-context mutex, dropping the doneClosed field and the lock release/reacquire around removeEvent. Collapse the three cancel paths into a single cancelLocked. Document the Deadline() vs. wall-clock footgun in godoc and README. Guard channel reads in TestWithTimeout_Trap with testCtx. Addresses review feedback on #30.
spikecurtis
approved these changes
May 21, 2026
Two paths called a mock context's Err() while holding the Mock mutex, which Err() also takes: - WithTimeout and friends with a canceled parent from the same Mock, because context.WithCancelCause(parent) calls parent.Err(). - TickerFunc exiting after its mock context was canceled. Both hung the Mock until the test binary timed out.
Stdlib contexts are done as soon as they are created with an expired deadline or a canceled parent, and as soon as the parent's cancel returns. Mock contexts did this in a goroutine, so Err() could still be nil, Advance failed the test on the stale event of a context whose parent was canceled, and a mock deadline firing before that goroutine ran reported DeadlineExceeded with a Canceled cause. inner no longer inherits parent cancellation, so cancel() alone picks the error and cause, preferring the parent's when it is done. Err(), Done(), and the Advance family check the parent instead of waiting for context.AfterFunc.
Stdlib contexts derived from a mock context each started a goroutine waiting on Done, so they were often still live after Advance().MustWait() or the cancel function returned. mockCtx now implements the AfterFunc method that the context package uses for custom parents, and runs the registered functions before fire or cancel returns. Nested mock contexts register the same way.
Deadline() returned mock time, which starts in 2024. Every reader we found in coder/coder, its dependencies, and the standard library compares the deadline with time.Now() or passes it to a wall-clock API, so mock contexts failed net.Dialer dials and gRPC calls immediately, disabled http.Client.Timeout and x/time/rate waits, and made stdlib timeouts derived from them never fire. Deadline() now reports the parent's deadline, or year 9999 when the parent has none, so code that requires a deadline still finds one. Done() and Err() still follow mock time. Code that computes remaining mock time from ctx.Deadline() no longer works; we found no such code.
Before Go 1.26, context.Cause reads the cause through Value without calling Err first, so Cause returned nil right after the parent was canceled until something called Err or Done. The parent-cancellation tests now observe through one path each, since every path also cancels the mock context and hid gaps in the others. The AfterFunc comment now says which derived contexts it covers.
Member
Author
|
@spikecurtis, here are the changes since your approval at 38f9e0f. Each fix commit adds tests that fail without it.
Follow-up PRs stacked on this one: #31 (traps catch the Verification:
Since e72b8ddRun counts and timings below are from one 96-CPU workspace and vary by machine.
Each fix has a test that fails without it. Three of those tests fail in some runs, not every run. On this workspace, removing the view's wait for Verified at 044f3de:
|
These sentences predate reporting wall-clock deadlines from Deadline() and read as if ctx.Deadline() returned mock time.
This was referenced Nov 3, 2026
The heading "Deadline() reports wall time" read as if the mock deadline were converted to wall time, which it is not. The README paragraph before it repeated the intro, and "Code outside the test" in the godoc was vague: net.Dialer and gRPC run inside the code under test.
Two comments said cancel() must run without Mock.mu because parent.Err() takes it, but cancel() takes Mock.mu itself whatever the parent is. The Done/Err/Value comment blamed context.AfterFunc alone, but a mock parent canceled before the child registered also notifies from a goroutine. The AfterFunc comment listed only fire and the cancel function as cancelers, leaving out parent cancellation and test cleanup. Also reflow the two comments still wrapped at about 80 columns.
ParentAlreadyCanceled and CanceledMockParent tested one contract with two kinds of parent. CanceledMockParent's goroutine and timeout guard could not report the deadlock it guards against: the test reached t.Fatal and then hung in the Mock cleanup, which needs the deadlocked Mock.mu, so the run still ended in go test's timeout panic. Calling WithTimeout directly gives the same panic, and it names the subtest. The mock parent now carries its own cause, so that case also fails when the parent's error and cause stop winning. The Peek check could not fail, because Err() and Peek both cancel the context first.
The goroutine and timeout guard could not report the deadlock: the test reached t.Fatal and then hung in the Mock cleanup, which needs the deadlocked Mock.mu, so the run ended in go test's timeout panic either way. Calling Wait directly gives the same panic with less code. The test moves next to the other TickerFunc tests, and its name now says what it covers.
Both check the parents of mock contexts before reading the schedule, as Advance and Peek do, but no test failed when either check was removed.
Mock contexts and stdlib contexts derived from them are done by the time Advance().MustWait() returns, but FiresOnAdvance and WithTimeoutCause_Cause still waited on Done with a timeout. The wait hid a deadline or derived cancellation that lands after MustWait returns: with either made asynchronous, the old WithTimeoutCause_Cause passed in 200 of 200 runs.
Logging and tracing code often adds a context.WithValue layer, or a struct that embeds the context, between a mock context and the context.WithCancel that code under test derives from it. The context package forwards AfterFunc only from the direct parent, so such descendants waited for a goroutine and were usually still live after cancel() or Advance().MustWait() returned: 2000 of 2000 runs after cancel(), 1963 of 2000 after MustWait. Mock contexts now return a view whose Done and Value come from a stdlib context.WithCancel of the internal context. Stdlib finds that cancelCtx through any layer that forwards Done and Value and attaches descendants to it directly, so they are done when it is. cancel() also cancels it, so it returns after descendants are done when an expiry on another goroutine is still in progress. A stdlib parent's cancellation still reaches stdlib descendants of a mock context through a goroutine. The context package notifies non-stdlib children only that way.
A goroutine that outlives its test can still call WithTimeout. The context registered its test-end cleanup with a test whose cleanups had already run, so nothing canceled it and the goroutine could block on it forever. The context is now returned already canceled with cause ErrMockClockTestEnded.
When cancel() ran while another goroutine was canceling the context, c.cancel returned early and stdCancel canceled std with context.Canceled before that goroutine reached std. The context and its descendants then lost the expiry's DeadlineExceeded and custom cause, or a canceled parent's cause. c.cancel now cancels derived contexts, std first, before it calls into the parent to unregister, as stdlib does. A cancel() that loses the race waits for std instead of overriding it, so the wait never depends on a slow parent. TestWithTimeout_CancelDuringExpiry is now TestWithTimeout_CancelWhileCanceling and checks the exact error and cause for an expiry and for a canceled parent.
The test is now a table over the two ways to create a context after NewMock's cleanup has run. The new case checks the context inside the cleanup: without the fix, the context's own cleanup cancels it later in the same pass, so a check after the subtest returns would pass anyway.
A mock context recognized a mock parent only when the parent was the direct one. Behind a context.WithValue layer or an embedding wrapper it registered through context.AfterFunc and learned of the parent's cancellation on a goroutine, so after the outer cancel() returned it and its descendants were still live in 2000 of 2000 runs. newMockCtx now finds the nearest mock context through a private Value key and registers with it when Done is the same channel. A layer with its own Done, such as context.WithCancel, can be canceled on its own, so a context behind it still registers with that layer.
TestWithTimeout_FiresOnAdvance and TestWithTimeoutCause_Cause set up the same expiry and differed only in the constructor and the cause they expected. FiresOnAdvance is now a table over WithTimeout and WithTimeoutCause that checks the context and every descendant for the error and the cause.
"Parent cancellation propagates with stdlib semantics" promised more than mock contexts deliver: when a parent from the context package is canceled, Err reports it at once, but derived contexts and an earlier Done channel close only when a goroutine runs. The Clock and README docs now say what is done when cancel() or MustWait() returns, name that exception, and say how the test's end cancels contexts. The ErrMockClockTestEnded godoc now says which cleanup cancels a live context. A context is canceled by a cleanup registered when it was created, not by NewMock's, so it is canceled before cleanups registered between NewMock and its creation run.
A cancel() that lost the race to an expiry or a canceled parent waited for std only. Mock contexts created from the context were notified after std, on the goroutine that was still canceling, so cancel() returned with their descendants still live in up to 198 of 200 runs. Mock children now register in their own list. The goroutine that cancels a context cancels them as one tree, closes finished, and only then unregisters the tree from its parents, which can call into user code. cancel() and fire, which Advance().MustWait() waits for, wait for finished. A child that another goroutine is already canceling is pending rather than waited for, because the tree can reach an ancestor it is already canceling.
A mock context created from context.WithCancel of a mock context registered only with that layer. Canceling the outer mock context cancels the layer synchronously, but stdlib notifies the inner mock context on a goroutine, so after cancel() or Advance().MustWait() its descendants were still live in about 1990 of 2000 runs. The inner context now also registers with the mock context it finds through Value, so that context's cancellation reaches it on the same goroutine after std has canceled the layer between them. The layer can still be canceled on its own, so the context keeps its registration with the layer too. The stdlib goroutine can claim the inner context first; the previous commit's pending children cover that.
"Wait on Done instead of checking Err" read as advice about the mock context itself, whose Err reports a canceled parent at once. The Clock and README docs now name the derived context's Done and a Done channel obtained earlier, and say the delay applies when the cancellation starts at a parent from the context package, not when it starts at a mock context. Lines added on this branch now fit 100 columns with tabs at width 4.
context.WithoutCancel forwards Value, so a mock context created behind it registers with the original mock context. Canceling that context correctly left the detached child alone, but still added the child to pending when another goroutine was canceling it, so cancel() waited for independent work. The reproducer stalled 20 of 20 runs. Only a child whose parent is canceled is part of the cancellation, so only such a child can be pending.
The tree reads each child's parent on the canceling goroutine, which calls Err of every layer between the two mock contexts, including user types. The context package makes the same assumption.
checkParent read the parent even when c was already canceled, and the cancel that followed read it twice more through Err and Cause. Each of those reads did the same one level up, so Err on a canceled chain of mock contexts cost about 4^k parent reads: 56ms at depth 10, and a canceled chain of 30 never finished. A canceled c now returns before reading the parent, which makes the read constant time at any depth. cancelTree still reads the parent of a canceled c, but those reads are now constant time too.
The comment said the context package makes the same assumption about non-blocking layers. Stdlib calls a parent's Err and Value on the canceling goroutine only for a parent with its own AfterFunc method; children of a layer over a cancelCtx attach to that cancelCtx. The rule now also names Value, which Cause calls.
The test blocks in a layer's Err, which mock contexts do not support. The blocking only holds the detached cancellation in progress; the property under test is that canceling the ancestor never reads that layer, so a stalled detached subtree does not delay it.
mafredri
added a commit
that referenced
this pull request
Nov 4, 2026
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
Removing fire's wait for finished failed no test, although Advance().MustWait() relies on it: when the expiry cancels a context.WithCancel layer, stdlib starts a goroutine that cancels the mock contexts behind it, and MustWait returned while those were still being canceled. TestWithTimeout_WaitsForMockContextsBeingCanceled now has a Cancel case and a MustWait case over chains of 20 mock contexts, long enough that removing fire's wait fails the MustWait case in 92 to 96 of 100 runs and removing cancel's wait fails the Cancel case in 100 of 100. The blocking unregister case checked that cancel does not wait for a parent's stop function, which TestWithTimeout_CancelWhileCanceling checks, so it moves there as ExpiryUnregisteringDescendant.
The 30-deep chain was a row in descendants, which also feeds checks of which contexts are done, and its comment described the regression it guards as something that had happened before. It now has its own test that cancels the root of the chain: if reading a canceled chain costs parent reads exponential in its depth, cancel hangs, and go test -timeout names the test.
Removing a registration with append(s[:i], s[i+1:]...) left a copy of the last element in the backing array, so a mock context kept the most recently stopped function, and any context it references, reachable until a later registration overwrote the slot. slices.Delete zeroes the vacated slot, as registerChild's stop function already does.
The cancel closure and the mockCtx struct comments said finished closes before cancel unregisters contexts from their parents. That is false when a mock child is still being canceled by another goroutine: cancel runs the unregister functions while finished is still open. They now say that finished does not wait for those functions. The view's checkParent comment said std is canceled by a new goroutine "while std was registering"; it now names the window: c is canceled after context.WithCancel found c's Done open and before it called c.AfterFunc. The README paragraph mixed what is done when cancel() or MustWait() returns, the exception for parents from the context package, and the test-end cancellation. Each is now its own paragraph, and the exception shows how to wait on a derived context.
Removing the view's wait for std in checkParent failed no test in 3000 runs. With one mock context, the read right after the parent's cancel usually cancels the context itself, before the goroutine that the parent's cancel started can, so the read never lands while that goroutine is between canceling the context and canceling std. TestWithTimeout_ParentCanceled now derives 200 mock contexts from the parent, and its Err, Done, and Cause rows check each of them. Removing the wait fails at least one row in 191 to 196 of 200 runs.
The snippet called cancelParent and waited on child, neither of which the prose names, so a reader had to guess how they relate to ctx. It now creates the stdlib parent, the mock context, and the derived child. The test-end sentence now fills its lines to 100 columns.
Clock.WithDeadline and the README said the context and the contexts derived from it are done when cancel() or Advance().MustWait() returns. In 300 runs under -race, a context from context.WithoutCancel was live after both in all 300, as designed, and context.WithCancel over a Context type with its own Done channel was live in 295 after cancel() and 182 after MustWait, because the context package cancels it from a goroutine. Contexts from the other context.With functions, also over WithValue or a struct that embeds the context, and from the Mock's With methods, also over that Done-owning type, were done in every run. Both now name those contexts and the Done-owning exception, worded as in the AdvanceWaiter godoc.
mafredri
added a commit
that referenced
this pull request
Nov 4, 2026
Since #30, contexts derived through context.WithValue are canceled on the canceling goroutine, so they are done when Wait returns like every other derived context. In 500 runs under -race, none of the derived contexts tried (WithValue, WithCancel or WithTimeout over WithValue, a struct embedding the context) was live after MustWait; before #30's changes, the WithValue ones were live in about a quarter of runs.
mafredri
added a commit
that referenced
this pull request
Nov 4, 2026
Clock.WithDeadline on #30 now lists which derived contexts are done when MustWait returns, so AdvanceWaiter refers to that list instead of keeping its own copy, which had also said "this Mock's" where contexts from another Mock's With methods are done too.
mafredri
added a commit
that referenced
this pull request
Nov 4, 2026
The godoc said "when the test ended", but each context's cleanup is registered on the TestingT passed to NewMock. With a Mock created from a parent test's t, a probe showed the context still live in all of a subtest's cleanups and after t.Run returned, so a subtest's t.Cleanup(srv.Close) waiting on a handler blocked on it hung. It also dropped #30's sentence on contexts created after the test ended.
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.
This change lets users create context timeouts that are scheduled on mock time by introducing four new methods on Clock. The parent context cancellation is respected, and its timeout may still run on wall time, which allows use cases like setting maximum runtime limits for tests and keeps the model simple as Quartz does not translate wall time to mock time.
With a
*Mock,Done()andErr()follow mock time, butctx.Deadline()reports the parent's deadline, or year 9999 when the parent has none. Whencancel()returns, or the expiry'sAdvance().MustWait()returns, the context is done, along with the contexts derived from it with thecontextpackage'sWithfunctions other thanWithoutCancelor with the Mock'sWithmethods, also throughcontext.WithValueand wrappers that embed the context. A context derived through aContextimplementation with its ownDonechannel may still be live, because thecontextpackage cancels it on another goroutine. A cancellation that starts at a parent from thecontextpackage is also an exception:ctx.Err()reports it at once, but derived contexts, and actx.Done()channel obtained earlier, follow a moment later, because a context outside thecontextpackage learns of a stdlib parent's cancellation only throughcontext.AfterFunc, which runs on a new goroutine. Contexts still live when the test ends are canceled with causequartz.ErrMockClockTestEnded, and contexts created after it are returned already canceled with that cause.Why
Deadline()does not report mock timeCode that reads
ctx.Deadline()almost always works in wall time. We listed every non-test reader in coder/coder, in the 2,855 packages it depends on, and in the standard library. Each one does one of the following:ok, for example to require a deadline or to add a default timeout;net.Dialer,net.Conn.SetDeadline, or a DNS or websocket client;time.Now(), ashttp.Client, gRPC,x/time/rate, and the Anthropic, OpenAI, and Google GenAI SDKs do.None of them compares it with an injected clock. The mock starts in 2024, so a mock deadline looks expired to all of them: dials and gRPC calls fail immediately,
http.Client.Timeoutand stdlib timeouts derived from the context are skipped, andrate.Waitrefuses to wait.We tested six behaviors for
Deadline()against those readers (table below). Reporting the parent's deadline, or year 9999 when there is none, is the only one that:ok == truefor code that requires a deadline;Converting the mock deadline to wall time (
time.Now()plus the mock time left) fails that last check: a 100ms mock timeout made a real 300ms read fail on wall time without anyAdvance.The cost: code that computes remaining mock time from
ctx.Deadline()gets a meaningless result. We found no such code.Deadline() options tested against consumers (parent without a deadline)
contextdocs)ok == truenet.Dialer.DialContexthttp.Client.TimeoutappliesWithTimeoutchild firescontext.WithDeadlinex/time/rate.WaitSetReadDeadlineclock.Until(deadline)gives mock time leftCloses #14
Part of ENG-2715
BREAKING CHANGE: Clock gained four new methods, which is technically a breakage of the public API. Impact should be minimal considering Clock is not meant to be faked outside of this package.