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

fix: uv loop polling when render process reuse enabled #25869

Merged
merged 2 commits into from Oct 13, 2020

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Oct 10, 2020

Description of Change

Closes #22119.
Closes #25314.
Closes #23910.
Closes #25405.
Closes #25496.

Fixes an issue where some Node.js module API calls hung in the renderer process on reload when renderer process reuse was in effect. This was happening because when render process reuse was enabled, Chromium reused the same process, but the main thread was still bound to the old iocp (on windows, and respective interface for other platforms) because libuv didn't destroy it. The queue then went on to operate on this old iocp - when we reloaded, a new iocp was created but not bound to the main thread and so we would not properly be informed of events on it. This is fixed by preparing the message loop on every reload when reuse is enabled with node_bindings_->PrepareMessageLoop().

cc @zcbenz @deepak1556

Checklist

Release Notes

Notes: Fixed an issue where some Node.js module API calls hung in the renderer process after reloads when render process reuse was enabled.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Oct 10, 2020
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Oct 11, 2020
@zcbenz
Copy link
Member

zcbenz commented Oct 12, 2020

Busy looping is not a solution for listening to events. Apart from constant high CPU usage, when consuming streams it would add the delay for each chunk, making IO very slow.

@codebytere
Copy link
Member Author

codebytere commented Oct 12, 2020

@zcbenz that does make sense and i don't think this is the ideal solution either - it's not clear to me what specifically is lacking in the previous approach though to make it hang in the way it was doing. Do you have any thoughts on an alternative?

@zcbenz
Copy link
Member

zcbenz commented Oct 12, 2020

I have not looked into the issue, but it seems that the issue is caused by GetQueuedCompletionStatus(uv_loop_->iocp) no longer working after reloading. So I think there are few possibilities:

  1. Node has created a new uv loop or iocp after reloading while we are still using the old one. (Unlikely though since other platforms are still working.)
  2. Node does some initialization work to the iocp, after reloading the iocp was initialized again and stopped working.
  3. After reloading Electron does some cleanup work and breaks iocp.

@codebytere
Copy link
Member Author

@zcbenz determined the true underlying cause - updated the solution & PR body accordingly

@nornagon
Copy link
Member

Can we get a test?

@codebytere codebytere changed the title fix: uv loop polling on Windows fix: uv loop polling when render process reuse enabled Oct 12, 2020
Copy link
Member

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

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

Test seems to be failing on mac CI:

Error: done() called multiple times
    at IpcMainImpl.<anonymous> (electron/spec-main/api-browser-window-spec.ts:4301:11)
    at Object.<anonymous> (electron/js2c/browser_init.js:157:8980)

spec-main/api-browser-window-spec.ts Show resolved Hide resolved
@codebytere codebytere removed the request for review from a team October 13, 2020 03:26
@trop
Copy link
Contributor

trop bot commented Oct 13, 2020

I was unable to backport this PR to "9-x-y" cleanly;
you will need to perform this backport manually.

@trop
Copy link
Contributor

trop bot commented Oct 13, 2020

@codebytere has manually backported this PR to "11-x-y", please check out #25922

@trop
Copy link
Contributor

trop bot commented Oct 13, 2020

@codebytere has manually backported this PR to "10-x-y", please check out #25923

@trop
Copy link
Contributor

trop bot commented Oct 13, 2020

@codebytere has manually backported this PR to "9-x-y", please check out #25924

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment