process: add process.exitSoon()

[jasnell]

Checklist

• tests and code linting passes
• a test and/or benchmark is included
• documentation is changed or added
• the commit message follows commit guidelines

Affected core subsystem(s)

process

Description of change

Updated: adds process.exitSoon([code[, timeout]]) and 'exitingSoon' event to process. Allows exitingSoon handlers to defer exit until they are ready. Timer is optional.

When set, an internal timer will be set that will exit the process at the given timeout. In the meantime, the registered listeners for process.on('exitingSoon') will be invoked and passed a callback to be called when the handler is ready for the process to exit. The process will exit either when the internal timer fires or all the callbacks are called, whichever comes first.

This is an attempt to deal more intelligently with resource cleanup and async op completion on exit

Docs / test case included

Refs: #6456

[jasnell]

CI: https://ci.nodejs.org/job/node-test-pull-request/2434/

[eljefedelrodeodeljefe]

tiniest nit: it's probably historic here also, but can we avoid k-prefixed variable names, since const is common nowadays? See, here.

I like the idea of this though.

[addaleax]

I like the little k. It communicates more than just something not changing; It says “I’m an opaque constant, I’m an identifier for some thingy, and my exact value is irrelevant.”.

[eljefedelrodeodeljefe]

Alright. No strong feelings.

[geek]

Is this intended to allow code to do cleanup before an exit? If thats the case, it won't solve the problem as the timer can trigger before the cleanup is complete and the process will exit. If you set a really large timer then it may work, but you still must require that all ready functions are called by the event handlers, which the code isn't doing at this point.

[kzc]

-1

A timeout will not guarantee the complete flushing/draining of stdout/stderr.

If a timeout is specified then process.exit() will return and program execution will continue. That would be fine is this were a new function like process.cleanupAndExit, but not suitable for process.exit in my opinion.

[cjihrig]

I think I'm -1 on this as well. We've seen just in our own CI that timer-based solutions tend to introduce flaky behavior.

[kzc]

Discoverability of the functionality of process.exit is also a concern for applications that must work with all versions of node from 0.10.x through to node 6.x and beyond. How do you interrogate process.exit to see whether it has this timeout behavior to take advantage of it - Function.length?

Any change to process.exit must be a drop-in replacement for the historic process.exit that blocks until exit without returning to the event loop. That really restricts how process.exit can be altered.

[eljefedelrodeodeljefe]

Please don't limit every current contribution to process in light of the current problem we have. If the API makes sense for other purposes also we should add them. I am sure it's not intended to solely address the refed issue and we shouldn't minus one on every other feature that is not.

[jasnell]

Yep, there are definitely limitations with this approach. Btw, the intent here is not to say this is the way we should do this, but to give one concrete alternative possibility. (i.e. I'm not sure I'm +1 on this either ;-) ..)

[mscdex]

I think I am -1 on this approach as well for the reasons already given.

[mhdawson]

One approach would be shutdown hooks which are allowed to complete before the run-time shuts down. In this case when process.exit() was called, the shutdown hooks would be invoked and once they complete then the exit would complete. This has the advantage that you don't clobber the running of a hook with some arbitrary timeout and you can exit as soon as all the hooks complete. There still is the issue of what happens if a hook causes you to block forever, but that's potentially a different issue. It also means that for existing applications (where no hooks are registered) behavior is the same as today and you don't need to change any of the existing process.exit() calls.

I'd favor something based on that approach as opposed to using the timeout as described.

[jasnell]

We get that to an extent already with process.on('exit'), the shortcoming is that any async work gets lost. The reason I added the timeout timer in here is to ensure that the exitingSoon hooks don't run forever causing the exit to hang. We can take a slightly different approach than what this PR currently implements.

• Rather than extending process.exit() with a new param, we use something like process.exitSoon(). The signature would be process.exitSoon([code][, timeout]). If timeout is not provided, the exitingSoon callbacks are invoked but the internal exit timer is not created. The process will exit when all of the ready callbacks are invoked. This would have the effect of hanging the exit until the callbacks return.

If the timeout is provided, it acts as it does in this PR currently.... so, I could do something like:

function waitForStdioDrain(ready) {
  if (process.stdout.bufferSize > 0 || process.stderr.bufferSize > 0)
    setImmediate(() => waitForStdioDrain(ready));
  else
    ready();
}

process.on('exitingSoon', (ready) => {
  waitForStdioDrain(ready);
});

process.exitSoon(0);

[Fishrock123]

I can't see how adding more indeterminism to this (or anything else) can possibly help anything.

--1

[addaleax]

@jasnell I like that last proposal of yours, and preferably with running waitForStdioDrain() by default in some way? That would be possible, right?

[jasnell]

It certainly could, yes @addaleax

[mhdawson]

does this still apply to the updated proposal

" the shortcoming is that any async work gets lost."

except that they have the timeout period to complete ?

[jasnell]

@mhdawson ... only if you passed timeout to process.exitSoon(). If you called process.exitSoon(0) without setting the timeout, all of the hooks would have to call their ready callbacks before the process would actually exit.

[jasnell]

I will update this PR with the alternative and will keep iterating until we come to a solution that is workable.

[kzc]

process.exitSoon()

@jasnell +1 to new function name

One approach would be shutdown hooks which are allowed to complete before the run-time shuts down. In this case when process.exit() was called, the shutdown hooks would be invoked and once they complete then the exit would complete.

@mhdawson Is this shutdown hook a synchronous action without returning to the event loop?

Would it be possible to drain any node stream in such a shutdown hook?

[jasnell]

@kzc .. yes, event handlers are invoked sync and since you provide the handler, you'd be able to clean up anything you'd need as part of the flow, including any other streams you need to have drained.

[jasnell]

All, ok, I've pushed a refactored update that uses a separate process.exitSoon() method. The signature is process.exitSoon([code[, timeout]])...

• If timeout is not specified, the process will not exit until all of the exitingSoon handlers have called their ready() callbacks. The actual process.emit('exitingsoon') is done on nextTick() so the call does not block the current scope but the handlers themselves are invoke synchronously when it does emit.
• If timeout is specified, the internal force exit timer is created. A boolean is passed to the handlers so they know if there is a timer set or not.
• If there are no exitingSoon handlers registered, this simply falls through to process.exit() and exits immediately.
• The method returns true the first time it successfully schedules the exit, false thereafter, to protect against being called multiple times.

The test cases show examples.

Nothing specific to draining the stdio streams has been added to this yet but it could be done easily by adding a exitingSoon handler that watches the process.stdout.bufferSize and process.stderr.bufferSize and defers calling ready() until those are both 0. We can theoretically add our own exitingSoon handler that does this by default.

[kzc]

Is this PR adding an extra argument to process.exit?

[jasnell]

sorry no, in the refactored version this is changed. I just missed a doc edit... fixing now

[jasnell]

Fixed! :-)

[kzc]

Will sync shutdown hooks be part of this PR?

[jasnell]

You mean a default hook that ensures stdout/stderr are flushed? It's possible, they're not there yet as I'm trying to do this incrementally to make sure it's heading the right direction

[kzc]

I suppose the sync shutdown hook @mhdawson described would belong in another PR as it relates specifically to process.exit.

[jasnell]

@kzc ... how would that be different than the existing exit hooks?

[Fishrock123]

1. this is super easy to do in a user module
2. needs to be deterministic otherwise it should be considered harmful
3. unnecessary feature creep either way

My suggestion would be to approach this very differently:

Make process.exit() (or rather, void Exit()):

• uv_stop() (I think) the event loop
  • or whatever to stop anything new from happening but while keeping the threads alive to do writes
• attempt to flush any data
• exit

At the same time, I don't think we should alter process.abort().

Edit: it is possible that this is out of scope for process.exit(), but even if we add something new (which should be the fallback), it should have that behavior.

[Fishrock123]

(I would suggest opening a new PR for that, if we go that way. But we should not add this.)

[benjamingr]

I'm -1 on those changes at all, anything basic is as easily implementable at user-level. Anything non-basic is too specific for the application and task at hand and can't be generally handled from core anyway.

That said, I definitely like the fact we're having discussion about resource cleanup. I think it's an area where core should be involved in more - though possibly only through adding more and better documentation about the topic.

[jasnell]

We're not likely to get consensus on changes here following this approach. Closing.

[kzc]

@jasnell Thanks for exploring this design. It was a useful exercise. Without the event loop what actions you can perform in a shutdown hook are very limited. This is a deceptively difficult problem. I don't know how to solve it other than the way node-exit did it.

[benjamingr]

@kzc this is not a deceptively difficult problem - proper resource cleanup is a very difficult problem and is widely recognized as such :)

[jasnell]

Yep, my intent here as far as iterating on the design was to start with a design that was the least intrusive to the Node.js internals, purposefully starting with an approach that could be implementable in userland then iterate back from there. In my opinion, we should only have to touch the node internal native code if we can't find a pure javascript solution, and I see very little value in changing how process.exit() currently works if there is a pure js solution that can be sorted out -- regardless of whether it actually lives in core or not. However, I received some strong criticism privately outside of this thread that implied that the process I was following here was somehow inappropriate so I've decided to just drop it. I encourage others to continue exploring what the other options here are. I strongly believe that Node.js needs to be doing much more in terms of proper resource cleanup and flushing stdio on exit is a simple no brainer for me. I consider it a bug that we don't and I hope someone is able to find a workable solution.

[jasnell]

oh.. and while folks are looking at this, there's a somewhat related problem that I've been stewing over lately: #831 ... basically, if stdout is a pipe and the other end closes, stdout.write dies and throws because we have no way of detecting that the other end is closed. I started playing around with some ideas around a pure js solution (following my usual iterative approach) but I think a native solution is going to be the best path forward.