-
Notifications
You must be signed in to change notification settings - Fork 95
Ensure FFI events are processed sequentially #547
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: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 15a2fbc The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
This is pretty much the same as pythons implementation looks good to me
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.
lgtm, two thoughts. Thanks for fixing the problems.
return ev.message.case == 'publishTrack' && ev.message.value.asyncId == res.asyncId; | ||
}); | ||
try { | ||
const cb = await FfiClient.instance.waitFor<PublishTrackCallback>((ev) => { |
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.
curiously, does FfiClient.instance.request guarantee that it will trigger a PublishTrackCallback later ?
There is no need to change it now, but just an idea, I wonder if it makes sense to do things like
- prepare a waiter
- roomEventLock.lock()
- FfiClient.instance.request
- waiter.setAsyncId(res.asyncId)
- waiter will wait with some timeout, like await waiter.wait({ timeoutMs: 50 }); // not sure if that will bring more flakiness or less here.
if (ev.case == 'participantConnected') { | ||
const participant = this.createRemoteParticipant(ev.value.info!); | ||
this.remoteParticipants.set(participant.identity!, participant); | ||
this.emit(RoomEvent.ParticipantConnected, participant); |
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.
I have some concerns on this emit event since it is fired under the lock, and I wonder if it is a better pattern if we do things like
const unlock = await this.roomEventLock.lock();
const toEmit: Array<() => void> = [];
try {
// Push all the emit events in sequence to the array
toEmit.push(...);
} finally {
unlock();
}
// Fire the emit events
for (const fire of toEmit) {
try {
fire(); // consider await fire() ? if we need them to be synchronous
} catch ...
}
it is a suggestion, no need to work on it if you think the emit events are safe anyway.
For some actions (i.e. publishing + unpublishing a track) the local participant needs to acquire the processing lock to ensure any state updates are done before emitting other events on the room level.