-
Notifications
You must be signed in to change notification settings - Fork 632
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
node: process.on("beforeExit") #2331
Conversation
f991dc1
to
3b66278
Compare
"test-process-beforeexit.js", | ||
"test-process-binding-internalbinding-allowlist.js", | ||
"test-process-env-allowed-flags.js", | ||
"test-process-exit-from-before-exit.js", |
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.
great!
I have looked a bit into this, and from what I can tell, in Node next ticks aren't enough to keep the event loop alive. If there are any ticks enqueued in the We could fix this by removing the presence of next ticks from the conditions that keep an event loop alive, but then the various places in the CLI that fire a |
@andreubotella i think that's preferable behavior. We did remove next tick checks but then had trouble with second to last callback. I think your solution is gonna alleviate that. @cjihrig what do you think? |
That sounds like a worthwhile plan to me. |
It looks like my description of the issue above was essentially correct, but the fix I proposed wasn't. Next ticks by themselves can't keep the event loop alive, but async ops in their callbacks can. Therefore, it's not enough to drain the next ticks queue once after firing the I opened nodejs/node#43787 to test this behavior. Since the event loop's pending status determines whether the |
@@ -258,6 +262,13 @@ class Process extends EventEmitter { | |||
constructor() { | |||
super(); | |||
|
|||
globalThis.addEventListener("beforeunload", (e) => { | |||
super.emit("beforeExit", process.exitCode || 0); | |||
if (core.eventLoopHasMoreWork()) { |
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 event listener should call into processTicksAndRejections
from next_tick.ts before checking if there's more work. That would be enough to fix this issue, with no need to change Deno's event loop.
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.
Do you mean like this:
globalThis.addEventListener("beforeunload", (e) => {
super.emit("beforeExit", process.exitCode || 0);
processTicksAndRejections();
if (core.eventLoopHasMoreWork()) {
e.preventDefault();
}
});
If so, that doesn't seem to get deno test -A --unstable node/_tools/test.ts -- parallel/test-process-beforeexit.js
passing.
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.
It doesn't pass with Deno 1.23.4, but for me it's working with canary.
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.
Ah, right - because denoland/deno#14830 hasn't been released yet 🤦 . It's working for me with latest main.
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.
Thanks for figuring it out @andreubotella!
34c882b
to
55a466e
Compare
Blocked by denoland/deno#14830