-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add continual gathering policy to SettingEngine #3323
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
@joeturki I know we said we would discuss the API in the issue, but this seemed so straightforward that I made a quick PR. I have no issues with completely rewriting this, but I thought it would aid the discussion to have a rough draft. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3323 +/- ##
==========================================
+ Coverage 85.02% 85.04% +0.01%
==========================================
Files 80 80
Lines 9512 9520 +8
==========================================
+ Hits 8088 8096 +8
- Misses 1001 1002 +1
+ Partials 423 422 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
JoTurk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that this changes fundamental ICE behavior without defining the observable user contract. We agreed to add this to 4.3 but we need to define behavior.
If we just merge this we'll introduces undefined behavior (UB) around:
-
OnICECandidate(nil)
Today, many apps treat nil as terminal. Continual gathering breaks that assumption unless behavior is redefined. will we send nil after every iteration? will we not send nil at all? we need to define and test this. -
ICEGatheringState transitions becomes incoherent If the state reaches complete, but candidates can still appear later, the state machine lies. we don't define how the state will be in the peer. and we don't test it either.
so we have two assumptions either:
-
the state must be allowed to transition back to gathering, or
-
complete must be redefined under continual mode.
The PR does assert neither.
we will also need to add vnet tests around this similar to how we did with renomination.
|
That was quick, thank you Joe! Though, lets think about an alternative! We could also treat continual gathering as lots of mini ice-restarts. In that case it would make sense to do a bunch of state transitions and sending nil, but it needs change in pion/ice. I am tending towards the first approach as it seems semantically more aligned. Happy to add vnet tests! |
|
@joeturki Added the vnet test. Ones |
|
I commented on the transport PR 🙏 |
657e133 to
3c656ef
Compare
3c656ef to
58f18ba
Compare
|
@JoTurk vnet test enabled and ready for review. |
Description
Expose pion/ice's ContinualGatheringPolicy via the SettingEngine.
See #3321
I thought setting the gathering policy through the settings engine would be the best place for this as I am not aware of any w3c standard api.