Add gathering generation extension to candidates - #874
Conversation
| if prevState == GatheringStateGathering { | ||
| a.candidateNotifier.EnqueueCandidate(nil) | ||
| } |
There was a problem hiding this comment.
This is a slight departure from the previous behavior that was part of the base patch from @JoTurk
I'm not sure if this behavior (only send nil candidate when transitioning from Gathering to Complete) is more correct, or if we should keep the original behavior (send nil candidate anytime we transition into the Complete state)
There was a problem hiding this comment.
yeah, the reason i added this is because i just wanted to make the test pass (because the previous pr did close => complete), but sending nil regardless is correct i think.
There was a problem hiding this comment.
this is because we don't go to complete from close, like in the previous pr.
There was a problem hiding this comment.
If I'm reading this right it looks like the previous behavior of sending the nil candidate anytime we enter Complete as long as the previous state wasn't also Complete is in line with Chrome's behavior.
|
Converted to draft since I missed that I should be piping generation through the call stack from |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #874 +/- ##
==========================================
+ Coverage 88.04% 88.31% +0.27%
==========================================
Files 46 46
Lines 6507 6498 -9
==========================================
+ Hits 5729 5739 +10
+ Misses 532 516 -16
+ Partials 246 243 -3
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:
|
|
Thank you I'll check again how chrome and firefox handle generations and review this. this pr is continuation of pion/webrtc#3113 |
8b913a0 to
6f163a5
Compare
ecd2263 to
d5aaf7d
Compare
d5aaf7d to
5dad9ca
Compare
|
@boushley sorry I lost track of this, Will try to get back to you tomorrow. |
|
Ha! No problem, I only got around to fixing the linter errors today. Thanks for all you do! |
|
@boushley Is this ready for review? or quick one? |
|
I think we need to address the possible dead lock issue with the notification from Agent loop, but I need to find time to get back into the headspace to address that. |
|
Sounds good :) |
|
I'm open to ideas of how we should handle the concurrency issues here. I worry about the current state because it could potentially result in a deadlock where the Agent loop gets hung up trying to acquire the notifier lock, although I don't think there's a place where the notifier lock is dependent on the Agent loop, I haven't confirmed that side. I also don't see a clean way of getting the notification with the Should I dig in on what exactly the Notifier lock can hang on to gain confidence we're safe on that side? Or can you think of a cleaner way of getting the notification out on a less critical go routine? |
5bc1d13 to
3405c8f
Compare
|
Alright @JoTurk I got rid of the nested locking. After a deeper analysis I determined that it was safe in the current configuration since the notifier releases the Mutex before executing any user code. However I didn't want to depend on that so I went with this route. If you'd prefer I move the notification back into the agent loop I can do that and just document the assumptions in the code around the notifier to try and ensure that assumption remains accurate. |
|
@boushley I'm so sorry i lost this from my todo (again), I think your approach is the correct one, If you don't mind fixing the conflicts and I'll get into reviewing and testing this. |
719d682 to
a4f638d
Compare
3f0ef77 to
9a72213
Compare
Populate the generation extension on candidates added to ICE Agent.
9a72213 to
603539f
Compare
4 participants
Description
Populate the generation extension on candidates added to ICE Agent.