Skip to content
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

[MM-50751] Fix race condition attaching media tracks on join #346

Merged
merged 1 commit into from
Mar 1, 2023

Conversation

streamer45
Copy link
Contributor

Summary

This was an interesting one given I could never reproduce the issue on my end, neither locally nor when connecting on remote instances.

The bug has been there for a long time as I remember first encountering this on a Community recording a few months ago. Something in the recorder setup made it more likely to reproduce. In fact, I was finally able to do this earlier today through a quick load-test on Community.

The problem was that we are registering the media track events handler(s) when mounting the component (both widget and recording view) but any delay in doing so could mean events firing before we get a chance to listen and tracks getting "lost", or better, never attached to the document, hence never played. To fix this, before listening on the track events we also check for existing available tracks and attach them accordingly.

Thanks to @hanzei for raising this issue. I was almost convinced it was a Chromium bug after the previous failed attempts at reproducing this but indeed, as often is the case, it was an issue on our end.

Ticket Link

https://mattermost.atlassian.net/browse/MM-50751

@streamer45 streamer45 added the 2: Dev Review Requires review by a core committer label Feb 22, 2023
@streamer45 streamer45 self-assigned this Feb 22, 2023
@streamer45 streamer45 added this to the v0.14.0 / MM v7.9 milestone Feb 22, 2023
@hanzei
Copy link
Contributor

hanzei commented Feb 23, 2023

Wow, I was not expecting a fix that soon. Thank you @streamer45 ! 🚀

Copy link
Member

@cpoile cpoile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@cpoile cpoile added 3: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Mar 1, 2023
@streamer45 streamer45 merged commit 1cb5d62 into main Mar 1, 2023
@streamer45 streamer45 deleted the MM-50751 branch March 1, 2023 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants