Add deterministic UDP peer preparation - #585
Conversation
eca4dde to
674a4cf
Compare
Client.PrepareUDPPeer(ctx, peer) creates a permission and waits until the TURN server confirms a ChannelBind for the peer. After it returns nil, writes to that peer use ChannelData or fail for the lifetime of the allocation; they never silently fall back to Send indications. Failed or expired bindings and permission refresh failures terminalize readiness and fail subsequent writes. Per maintainer suggestion, this extends the existing binding manager instead of adding a new transaction/allocation lifecycle: a bind attempt is tracked by a per-binding channel under the existing muBind mutex, so same-peer callers coalesce onto one shared attempt and a caller's context cancellation wakes only that waiter, leaving the shared work running. Peer addresses are reduced to a canonical form so aliases (IPv4-mapped IPv6, zoned addresses) share one permission and one channel binding. UDPConn.Close now returns only after allocation-owned goroutines have finished: the three refresh timers are stopped and joined, and bind or permission workers are tracked by a WaitGroup gated against close. The internal ChannelBind 400 close path uses a join-free startClose so a worker never joins itself. The client still never closes the caller-owned socket or touches its deadlines. With an unresponsive server, Close latency is bounded by the STUN retransmission budget, since in-flight transaction waits are not interruptible; prompt interruption is deliberately left out to keep this change minimal.
674a4cf to
ff1ce8f
Compare
|
@JoTurk reworked per the Discord discussion — thanks for the pointer. The original approach is dropped entirely; the branch now reuses and extends the current binds with the existing mutex covering the bind, and no new transaction or allocation lifecycle. Where it landed: ~447 non-test lines including comments (~320 code lines). That covers both readiness ( Tests, all under |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #585 +/- ##
==========================================
- Coverage 82.32% 81.97% -0.36%
==========================================
Files 46 46
Lines 3304 3517 +213
==========================================
+ Hits 2720 2883 +163
- Misses 381 416 +35
- Partials 203 218 +15
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:
|
A second PrepareUDPPeer caller for the same peer could block on the permission mutex, held across the whole CreatePermission transaction, instead of the cancellation-aware attempt channel. Move attempt bookkeeping to its own mutex so waiters always block in the select and caller cancellation works during an in-flight permission exchange. Add a regression test using a delayed permission transaction.
|
Heads up — while poking at the close wait-time gap I mentioned on Discord, I found a bug in this branch: a second While I was in there I started putting together that wait-time follow-up for after this lands. Shape so far:
|
TestPeriodicTimer/stop_inside_handler asserted IsRunning 30ms after starting a 20ms timer, so on a slow runner the check could run before the handler had fired, failing with "should not be running" (seen on the macOS CI runner). A fired handler that calls Stop can never leave the timer running: Stop clears stopFunc under the mutex before it returns, and IsRunning reports stopFunc. Signal from the handler after it calls Stop and assert once the signal arrives, instead of calibrating a sleep against the timer interval. The timer behavior under test is unchanged.
|
The macOS 1.25 failure is a flake in the pre-existing |
|
@JoTurk let me know if you'd rather I squash/force these. |
|
@the-sarge It's fine to keep the commits we can always squash merge or do it before merge. |
|
No worries - I had no idea how many different projects you guys had going on! Impressive. Thanks for all of it. |
|
I decided to go a different way so I could make progress...feel free to close this if you don't want it. |
2 participants
Summary
Add
Client.PrepareUDPPeer(ctx, peer): an opt-in way for applications to wait until a UDP peer has a confirmed permission and ChannelBind before sending application packets.This is a rework of the original diff. It now reuses and extends the existing binding manager, with the existing mutexes covering the bind, instead of introducing a new transaction/allocation lifecycle. The non-test diff is 447 insertions / 23 deletions (~320 code lines excluding comments and blanks), plus 340 lines of tests.
Motivation
Rework
Rebuilt per JoTurk's suggestion on Discord (make the existing mutex cover the bind; reuse/extend the current binds; no new transaction and allocation lifecycle):
muBind.PrepareUDPPeerwaiters select on that channel, so same-peer callers coalesce onto one shared attempt and a caller's context cancellation wakes only that waiter with its cause, leaving the shared attempt running. The binding state machine itself is unchanged.terminalizetransition folded into the binding's existing mutex, so a completing bind attempt cannot resurrect a permanently failed binding, and anonPermRefreshFailurecallback on the allocation, because permission refresh failures were previously log-only but readiness must observe them.PeriodicTimergainsStopAndWait, and bind/permission workers are tracked by a WaitGroup gated by the existingcloseMutex.Proposed behavior
PrepareUDPPeer(ctx, peer)resolves once the permission and ChannelBind for that peer are confirmed. After it succeeds, writes to that peer use ChannelData or return an error for the lifetime of the allocation — never a silent fallback to Send Indications — so the packet profile is deterministic before the first application byte.Closereturns. Socket interruption remains the caller's responsibility: the client never closes the caller-ownedClientConfig.Connor mutates its deadlines, soClosecompletes once the socket owner permits or actively unblocks in-flight I/O.Closecan take up to the STUN retransmission budget (~63.5 s with defaults) while an in-flight transaction runs out, because transaction waits are not interruptible in this minimal version. Prompt interruption (context-aware transaction waits) is deliberately left out to keep this PR small.Compatibility
PrepareUDPPeer, with one intentional refinement:WriteTonow canonicalizes*net.UDPAddraliases of the same peer so they share one permission and one channel binding.Testing
go test -race -count=1 ./internal/client .passes.golangci-lint runwith the repo config reports no new findings versusmain.-race): readiness success then ChannelData-only writes; invalid-peer rejection (unspecified, multicast, zoned, bad port, non-UDP); peer aliases sharing the prepared binding; same-peer coalescing onto one bind and one permission; waiter-local cancellation with its cause while the shared bind survives; permission refresh failure failing writes with no Send-indication fallback; bind failure surfacing to the preparing caller; terminal failure surviving an in-flight bind success;Closejoining in-flight bind workers.