-
Notifications
You must be signed in to change notification settings - Fork 5
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
fix(blink): resend signals to ensure all peers connect #358
Conversation
dial peers who aren't connected and have a lesser DID
|
||
impl Drop for NotifyWrapper { | ||
fn drop(&mut self) { | ||
self.notify.notify_waiters(); |
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.
you should probably check the strong count of the reference so if its the only thing left, it can signal its waiters. Eg
if Arc::strong_count(&self.notify) == 1 {
self.notify.notify_waiters();
}
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.
notify
gets cloned and passed to some tasks. I think that dropping NotifyWrapper
wouldn't necessarily mean the strong count would be one. I hoped that by putting NotifyWrapper in an Arc, Drop wouldn't be called until all the Arc<NotifyWrapper>
s were dropped.
What this PR does 📖
blink_controller.rs
instead of split between the blink implementation,webrtc_controller.rs
(removed) andcall_initiation.rs
(also removed).the new design made it easy to add the following improvements:
other improvements
BlinkImpl
is cloned and that clone is dropped, it won't shut down the background tasks anymore.ParticipantState
in theAnnounce
event and removing the other signals. Blink now has an event forParticipantStateChanged
.Which issue(s) this PR fixes 🔨
Not an issue in Warp but should address the issue where, especially during a group call, not all the participants can hear each other.
Special notes for reviewers 🗒️
Additional comments 🎤