Emit keepalive event

[sam-github]

This was partially spurred by unit tests silently exiting. The code under test was buggy, it failed to schedule any work, but the result was the test runner bailing with exit 0, and no clear indication in the output what had happened. This event would allow timer-less detection of the exit condition.

Possible names for this event were:

• dead, my favorite, its emitted when the event loop is no longer alive, and is short
• preexit, also good, but might suggest that people use the event for actions that they want "before exit", when the exit event is the right choice
• keepalive, what's in the code now, indicates the purpose, keeping node alive after the event loop empties

I'm open to suggestions if this is considered useful.

I didn't know how to structure a node pull request that will fail to build without a libuv upgrade, so I applied joyent/libuv#813 to deps/uv and used that as a base.

[Nodejs-Jenkins]

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

The following commiters were not found in the CLA:

• Sam Roberts

Please see CONTRIBUTING.md for more information

[trevnorris]

Don't need the extra v8:: here.

[trevnorris]

I do have some reservations about this. It's fairly trivial to prevent the process from ever exiting:

process.on('keepalive', function() {
  console.log('about to die');
});

Why couldn't you listen for process.on('exit') and check the ref counter there?

[tjfontaine]

The option to keep the event loop alive is specifically what this is meant to provide, whereas you're not supposed to do anything async in process.on('exit') you could in this event, and then once that is done the event will re-fire so you can decide if you want to do it again, ad nauseam.

We talked about this briefly today, it was suggested that the name could be process.on('beforeExit') which somewhat correlates to window.onbeforeunload as process.on('exit') correlates to window.onunload

[trevnorris]

ah yeah. duh.

[tjfontaine]

My only real follow up has to do with the interaction of process.exit() if/when someone calls that explicitly this event won't fire, and there won't be a chance to keep the loop alive.

That seems like the right behavior, but it should probably be documented and perhaps tested for.

It may be that for consistency that this event should accept an argument indicating this is being fired in a forceful shutdown.

Just food for thought.

[trevnorris]

for these, going to use beforeExit from the conversation earlier.

here are a few cases that might throw devs off that may need to be documented:

// using `on` instead of `once` for things like `log` that
// devs may not expect
process.on('beforeExit', function() {
  console.log('never ending loop');
});

// beforeExit won't fire for either
process.once('beforeExit', function() { });
process.exit();
// or
throw Error('hello');

Also, i'm not sure that I'd agree that process.nextTick should cause beforeExit to fire again. e.g.

process.on('beforeExit', function() {
  process.nextTick(function() { /* never ending loop */ });
});

The reason this happens is because nextTick calls _needTickCallback. I dunno, but logically since I know that nextTick callbacks run within the same uv_run event, and if I don't do anything async in the callback, then it should be the last thing that runs.

[sam-github]

Thank you both for the feedback.

@tjfontaine I can add test for process.exit() not giving a chance to keep node alive. I think that is the predictable and desirable behaviour.

@trevnorris It wouldn't have occurred to me that process.nextTick() would behave any differently from setImmediate(), or net.connect(), so I'm glad it doesn't. Its an async call, it got scheduled, so it keeps node alive.

I can mention more explicitly that calling any asynchronous function will cause node to continue instead of exiting, and use console.log as an example of a function that is sometimes async.

Btw, I'm fine with beforeExit, but I'm not the one who is going to have to close the bug reports claiming it "doesn't fire before exit" (process.exit(), throw, etc.). That's why I suggested keepAlive, dead, quiet, loopQuiet, loopEmpty, or something like that, they seem less misleading. Its also why I tried to highlight in the docs that it is not a replacement for the exit event.

I'll rewrite this to beforeExit tomorrow if there are no other comments, and add some more tests.

[trevnorris]

Btw, I'm fine with beforeExit, but I'm not the one who is going to have to close the bug reports claiming it "doesn't fire before exit" (process.exit(), throw, etc.)

good point. let's get some more feedback there.

/cc @isaacs

[isaacs]

Discussed with @tjfontaine and @trevnorris in IRC. Here are some decisions we've come to:

1. process.exit() and throw should not trigger a beforeExit event. That event should only be used when it's actually possible to cancel the graceful exit by scheduling more work. Since this is not possible in the case of process.exit() or an unhandled throw, it should not emit beforeExit. (This needs to be documented a lot, of course.)

2. The interaction with process.nextTick should be fixed in 0.12, when we remove the TickSpinner. It's an issue in 0.10, because nextTick sometimes falls back to setImmediate-like behavior past a certain depth, so the TickSpinner is required.

3. The interaction with console.log() is a bug, I think. (Could be triggering a nextTick in the stream.Writable code for process.stdout, I wonder?) The logic should be:

   No more work to do!
   process.emit('beforeExit')
   if still no more work to do, process.exit()
   

   Since process.stdout.write() is synchronous, I'd expect that there ought to not be a writeReq hanging around afterwards for any reason.

[bnoordhuis]

Since process.stdout.write() is synchronous, I'd expect that there ought to not be a writeReq hanging around afterwards for any reason.

Well... process.stdout is async when it's a pipe.

[trevnorris]

@isaacs yes, the reason for console.log to cause the infinite loop is from the following in lib/_stream_writable.js:

function onwrite(stream, er) {
  // ...
    if (sync) {
      process.nextTick(function() {
        afterWrite(stream, state, finished, cb);
      });
    } else {
      afterWrite(stream, state, finished, cb);
    }

@sam-github because of the coming change to process.nextTick you should change the test to instead use setImmediate.

[trevnorris]

Well... process.stdout is async when it's a pipe.

@bnoordhuis would that be changed by #5595?

[bnoordhuis]

On Windows maybe, on Unices we want to keep the current behavior.

[sam-github]

I renamed, resynced code with process.emit('exit'), tested process.exit() doesn't allow restart, tested 3 variants of work (immediates, timers, server sockets), rewrote the docs, and explicitly don't depend on nextTick keeping the loop alive.

I think that addresses all the comments, except mine ;-)

Btw, I'm fine with beforeExit, but I'm not the one who is going to have to close the bug reports
claiming it "doesn't fire before exit" (process.exit(), throw, etc.). That's why I suggested keepAlive,
dead, quiet, loopQuiet, loopEmpty, or something like that, they seem less misleading.

[bnoordhuis]

Closing, no longer applies. Superseded by #6316.