A better way to detect a process is exiting

[bcoe]

Some Background

Part of the motivation of building NODE_V8_COVERAGE into core, was that it made it significantly easier to capture exit behavior.

Prior to this, tools like nyc used error-prone hacks like, signal-exit to detect process termination.

Proposal

It would be nice to have an API hook that fires when Node.js is about to terminate, regardless of whether it's:

1. shutting down normally.
2. had an exception thrown.
3. been sent a signal.

related conversations: nodejs/node#29480, nodejs/node#28960

Just wanted to kick off this conversation (@sindresorhus actually brought it up initially), CC: @coreyfarrell, @isaacs.

[coreyfarrell]

I know this is a can of worms but the ability to have an async exit handler would be fantastic. Currently it is only possible to run synchronous functions in the exit handler (IE signal-exit). For nyc this means that large amounts of code is forced to be synchronous, and this ripples out to everything directly and indirectly called from the signal-exit handler.

I understand that allowing the event loop to resume could allow excessive deferment of the exit but this is already the case, IE child_process.spawnSync can spawn a long running process from the exit handler. So on balance I think it would be better to eliminate situations where sync code must be used. To mitigate this I think that calling process.exit() or any uncaught exception after exit handling is initiated should cause immediate exit.

[sindresorhus]

This is sorely needed.

There are many use-cases that requires running code right before exit:

• Test cleanup
• Restoring hidden cursor
• Gracefully shutting down a cluster or external services
• Failure logging
• Preserving in-memory data in case of failure
• Asynchronously rollback changes on failure

I built exit-hook for this purpose, but it is unfortunately not able to catch all the ways a Node.js process can exit, and it also doesn't support async. Someone built an async port, async-exit-hook, but it's very buggy and not maintained anymore. I have also used signal-exit in the past, but it also has problems.

[sam-github]

A little more specific info about what is wanted would be helpful here, but I'll take a guess at one thing, which is allowing async code like the below:

process.once('exit', () => {
  asyncCall(() => {
      anotherAsyncCall(() => {
      }
  })
})
process.exit()

process.exit() will force node to exit, even if there is ongoing asyncness that would normally keep the loop/node running. If the exit event handler adds more asyncness to the loop, and then wants node to exit when the "just added" asyncness completes, and to ignore the "already present" asyncness that was there... that's a bit challenging. Tracking which things are registered with the loop, and who registered them, involves the long-stack problem (so that nested async calls work), as well as somehow tracking pools of activity at the uv level.

Note that in the case of node doing a "normal" exit because the loop is empty, this already possible. There is https://nodejs.org/api/process.html#process_event_beforeexit, and it does allow async code to be scheduled on exit, and after that when the loop is empty again, node will attempt to exit again (after emitting beforeExit again, which might wake it up... etc.).

[ehmicky]

An additional use case for it would be to clean up CLI spinners or progress bars.

[coreyfarrell]

If the exit event handler adds more asyncness to the loop, and then wants node to exit when the "just added" asyncness completes, and to ignore the "already present" asyncness that was there... that's a bit challenging.

For nyc or a test runner to use async exit handlers I think it would be expected that pending asyncness initialized before process.exit() would be dropped. I think this could be accomplished by node.js replacing the event loop with a new empty one. This alone could cause an issue if already running but unresolved async handlers needed to resolve for the exit handler to function. For example in source-map@>=0.7.0 the SourceMapConsumer returns a promise which depends on a shared promise which loads WASM. If an application initialized this load then ran process.exit() before the WASM load completed then I assume the new event loop would fail to ever resolve new SourceMapConsumer. I'm not sure the best solution to that, nyc could protect itself by having the exit handler purge require.cache of everything from the source-map then load a fresh copy. Maybe even purge the entire require.cache.

A more generic solution might be if we could start a worker_thread and synchronously wait for it to finish. From what I can tell this would eliminate the risk of exit handlers waiting on a promise that will never resolve and would allow the exit handler to use async code. No idea what it would take to implement Worker#waitSync. Also unsure how much overhead it would introduce to create a worker_thread with a potentially large workerData object. Just wanted to bring this up as this could be a way for the main thread running the exit handler to remain synchronous.

[chancancode]

Hello! Thank you for looking into this. I agree with the needs and I think I mostly said what I wanted to say in this issue.

Reading it now, I think the originally issue unnecessarily coupled it with async functions/promise cancellation, etc, which is not really needed to illustrate the core issue.

From my perspective, the single handler APIs are only viable if you are writing an application. It could be nicer, and handle more edge cases, but if you have full control about the desired behavior on the final "app" (executable/script), you can probably make it work enough.

However, if you are a library/middleware/plugin designed to be "pulled in" to an app, there is nothing that really works IMO. Some challenges are:

• If the "host" wants to handle (say) SIGINT, you should respect that
• You want to play nice with other library/middleware/plugin that also wants to do cleanup and let everyone do their thing before exiting

IMO, these two are the core unsolvable constraints with the current APIs. I don't think it is a fundamental limitation to how the event loop works, how signals are delivered etc, but someone need to be the "coordinator" here, because again, you can actually build this behavior if you control everything end-to-end. I just think that should be the Node runtime (or the standard library at least).

[coreyfarrell]

I've found a way to eliminate use of the source-map module from the call path of nyc's signal-exit callback so I can live without the ability to run async code inside exit handlers. I still think this would be a useful follow-up but having node.js support sync exit handlers will be much easier to implement and get merged.

[sam-github]

Good idea to leave async out of it for now.

I don't want to edit the subject without permission, but @bcoe I suggest you change "exiting" to "terminating". Processes terminate on Unix because they exited, or because they were signalled, and the exit case is already handled well enough (I think), its a handler for the signal paths that are being asked for (also, I think).

I'm not as familiar with Windows, but there are no signals, so that distinction doesn't exist, but IIRC there is a ProcessTerminate() (name?) API that is somewhat equivalent to kill(2) except I think the caller decides what the exit status will be, and I think that not all causes of termination are trappable - the API might even allow the caller to decide? This is from bad memory, investigation required. libuv does some work to make it look to casual child_process users like windows does have signals, but cracks in the emulation are observable.

If the requirements could enumerate either in words or js code the conditions that should be trapped and forwarded to a termination handler, that would be handy. For example, I can read https://github.com/sindresorhus/exit-hook/blob/master/index.js#L18, but since @sindresorhus said it does not meet requirements, its not quite the requirements! :-)

Note that https://github.com/sindresorhus/exit-hook/blob/b504031173d8e2c2ce6bb9f0b6557cbe6dedecd5/index.js#L18 should be process.kill(process.pid, signal) to preserve the termination status.

And just to set the bar for tooling that would be using this... it is literally impossible on Unix, from within a process, to hook all causes of process termination, SIGKILL in particular is impossible to trap, and SIGSEGV is unwise to (if node even allows it), so if tooling layers need to be robust to all cases of termination, they need to be aware of this. But probably coverage checkers don't need to account for processes that were SIGKILLed or that seg faulted!

[boneskull]

If the requirements could enumerate either in words or js code the conditions that should be trapped and forwarded to a termination handler, that would be handy

^ next steps

[coreyfarrell]

If a signal which results in termination can safely be hooked then I want it to execute my terminate hook. signal-exit does capture a larger list of signals (https://github.com/tapjs/signal-exit/blob/master/signals.js) compared to exit-hook, these do not include the signals which are impossible to hook or documented by node.js as unsafe to hook. I would not object to removal of SIGABRT from the list of reasons.

From signal-exit README:

When you want to fire an event no matter how a process exits:

• reaching the end of execution.
• explicitly having process.exit(code) called.
• having process.kill(pid, sig) called.
• receiving a fatal signal from outside the process

I would add DEP0018 unhandled promise rejections to this list of reasons once that results in process termination with a normal error, but probably not when --abort-on-uncaught-exception is enabled.

For nyc one key detail of signal-exit is the alwaysLast option. Maybe nyc would just have to document that the callback of process.on('terminate', ...) cannot be covered but instead process.prependListener('terminate', ...) would need to be used if they wanted the coverage for callback function. In any case if the terminate hook were accomplished through a normal event emitter API I think it would result in bugs against nyc.

An alternative would be to support a two-pass execution of terminate events, essentially make the following work:

process.on('terminate', () => {
  process.on('terminate', () => {
    // run after all terminate handlers that were added before exit
  });
});

Instead of process.emit('terminate') the event could be emitted with

function terminateHookPass() {
  const hooks = process.listeners('terminate');
  process.removeAllListeners('terminate');
  for (const hook in hooks) {
    hook.apply(process)
  }
}

let dispatchTerminateDone = false;
function dispatchTerminateHooks() {
  if (dispatchTerminateDone) {
    return;
  }

  // Ignore hooks added during the second pass even
  // if dispatchTerminateHooks is called again.
  // Not sure this will actually be needed
  dispatchTerminateDone = true;
  // Execute hooks added during operation
  terminateHookPass();
  // Execute hooks added during the first pass (`alwaysLast` hooks)
  terminateHookPass();
  process.removeAllListeners('terminate');
}

An alternative would be a process.createTerminationHook({alwaysLast: true, callback() {}}).enable() API similar to what is provided for async_hooks. nyc would likely still see bugs on code which uses alwaysLast: true but this would be much less likely than with an EventEmitter run by a single pass process.emit('terminate').

I have not yet given thought to what information should be passed to the terminate event if any. nyc doesn't need to know the why the terminate is occurring but other tools might?