node-exit icon indicating copy to clipboard operation
node-exit copied to clipboard

process.on("exit", fn) not being respected

Open cowboy opened this issue 11 years ago • 16 comments

See sindresorhus/time-grunt#15.

Given the following code:

console.log("before");
process.on("exit", function() {
  console.log("on exit");
});
process.exit();
console.log("after");
  • In non-Windows Node.js, this properly logs before then on exit.
  • In Windows Node.js, this might log nothing, all three things, or some combination of them. Kinda random, depends on the shell, if output is piped or not, etc.
  • With require("exit")(); instead of process.exit(); only before is logged. on exit should be logged.

Take a look at this commit in the node-exit branch.

Granted, that code is currently failing... but if we can fix it, we might be able to allow @sindresorhus's plugin to continue working.

Possible solution: Set stream.write to a NOP function after the exit event has been called?

cowboy avatar Nov 26 '13 19:11 cowboy

Specific issue we found, running this with output redirection:

console.log("before");
process.on("exit",function(){console.log("on exit");});
process.exit();
console.log("after");

PowerShell ignores "on exit"

vladikoff avatar Nov 26 '13 19:11 vladikoff

Rebased on top of 0.1.2 via e90e70e21bacb35c970c5cd92debdaece794eebc.

cowboy avatar Nov 26 '13 20:11 cowboy

In 0778a3303881eabbd439f2099cf143ea14fbcb7d I've overridden process.emit, which should allow synchronous stdout / stderr writes inside process.on("exit", fn) to work, while still preventing "after" from being written in the original example.

Thoughts?

/cc @sindresorhus

cowboy avatar Nov 26 '13 20:11 cowboy

@cowboy I'm confused... maybe you can shed some light on this:

As I understand it, using require("exit")() instead of process.exit() changes the exit semantics. The first allows following statements to be executed (until actual process.exit() is called), while the latter doesn't.

Consider the following case:

if (shouldExit) {
  require("exit")(errorCode);
}
// assume we can continue
doContinue();

If I'm not mistaken, doContinue may be called in this case but it would never be called when using process.exit(). Shutting down stream writes seems random to me, as it only tries to keep the exit semantics for stream writes but not for all other code that may be executed. I'm sure I miss something here...

alimfeld avatar Nov 26 '13 20:11 alimfeld

I dunno.

cowboy avatar Nov 26 '13 21:11 cowboy

Opinions? (Sorry to keep bothering...)

Why is it a good idea to prevent further writing to streams in node-exit?

(I feel it causes more harm than good as it breaks process exit handlers writing to streams and somehow tries to conceal the changed exit semantics of node-exit).

alimfeld avatar Nov 27 '13 07:11 alimfeld

I'm operating under the assumption that once process.exit() is called, nothing after that except things in process.on('exit', fn) handlers should run.

Does that seem fair?

I was addressing the part of that assumption that deals with output stdout and stderr logging, since that's how this issue manifests itself in my code. I'm not sure how to handle anything other than that.

cowboy avatar Nov 27 '13 14:11 cowboy

@alimfeld the goal is to eliminate inconsistencies, I believe if streams are allowed then you get different outputs between UNIX and Win32. So currently the plan is try and find a way to make node-exit capture the process exit

vladikoff avatar Nov 27 '13 19:11 vladikoff

@cowboy fair enough... thanks for clarifying :smile:

alimfeld avatar Nov 28 '13 20:11 alimfeld

0778a33 works for me!

We use time-grunt as well as a custom plugin that formats things for teamcity (using a method similar to time-grunt. Would love to see this land!

pifantastic avatar Dec 09 '13 23:12 pifantastic

Any new ideas on this? It's been a while.

sindresorhus avatar Dec 13 '13 07:12 sindresorhus

The testcase is flawed. It works fine on OS X since process.exit() is called by it.

module.exports = function (grunt) {
    console.log("before");
    process.on("exit", function() {
      console.log("on exit");
    });
    process.exit();
    console.log("after");
};

but don't if you leave it up to grunt to call process.exit(). Like:

module.exports = function (grunt) {
    console.log("before");
    process.on("exit", function() {
      console.log("on exit");
    });
    console.log("after");
};

on exit is not logged.

sindresorhus avatar Dec 13 '13 11:12 sindresorhus

Any chance of merging the fix? time-grunt doesn't work at all without it.

mgol avatar Jan 31 '14 20:01 mgol

@sindresorhus Can you provide a full test case? The following:

console.log("before");
process.on("exit", function() {
    console.log("on exit");
});
console.log("after");

properly logs:

before
after
on exit

on OS X 10.9.2.

EDIT: Nvm, I wasn't using the node-exit module

mgol avatar Mar 01 '14 15:03 mgol

Any news on this status of this bug? Should I still be downgrading to Grunt 0.4.1? Is there a branch that works that can be used in the meantime?

nickpresta avatar Dec 10 '14 20:12 nickpresta

Oh my, and I was wondering why

process.on('exit', () => {
    console.log("on exit")
})

inside my task isn't doing anything. So, Grunt v1.0.0 is affected by this, right?

ArmorDarks avatar Mar 29 '18 10:03 ArmorDarks