Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
cowboy opened this issue Nov 26, 2013 · 16 comments
Open

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

cowboy opened this issue Nov 26, 2013 · 16 comments
Labels

Comments

@cowboy
Copy link
Owner

cowboy commented Nov 26, 2013

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?

@vladikoff
Copy link

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"

@cowboy
Copy link
Owner Author

cowboy commented Nov 26, 2013

Rebased on top of 0.1.2 via e90e70e.

@cowboy
Copy link
Owner Author

cowboy commented Nov 26, 2013

In 0778a33 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

@alimfeld
Copy link

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

@cowboy
Copy link
Owner Author

cowboy commented Nov 26, 2013

I dunno.

@alimfeld
Copy link

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).

@cowboy
Copy link
Owner Author

cowboy commented Nov 27, 2013

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.

@vladikoff
Copy link

@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

@alimfeld
Copy link

@cowboy fair enough... thanks for clarifying 😄

@pifantastic
Copy link

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!

@sindresorhus
Copy link

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

@sindresorhus
Copy link

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.

@mgol
Copy link

mgol commented Jan 31, 2014

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

@mgol
Copy link

mgol commented Mar 1, 2014

@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

prust added a commit to CSNW/precommit-hook that referenced this issue Sep 9, 2014
On Windows, when running mocha after jshint, all mocha output (passes, failures, which tests failed, etc) is truncated due to this bug: cowboy/node-exit#7.
@nickpresta
Copy link

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?

@ArmorDarks
Copy link

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants