improve process.exit() workaround

[alexlamsl]

• use public API
• fix issue with Node.js 0.10 on WIndows

Related:
#1061
mochajs/mocha#333 (comment)

[kzc]

Can this fix only be applied to node 0.10 and keep the old way for other versions? By not calling setBlocking on stdout and stderr, output may not work correctly on node 6 and earlier on files larger than 64K on Linux. Node's flushing of stdout/stderr upon process.exit or uncaught exception is incredibly broken.

[alexlamsl]

This workaround is closer to the documented advice and the workaround I referenced above.

Note that uncaughtException isn't actually being used to wait on the flushing of stdout/stderr - it's just there to pair with the immediate throw:

If it is necessary to terminate the Node.js process due to an error condition, throwing an uncaught error and allowing the process to terminate accordingly is safer than calling process.exit().

It is then just a matter of waiting on the event loop for the two streams to be completely drained before calling process.exit().

[kzc]

I am familiar with that documented advice. Much of it is wrong. Due to a basic node flaw it won't flush stdout/stderr for uncaught exceptions for latest versions of node, nor for process.exit on older versions of node on Linux. If you google this issue you'll see me pulling my hair out on various node and libuv issues.

[kzc]

It is then just a matter of waiting on the event loop for the two streams to be completely drained before calling process.exit().

The event loop is no longer running for an uncaught exception and process.exit.

[alexlamsl]

The event loop is no longer running for an uncaught exception and process.exit.

process.exit() I agree. But if the event loop has stopped after uncaughtException, then bin/uglifyjs wouldn't give the correct exitCode in my workaround - which isn't the case otherwise a bunch of mocha tests would fail (& they did during experimentation).

[alexlamsl]

I understand that the actual uncaught exception would have its stderr print-out truncated (regardless of uncaughtException event handling).

I'm not trying to write anything out for that throw exit;, merely terminating whatever sequential order of execution we were in when (the replaced version of) process.exit() is invoked.

[kzc]

I'm not worried about the tests but for normal uglify use. In the event of exception or process.exit stdout/stderr may not be flushed on older versions of node. In recent versions of node setBlocking(true) is called on stdout and stderr for console applications.

Can the fix in this PR only be applied to the test runner and revert the old setBlocking behavior in the uglify library itself?

[alexlamsl]

Can you prove this PR actually breaks normal usage?

Some of the mocha tests are invoking bin/uglifyjs, so I don't see how it can tell if it is within a testing environment or not.

[kzc]

Can you prove this PR actually breaks normal usage?

Testing aside, the real question is whether this new way works for normal uglify CLI use in all situations with all node versions on all OSes.

I wasted enough of my life in the node and libuv projects trying to fix this very issue. I gave up:

libuv/libuv#876

Some of the mocha tests are invoking bin/uglifyjs, so I don't see how it can tell if it is within a testing environment or not.

I can appreciate that's a problem.

[alexlamsl]

I do notice & appreciate your heroic efforts in trying to get node to fix this properly:
nodejs/node#6297 (comment)

I'm planning to take some risk and see what breaks, and if so to come up with some mocha tests to cover this in the future.

You probably know this already, but usage of process.exit() is not limited to bin/uglifyjs - the underlying commander (& yargs back in uglify-js@2) also invokes that. So we have no choice but to override process.exit instead of just an internal wrapper function.

[kzc]

Unfortunately the commander setBlocking(true) fix is only run for bin/minify, not minify().

[alexlamsl]

Why are we concerned with stdio and stderr in API usage of uglify-js? 😮

[alexlamsl]

AFAICT commander doesn't do setBlocking(true) - they had a discussion but that's about it:
tj/commander.js#530

[kzc]

I stand corrected. I know yargs did.

[alexlamsl]

Specifically, you did? 👻

[kzc]

Why are we concerned with stdio and stderr in API usage of uglify-js?

Back when yargs was used uglifyjs -h was truncated on Linux in node 6.0.0.

To your point, perhaps for minify() it's not an issue.

Specifically, you did?

Both uglify and yargs independently implemented setBlocking at the time.

Let's go with your fix and see if anyone notices a problem.