Avoid output truncation in node 5.12.1-6.2.0

[dasilvacontin]

As reported in #2471, the help command's output was being cut off.
Streams were probably being cut off in other cases too.

Solution borrowed from mishoo/UglifyJS#1061.

Closes #2471.

[dasilvacontin]

I'm worried that making streams blocking might be a noticeable performance hit in large test suites. I'll have to research.

[dasilvacontin]

Confirmed it happens in other cases:

Error: Suite "MyClass MyClass2 MyClass3" was defined but no callback was supplied. Supply a callback or explicitly skip the suite.
    at Object.create (/Users/dasilvacontin/github/mochajs/mocha/lib/interfaces/common.js:117:17)
    at context.describe.context.context (/Users/dasilvacontin/github/mochajs/mocha/lib/interfaces/bdd.js:44:27)
    at Object.<anonymous> (/Users/dasilvacontin/temp/silly-tests.js:10:1)
    at Module._compile (module.js:541:32)
    at Object.Module._extensions..js (module.js:550:10)
    at Module.load (module.js:458:32)
    at tryModuleLoad (module.js:417:12)
    at Function.Module._load (module.js:409:3)
    at Module.require (module.js:468:17)
    at require (internal/module.js:20:19)
    at /Users/dasilvacontin/github/mochajs/mocha/li% 

[boneskull]

@dasilvacontin What if we enabled this upon the process.exit event?

[dasilvacontin]

That was my initial intention, not sure why I backed off. Will try and see.

On 1 Nov 2016, at 06:50, Christopher Hiller [email protected] wrote:

@dasilvacontin What if we enabled this upon the process.exit event?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[ScottFreeCode]

I believe, when I read through the vast web of linked issues surrounding the Node problem, that someone found that it doesn't work unless the streams are made blocking before the output happens rather than merely before the exit happens. I haven't tried to confirm this, however. (I believe someone also found that throwing an exception to kill the process can cause the same truncation issue. I wish I'd kept the links to these two particular comments, the web of linked issues was... horrifyingly large, partly because the problem goes back years to before version 0.10!)

[boneskull]

Why has it only surfaced relatively recently?

[ScottFreeCode]

Supposedly the Node 6 change that made the issue manifest more is a change in chunk size: nodejs/node#6456 (comment)

The relevant bit here about needing to block before output rather than merely before exit: nodejs/node#6456 (comment)

Other interesting claims...

• the underlying problem has something to do with non-blocking writes and exit: stdio buffered writes (chunked) issues & process.exit() truncation nodejs/node#6456 (comment) Asynchronous write() bug nodejs/node#2049
• different OSes have different circumstances for its manifestation: length of buffer, whether piped
• the current issue exists in all current >1.x versions of Node: stdio buffered writes (chunked) issues & process.exit() truncation nodejs/node#6456 (comment) stdio buffered writes (chunked) issues & process.exit() truncation nodejs/node#6456 (comment)
• similar, potentially related issues go back to at least 0.10: Chunked stdout/stderr drops writes if terminated early. nodejs/node#784 Stdout output is not completely flushed upon process exit. nodejs/node-v0.x-archive#8329 (comment) Stdout output is not completely flushed upon process exit. nodejs/node-v0.x-archive#8329 (comment) (and maybe older than that, depending on whether any of the issues linked from those, from 2011/2012, are really the same problem)
• it also happens for process killed by exception (so you can't throw as an alternative to process.exit(1)): Missing or truncated error message nodejs/node#6867
• there are now GitHub issues just to track other GitHub issues about this: Tracking issue: stdio problems nodejs/node#6980

And, given the number of fixes that have been proposed/attempted over the years (which can be found in issues and PRs linked from the above) only to stagnate because of competition with other possible fixes, disagreement as to whether waiting to flush when exiting is "wrong" (despite the fact that it's in Posix and every other mainstream language and the Node docs historically and the expectation of Node users), and disagreement as to whether it can be fixed in userland... I'm not expecting Node to ever actually fix this. 😢

So, unless blocking IO is acceptable for the whole program, I'm starting to suspect that the only other reliable way to work around the issue looks something like:

function Shutdown(exitCode) {
  this.exitCode = exitCode
}

try {

  // all application code here

  // where you would normally have used process.exit(#) (or perhaps, if Node allows it, monkey-patch that to do this):
  throw new Shutdown(#)

} catch (error) {
  if (!(error instanceof Shutdown)) {
    console.error(error) // or however is best to replicate how Node would have printed it if it were uncaught
  }
  process.exitCode = (error.exitCode != undefined ? error.exitCode : 1)
}

[eriktrom]

@dasilvacontin - https://github.com/yargs/set-blocking - same as u have, but with isTTY check added, dont skip that check :)

[kunagpal]

It might also be useful to ensure the current node version falls within 5.12.1-6.2.0 via a process.version check.

[dasilvacontin]

@kunagpal did you mention the range 5.12.1-6.2.0 because I did so initially, or? I can't recall the "why" behind the range, and @ScottFreeCode mentions the issue being present since 0.10.

[kunagpal]

@dasilvacontin The former, since it appears that the issue affects the specified range.

[ScottFreeCode]

Even setting aside the question of what range of Node versions may be affected, I would presume that it is not useful to behave differently based on version ranges unless there's a specific need for it that can't be satisfied by some other more meaningful check. Compare with browser sniffing vs. feature sniffing for example.

I'd rather spend time checking whether the proposed workaround actually fixes the issue, whether any workaround that does fix the issue has any negative performance impact, that the workaround doesn't depend on features that aren't supported in the earlier Node versions we support, etc.

[stale]

I am a bot that watches issues for inactivity.
This issue hasn't had any recent activity, and I'm labeling it stale. In 14 days, if there are no further comments or activity, I will close this issue.
Thanks for contributing to Mocha!

[boneskull]

@dasilvacontin Care to revisit?