-
-
Notifications
You must be signed in to change notification settings - Fork 580
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 race in creating calls #2663
Conversation
Second attempt, just put a mutex around the method that processes member state events. Also more logging. Supersedes #2662
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.
Otherwise, LGTM
@@ -185,6 +185,9 @@ export class GroupCall extends TypedEventEmitter< | |||
private resendMemberStateTimer: ReturnType<typeof setInterval> | null = null; | |||
private initWithAudioMuted = false; | |||
private initWithVideoMuted = false; | |||
// We use this as a set of mutexes, effectively, to make sure we only enter the function | |||
// to process a member state event once at any one time for each member | |||
private processingMemberStateEvent = new Map<string, boolean>(); |
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.
Why is this a map? Wouldn't a Set
work?
try { | ||
return this.processMemberStateEventInternal(event); | ||
} finally { | ||
this.processingMemberStateEvent.delete(event.getStateKey()); | ||
} |
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.
What's the benefit of try/finally here?
Also, what if the event changes while we're processing it? |
Hmm, yes, you're right. We need to handle the newer ones somehow. |
Going back to #2662 again |
Second attempt, just put a mutex around the method that processes member state events. The previous change meant that uninitialised calls were sitting in the map which is not a thing this code is prepared to handle and was causing test failures.
Also more logging.
Supersedes #2662
Checklist
Here's what your changelog entry will look like:
🐛 Bug Fixes