node: process.on("beforeExit")#2331
Conversation
|
|
cjihrig
force-pushed
the
process_on_beforeexit
branch
from
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", |
|
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 |
|
|
||
| globalThis.addEventListener("beforeunload", (e) => { | ||
| super.emit("beforeExit", process.exitCode || 0); | ||
| if (core.eventLoopHasMoreWork()) { |
There was a problem hiding this comment.
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.
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.
It doesn't pass with Deno 1.23.4, but for me it's working with canary.
There was a problem hiding this comment.
Ah, right - because denoland/deno#14830 hasn't been released yet 🤦 . It's working for me with latest main.
cjihrig
force-pushed
the
process_on_beforeexit
branch
from
34c882b to
55a466e
Compare
bartlomieju
changed the title
5 participants
Blocked by denoland/deno#14830