Add AfterTimeout to Deadline by noboruma · Pull Request #396 · pion/transport

noboruma · 2026-09-02T15:13:13Z

Add AfterFunc to deadline to allow for continuation on when deadline are reached.
Convenient for avoiding long running goroutines with connection contexts.

This change is needed for: pion/dtls#1096

codecov · 2026-09-02T15:19:51Z

Codecov Report

❌ Patch coverage is 92.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.46%. Comparing base (af01cdd) to head (f106894).

Files with missing lines	Patch %	Lines
deadline/deadline.go	92.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #396      +/-   ##
==========================================
+ Coverage   84.18%   84.46%   +0.27%     
==========================================
  Files          41       41              
  Lines        3396     3418      +22     
==========================================
+ Hits         2859     2887      +28     
+ Misses        398      393       -5     
+ Partials      139      138       -1

Flag	Coverage Δ
go	`84.46% <92.00%> (+0.27%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

paulwe · 2026-09-06T09:48:28Z

 	if setTo.IsZero() {
 		d.pending--
 		d.state = deadlineStopped
+		clear(d.cbs)


clearing d.cbs here breaks the context.afterFuncer contract - there's no way to disambiguate "already fired" from "discarded" when the cancel func returns false. it also orphans derived contexts eg. from context.WithCancel(d).

Fair enough, if we consider putting to 0 was akin to be indefinitely running, however this indeed breaks the case where the deadline would be reset after that.

However I was under the impression here we are supporting the stopped state since we have this deadlineStopped state. Maybe we should not support stopping then? (either we are starting or started or exceeded)

paulwe · 2026-09-06T09:48:29Z

 		assert.Equal(t, expectedCalls, calls, "Wrong order of deadline signal")
 	})
+
+	t.Run("DeadlineAfterFuncExceedFuture", func(t *testing.T) {


these can probably be a table test eg.{name, ops func(*Deadline, time.Time), want int32} with additional cases to cover AfterFunc(f) while already exceeded, context.WithCancel(d) cancel propagation, and multiple callbacks.

I have added tests for already exceeded and multi callbacks
Cancel is out of Deadline scope if I am not mistaken?
That's the whole reason we need AfterFunc-like behavior actually

Happy to move the tests into a table, but does it not make error tracing harder?

paulwe · 2026-09-06T10:00:21Z

+// AfterFunc attaches a function to the deadline.
+// The functions will be triggered on deadline exceeded.
+// If the deadline is reset, the functions are skipped.
+// Attached functions are only triggered once.


the wording is unclear because there is no "reset" function and Set(zero) and Set(futureTime) have opposite effects on the pending callbacks. the cancel func semantics should be documented. it should be stated that this implements the context.afterFuncer interface so someone doesn't change the signature later and silently break it

I have reworded the whole accordingly (and also renamed the function as per Jo's comment)

paulwe · 2026-09-06T11:04:50Z

 	done := d.done
+	for _, cb := range d.cbs {
+		go cb()
+	}
+	clear(d.cbs)
 	d.mu.Unlock()

 	close(done)


my code here is wrong: context.go:111

// If Done is not yet closed, Err returns nil.

close(d.done) should move into the critical section above the dispatch loop so callbacks cannot observe done unclosed.

Interesting, I now wonder if this is the root cause of the issue that triggered this whole PR.
I was using this struct directly with context.AfterFunc but was hitting panics because of Err() returning nil.

Let me double check that once again, maybe we can discard the whole PR and only keep the fix

So actually by design Deadline cannot be used for context.AfterFunc because of it's mutable nature, whereas context.AfterFunc relies on the fact the context cannot change once it's "done".
So we still need our own API, as proposed

it's worse than i originally thought - implementations of context.Context must be monotonic state machines. stdlib can panic when a context is derived from a Deadline that is subsequently reset whether it implements context.afterFuncer or not. here is a failing test demonstrating the bug: gist

@paulwe this is pre-existing, Set already violates the context.Context contract, and the panic can already happen, the linked dtls pr shouldn't trigger this panic with AfterTimeout but since this is public I support fixing this and pushing a new transport major release if needed.

because the incompatibility is irreconcilable i don't think Deadline should pretend to implement context.Context. #397 implements a helper for deriving a real context from the deadline and #398 breaks the interface.

paulwe · 2026-09-06T11:20:45Z

+		d.Set(now.Add(10 * time.Millisecond))
+		<-time.After(20 * time.Millisecond)
+		d.Set(now.Add(10 * time.Millisecond))
+		<-time.After(20 * time.Millisecond)


10/20ms margins will probably flake in CI. also double check that this is taking the expected path...

Ideally we should be able to control a fake clock but I have simply copied from above tests.
Maybe I can do better with channel synchronization instead, otherwise do you suggest higher timers?

usually 100-200ms is enough slack for oversubscribed gh test runners. if the numbers you chose are found elsewhere in the package i retract my suggestion though

We should start using synctest for time tests like this, it should be beind a 1.25 go build flag.

JoTurk · 2026-09-06T12:27:30Z

@noboruma I thought about this a lot and the dtls pr l, I think the cleanest approach is to avoid implementing context.afterFuncer on Deadline. Could this instead expose something like OnDeadline or AfterDeadline have DTLS use that directly? This also give us the freedom to use a different notify design other than a callback.
I'll also push to your other dtls pr to fix some minor issues and try to get it merged today and I can help with this.

noboruma · 2026-09-11T13:31:14Z

So with #397 this PR is now obsolete.
We can simply start using context.AfterFunc using the returned Context().

noboruma force-pushed the deadline-afterfunc branch 2 times, most recently from 9a2ebc9 to 784a23b Compare September 2, 2026 15:16

noboruma force-pushed the deadline-afterfunc branch 4 times, most recently from 6129112 to 5b8ecf7 Compare September 2, 2026 15:43

noboruma mentioned this pull request Sep 2, 2026

Optimize allocations for handshake and compaction pion/dtls#1096

Draft

noboruma requested a review from paulwe September 2, 2026 23:16

paulwe reviewed Sep 6, 2026

View reviewed changes

noboruma force-pushed the deadline-afterfunc branch 2 times, most recently from 5983fa3 to 16b97bc Compare September 7, 2026 14:41

Add AfterTimeout to Deadline

f106894

noboruma force-pushed the deadline-afterfunc branch from 16b97bc to f106894 Compare September 7, 2026 14:45

noboruma changed the title ~~Add AfterFunc to Deadline~~ Add AfterTimeout to Deadline Sep 7, 2026

noboruma requested a review from paulwe September 8, 2026 15:07

noboruma mentioned this pull request Sep 10, 2026

Add Deadline.Context for derived contexts #397

Merged

noboruma closed this Sep 11, 2026

Uh oh!

Conversation

noboruma commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noboruma Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noboruma Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noboruma Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulwe Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoTurk Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulwe Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoTurk commented Sep 6, 2026

Uh oh!

noboruma commented Sep 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

noboruma commented Sep 2, 2026 •

edited

Loading

codecov Bot commented Sep 2, 2026 •

edited

Loading

noboruma Sep 7, 2026 •

edited

Loading

noboruma Sep 7, 2026 •

edited

Loading

noboruma Sep 7, 2026 •

edited

Loading

paulwe Sep 9, 2026 •

edited

Loading

JoTurk Sep 9, 2026 •

edited

Loading

paulwe Sep 9, 2026 •

edited

Loading