Add Deadline.Context for derived contexts - #397
Conversation
Deadline is revivable via Set, so it is not monotonic: Err goes from non-nil back to nil and Done hands out a fresh channel, both of which context.Context forbids. Deriving a context from a Deadline is therefore unsafe -- propagateCancel reads parent.Err() after observing Done, and cancelCtx.cancel panics on a nil error. Context returns a real cancel context scoped to the current deadline generation instead. It is memoized while the deadline is live, cancelled with context.DeadlineExceeded as its cause when the deadline fires, and replaced on the next call after that, so what callers hold is monotonic even though Deadline is not. Because it is a *cancelCtx, children register in its map rather than each spawning a watchdog goroutine. timeout now closes done inside the critical section via the shared fire helper, which also removes the window where Err reported DeadlineExceeded while Done was still open.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #397 +/- ##
==========================================
+ Coverage 84.18% 84.36% +0.18%
==========================================
Files 41 41
Lines 3396 3416 +20
==========================================
+ Hits 2859 2882 +23
+ Misses 398 395 -3
Partials 139 139
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| @@ -74,7 +108,7 @@ | |||
| d.pending-- | |||
| d.state = deadlineStopped | |||
There was a problem hiding this comment.
Nit here, could we rename this deadlineSuspended?
I feel Stop could be misinterpreted as cancel, whereas suspended implies the context returned by Context() is still alive
There was a problem hiding this comment.
Done in b2a362c -- renamed to deadlineSuspended, and the matching test is now SuspendKeepsContextLive.
Agreed on the reasoning: with Context() in the picture the distinction actually matters, since suspending leaves the context live while expiry cancels it. SuspendAfterExpiryGivesLiveContext pins that.
| d.ctx, d.cancel = nil, nil | ||
|
|
||
| return cancel | ||
| } | ||
|
|
||
| // Context returns a context for the current deadline, canceled with | ||
| // context.DeadlineExceeded as its Cause. | ||
| func (d *Deadline) Context() context.Context { | ||
| d.mu.Lock() | ||
| defer d.mu.Unlock() | ||
| if d.ctx == nil { |
There was a problem hiding this comment.
Does this mean we'll return a new context if the context cancelled by a deadline?
There was a problem hiding this comment.
I believe this can break SCTP if we migrate to this API. If stream's Write calls Context after the deadline has already expired, it will create a new non-canceled context. If the SCTP write is currently blocked it will then then potentially block forever....
There was a problem hiding this comment.
I think it will be a bigger problem for DTLS if we merge @noboruma PR's pion/dtls#1096 because we'll have multiple Context calls and the deadline can expire between handshake for example and contextWithClose then we'll just get an unbounded timeout. unless we create a single context and store it in conn but then Set will not work (we can make Conn reset the context everytime it calls Set but that will be an anti pattern imo).
There was a problem hiding this comment.
I think we can just check for if d.state == deadlineExceeded { and return a cancelled context.
There was a problem hiding this comment.
Good catch, and you're right about both consequences. Fixed in b2a362c.
Context() now short-circuits on the state, as you suggested:
if d.state == deadlineExceeded {
return exceededContext
}exceededContext is a single package-level context canceled with context.DeadlineExceeded at init. Cancellation is immutable, so one instance is safe to share with every caller and costs no allocation on a path that only runs after a deadline has already blown.
Three regression tests cover it, and all three hang for the full assertion timeout against the old code, which is exactly the symptom you described:
ExceededReturnsCanceledContext--Set(past)thenContext()ExceededByTimerReturnsCanceledContext-- same via the timerDerivedFromExceededIsCanceled--context.WithCancel(d.Context())is canceled immediately, with the cause propagating through asDeadlineExceeded
SuspendAfterExpiryGivesLiveContext pins the other direction: once Set re-arms or suspends, Context() goes back to handing out a live one.
Context() minted a fresh, live context whenever the deadline had already fired, because fire() clears the memoized one. A caller that asks for a context after expiry and only consults it while blocked would then wait forever: an SCTP stream Write, or a DTLS handshake whose deadline elapses before contextWithClose, would see an unbounded timeout instead of a cancellation. Short-circuit on the exceeded state and return a shared, pre-canceled context. Cancellation is immutable, so one package-level instance is safe to hand to every caller and costs no allocation on a path that only runs after a deadline has already blown. Also rename deadlineStopped to deadlineSuspended: "stopped" reads like "cancelled", where the point is that Context() stays live in that state.
3 participants
deadline.Deadlineis revivable viaSet, so it is not monotonic, andcontext.Contextrequires that it be:After a
Deadlineexpires and is set again,Err()goes fromDeadlineExceededback tonilandDone()hands out a fresh open channel while earlier holders still see the old one closed — two observers of the same "context" disagreeing permanently.That makes deriving a context from a
Deadlineunsafe. Both propagation paths incontextreadparent.Err()after observingDone:and
cancelCtx.cancelpanics on a nil error, before its already-cancelled early return. ASetlanding in that window panics inside the standard library:This reproduces on
maintoday with no callback API in the tree.This PR
Context()returns a real cancel context scoped to the current deadline generation, so what callers hold is monotonic even thoughDeadlineis not:context.DeadlineExceededas itscontext.CauseBecause the returned value is a
*cancelCtx,parentCancelCtxfinds it and children register in its map — deriving 100 contexts spawns 0 goroutines, versus one watchdog goroutine per child for any parentcontextcannot recognise.timeoutandSetnow share afirehelper, which also closesdoneinside the critical section and removes the pre-existing window whereErr()reportedDeadlineExceededwhileDone()was still open.Purely additive —
Set,Done,ErrandDeadlineare untouched and thecontext.Contextimplementation is left in place. #398 removes that separately.Testing
TestDeadlineContextcovers memoization, extend, stop, timer and past-deadline expiry, generation replacement with the retired context staying cancelled, derived-context cancellation, the zero-goroutine property, and concurrentContext/Set. Green under-race.