Add gather generations and tag candidates - #945
Open
maum-meliora wants to merge 4 commits into
Open
Conversation
An ICE generation's local credentials and gathering state were held as separate Agent fields, so a restart updated them piecemeal. An in-flight gather cycle could then read a credential the restart had already rewritten — a data race — or write gathering state the restart had just reset. Introduce iceGeneration, which bundles the generation id, local credentials, and the gathering state. - Restart replaces the active *iceGeneration on the event loop instead of updating fields piecemeal. - A gather cycle captures the generation it started under and reads that generation's credentials, immutable for the life of the cycle, so off-loop gather goroutines never touch shared credential fields — removing the data race on restart. - The gather goroutine's cancel/done handles stay on the Agent rather than the generation, so Close still joins a cycle that a restart superseded before a new gather began.
Tag every gathered candidate with a "generation" extension holding the candidate's gather generation id — the attribute libwebrtc-based browsers emit so peers can tell candidates of a restarted gather cycle apart from the previous cycle's. - A cycle superseded by a restart still tags its own generation, not the agent's current generation. - Only an ICE restart advances the generation — a continual-gathering re-gather reuses its cycle's generation — and the initial gather is generation 0, matching libwebrtc. - The marshaled candidate string gains one additive token; peers that follow RFC 5245 extension parsing ignore unknown extensions. Continues pion#874 by @boushley.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #945 +/- ##
==========================================
- Coverage 88.41% 88.21% -0.20%
==========================================
Files 46 46
Lines 6291 6278 -13
==========================================
- Hits 5562 5538 -24
- Misses 497 504 +7
- Partials 232 236 +4
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:
|
…ension # Conflicts: # agent_test.go
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.
Follows up on #874 by @boushley, carrying the same goal forward with a restart-scoped implementation aligned with libwebrtc.
Tag every gathered candidate with a
generationextension holding thecandidate's gather generation id — the attribute libwebrtc-based
browsers emit so peers can tell candidates of a restarted gather cycle
apart from the previous cycle's.
Two commits:
and gathering state were separate Agent fields that a restart
updated piecemeal — an in-flight gather cycle could read a
credential the restart had already rewritten, a data race. The new
iceGenerationtype bundles the generation id, credentials, andgathering state;
Restartswaps the active generation on the eventloop, and a gather cycle reads credentials only from the generation
it captured at start, immutable for the life of the cycle.
generationcandidate extension. A superseded cycletags its own generation, not the agent's current generation. Only
an ICE restart advances the generation — a continual-gathering
re-gather reuses its cycle's generation, and the initial gather is
generation 0 — matching libwebrtc.
Behavior notes
a=candidatelines) gainone additive
generation Ntoken; peers that follow RFC 5245extension parsing ignore unknown extensions.
Closejoins a gather cycle that a restart superseded when no newgather followed; restart-then-regather leaves the previous cycle to
context cancellation, unchanged from current behavior.
Testing
canceled cycle's state write is dropped with no late
end-of-candidates;
Closejoins a superseded gather. A vnet testasserts
generation 0, thengeneration 1after a restart.-race;golangci-lintclean. End-to-endagainst pion/webrtc main (local replace): SDP candidate lines carry
generation 0, thengeneration 1after an ICE restart.Review focus
this PR advances the generation only on ICE restart, matching
libwebrtc (a generation is an allocator session, created only on
credential change —
p2p/base/p2p_transport_channel.cc). For theflapping scenario discussed in Add gathering generation extension to candidates #874, libwebrtc prunes failed-network
ports and relies on connectivity checks rather than generation
filtering; equivalent cleanup is out of scope for this PR.