fix: use Node's microtasks policy in node_main.cc #23153
Conversation
| @@ -146,6 +146,10 @@ int NodeMain(int argc, char* argv[]) { | |||
| JavascriptEnvironment gin_env(loop); | |||
|
|
|||
| v8::Isolate* isolate = gin_env.isolate(); | |||
| // TODO(ckerr) and/or TODO(codebytere) use node::SetIsolateMiscHandlers() | |||
| node::IsolateSettings is; | |||
There was a problem hiding this comment.
for posterity's sake, can we note that this means v8::MicrotasksPolicy::kExplicit?
There was a problem hiding this comment.
My rationale for using node::IsolateSettings wasn't so much "show that we're using kExplicit" as it was "show that we're using whatever Node is using" -- e.g. if they were to change from kExplicit to something else in their code, we would pick up that change automatically.
If we want to make it obvious that the mode is kExplicit, maybe we should just use that directly instead of using IsolateSettings. Would that be your preference?
|
This seems like a potentially dangerous change since we aren't particularly careful to execute microtasks when we exit a C++ context. The V8 docs on MicrotasksPolicy say that I'm a little hazy as to what the implications are of not having good hygiene around executing microtasks when we ought to be. Could there be issues where e.g. we get an event in C++ (like a tray icon click, for instance), which we then run a JS callback for, but we forget to run microtasks and then the main thread goes back to sleep again without having executed microtasks? Could that result in weird things like promises or timers not executing when they're supposed to? |
This change only affects Electron when running in Node emulation mode -- for the tray icon example, there won't be any tray icon nor clicks in Node emulation mode. 🙂 I don't think there are any callbacks in Node mode that could have side-effects as you're describing -- is there something in particular you have in mind? |
|
@nornagon we don't run into this problem in non-emulation mode bc we already override it. @ckerr has a great writeup linked in PR :) |
|
@codebytere ah, I missed the bit in the writeup that mentions that we set this elsewhere during non-RUN_AS_NODE mode. That's exactly the thing I was concerned about though, which is that we're setting the |
|
To my knowledge we do via our microtasks runner - we have a specific task observer to flush the queued microtasks which runs at the end of every task and is based off of Blink's |
|
Release Notes Persisted
|
Fixes #21515. See that ticket for a looong writeup.
Description of Change
Node uses a
kExplicitmicrotasks policy; previously we were using the V8 default value ofkAuto. This PR ensures we use the same policy as Node by extracting it fromnode::IsolateSettings.CC @codebytere,@nornagon
Checklist
npm testpassesRelease Notes
Notes: Fixed Promise timeout issue when running Electron as Node.