-
Notifications
You must be signed in to change notification settings - Fork 15k
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
refactor: rewire the desktop capturer API to remove race conditions #18029
Conversation
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.
the js stuff seems janky as heck to me but i don't have any better suggestions so /shrug
requesting changes for minor nits in the c++
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.
Agree with the changes proposed by @nornagon , LGTM otherwise.
We now create a new instance of atom::api::DesktopCapturer for every request instead of weirdly re-using the same instance and queuing requests. This means there is now a 1:1 relationship between request and DesktopCapturer so there isn't a race condition between the observer for one request calling back before the observer of another. This is an issue ever since the backing APIs moved to worker threads. This also does a few things to ensure memory management * Only ever listen to one event per-request, after that we wipe the emit function to ignore all future events * Ensures we clean up the window_capturer_, screen_capturer_ and captured_sources_ in native land once the request is over. This _in theory_ fixes a flake we've been seeing on CI where we try to resolve the promise for a request that no longerr exists.
4ba8bc0
to
6f437c0
Compare
@nornagon Yeah the JS side of this is Not Too Great ™️ but this was the shortest refactor that fixed the issues. I'm planning on following up at some point with a proper rewrite with promises in native land and such 🤔 But that's a bigger task |
Release Notes Persisted
|
I was unable to backport this PR to "5-0-x" cleanly; |
I have automatically backported this PR to "6-0-x", please check out #18042 |
…lectron#18029) We now create a new instance of atom::api::DesktopCapturer for every request instead of weirdly re-using the same instance and queuing requests. This means there is now a 1:1 relationship between request and DesktopCapturer so there isn't a race condition between the observer for one request calling back before the observer of another. This is an issue ever since the backing APIs moved to worker threads. This also does a few things to ensure memory management * Only ever listen to one event per-request, after that we wipe the emit function to ignore all future events * Ensures we clean up the window_capturer_, screen_capturer_ and captured_sources_ in native land once the request is over. This _in theory_ fixes a flake we've been seeing on CI where we try to resolve the promise for a request that no longerr exists.
We now create a new instance of atom::api::DesktopCapturer for every request instead of weirdly re-using the same instance and queuing requests. This means there is now a 1:1 relationship between request and DesktopCapturer so there isn't a race condition between the observer for one request calling back before the observer of another. This is an issue ever since the backing APIs moved to worker threads.
This also does a few things to ensure memory management
window_capturer_
,screen_capturer_
andcaptured_sources_
in native land once the request is over.This in theory fixes a flake we've been seeing on CI where we try to resolve the promise for a request that no longr exists.
Notes: Fixed race condition in the
desktopCapturer
module where some requests for sources would never be resolved or unhandled exceptions would be thrown in the main process.