Drop context.Context implementation from Deadline - #398
Open
paulwe wants to merge 3 commits into
Open
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.
paulwe
force-pushed
the
deadline-drop-context-iface
branch
from
September 9, 2026 12:35
9f4abaf to
abfbdd3
Compare
paulwe
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## deadline-context #398 +/- ##
====================================================
+ Coverage 84.36% 84.41% +0.04%
====================================================
Files 41 41
Lines 3416 3414 -2
====================================================
Hits 2882 2882
+ Misses 395 393 -2
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:
|
noboruma
approved these changes
Sep 10, 2026
JoTurk
reviewed
Sep 10, 2026
Comment on lines
-162
to
-164
| func (d *Deadline) Value(any) any { | ||
| return nil | ||
| } |
Member
There was a problem hiding this comment.
we should do a major release when we merge this.
Contributor
Author
There was a problem hiding this comment.
Agreed -- removing a method from an exported type is breaking, so this wants a major bump. Flagged in the PR body too; happy to leave the sequencing to you.
Worth noting the two downstream call sites this turns into compile errors still need fixing before it can land: pion/dtls conn.go:586 and pion/sctp stream.go:330. Both become .Context() calls once #397 is in.
Member
There was a problem hiding this comment.
I think we can get more changes to pion/transport and release transport@v5 too. There are other stuff that we can improve.
Contributor
Author
There was a problem hiding this comment.
sgtm. do we want to hold on landing this one?
Member
There was a problem hiding this comment.
I think yeah, so other people don't accidentally tag it in v4 by mistake
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.
Deadline never satisfied the Context contract. Set revives it, so Err goes from non-nil back to nil and Done hands out a new channel while earlier holders still see the old one closed: two observers of the same "context" disagreeing permanently. Any context derived from it can panic in the standard library, because both propagateCancel paths read parent.Err() after observing Done, and cancelCtx.cancel panics on a nil error. This reproduces today with no callback API involved. Dropping the no-op Value method breaks the interface assertion and turns those derivations into compile errors. Callers that want a context should use Deadline.Context, which is monotonic. Set, Done, Err and Deadline are untouched: across pion/transport, pion/dtls and pion/sctp the only methods invoked on a Deadline are Set and Done. Known downstream fallout, both passing a Deadline to an unexported function that takes a context.Context: pion/dtls conn.go:586 c.contextWithClose(c.writeDeadline) pion/sctp stream.go:330 sendPayloadData(s.writeDeadline, chunks)
paulwe
force-pushed
the
deadline-drop-context-iface
branch
from
September 10, 2026 10:25
abfbdd3 to
fee5d42
Compare
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
3 participants
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 #397 — review that one first; this PR's diff is the single commit on top.
Deadlinenever satisfied theContextcontract, for the reasons in #397:Setrevives it, soErr()goes from non-nil back to nil andDone()returns a new channel while earlier holders still see the old one closed, and any context derived from it can panic insidecontextwithmissing cancel error.var _ context.Context = (*Deadline)(nil)asserts a contract the type provably breaks. Now that #397 offersDeadline.Context()as the honest alternative, this drops the claim: deleting the no-opValuemethod breaks the interface, which turns every derivation from aDeadlineinto a compile error instead of a runtime panic.Set,Done,ErrandDeadlineare all kept — acrosspion/transport,pion/dtlsandpion/sctpthe only methods ever invoked on aDeadlineareSetandDone. Nothing in any of the three exposes a*deadline.Deadlinein exported API; every holder is an unexported struct field.Blast radius
Measured by building all three repos against this branch with
-gcflags=-e, test packages included. Exactly two call sites break, both passing aDeadlineto an unexported function that takes acontext.Context:conn.go:586—c.contextWithClose(c.writeDeadline)stream.go:330—sendPayloadData(s.writeDeadline, chunks)pion/transportitself needs no changes: builds, vets and passes all tests.Both are mechanical, and both sites are already coping with the unsoundness:
contextWithClose(dtlsconn.go:858) doescontext.WithCancelCause(context.WithoutCancel(ctx))and then hand-rolls the propagation in a goroutine witherr := ctx.Err(); if err == nil { err = context.DeadlineExceeded }— that nil guard is the non-monotonicity, already observed and patched at the symptom. It can takec.writeDeadline.Context()and drop both theWithoutCanceland the guard, or become its siblingcontextWithCloseAndWriteDeadline, which already watchesc.writeDeadline.Done()as a plain channel.sendPayloadDatadoescase <-ctx.Done(): return ctx.Err(). With a revivedDeadlinethat returns nil, so an aborted write currently reports success — a silent wrong result onmastertoday, which this change converts into a compile error.Note on versioning
Removing a method from an exported type is formally breaking for
transport/v4, so this may want a major bump — or the view that a type which never satisfied the contract was never legitimately aContext. Flagging for maintainers rather than assuming.