node: process.on("beforeExit")

[bartlomieju]

Blocked by denoland/deno#14830

[CLAassistant]

All committers have signed the CLA.

[kt3k]

great!

[andreubotella]

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 beforeExit event listener, they will run, but they won't cause another beforeExit event to be fired.

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 beforeunload event must make sure to also run the next ticks callback before firing the unload event.

[bartlomieju]

@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?

[cjihrig]

That sounds like a worthwhile plan to me.

[andreubotella]

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 beforeunload event – the queue has to be drained before determining whether to continue running.

I opened nodejs/node#43787 to test this behavior.

Since the event loop's pending status determines whether the beforeunload is canceled, for beforeExit, I think the next ticks queue should be drained from the beforeunload event listener before checking the pending status. This would mean there's no need to drain the queue for regular beforeunload events (as I suggested above), but I think we still should do that, for consistency between beforeunload and beforeExit.

[andreubotella]

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.

[cjihrig]

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.

[andreubotella]

It doesn't pass with Deno 1.23.4, but for me it's working with canary.

[cjihrig]

Ah, right - because denoland/deno#14830 hasn't been released yet 🤦 . It's working for me with latest main.

[bartlomieju]

Thanks for figuring it out @andreubotella!