console,process: refactoring stdout/stderr error handling - #6174
Conversation
Sorry, something went wrong.
On an error in stdout/stderr stream, redirect the error to a new `'stdio-error'` event and set the stdout/stderr to a null stream. Additional refactoring the stdio.js while in the code.
| Console.prototype.log = function() { | ||
| this._stdout.write(util.format.apply(null, arguments) + '\n'); | ||
| Console.prototype.log = function(...args) { | ||
| this._stdout.write(`${util.format(...args)}\n`); |
There was a problem hiding this comment.
Isn't ...args still slightly slower than .apply?
Not that it would probably be measurable given log.
Sorry, something went wrong.
There was a problem hiding this comment.
FWIW v8 5.x supposedly has improved this significantly (according to the v8 blog), but we will have to test and see how it compares there.
Also, the other day I happened to notice that backtick strings are much slower to create than typical concatenation. I'm not sure what, if any, impact it has on GC though.
Sorry, something went wrong.
There was a problem hiding this comment.
Yep, as indicated, much of this change is speculative. The code cleanups
make a few assumptions about what v8 v5 should be doing as far as
optimizations but those assumptions have yet to be tested. Those specific
changes can be pulled back out of this if necessary tho.
Sorry, something went wrong.
There was a problem hiding this comment.
I've separated the more general modifications to console out to a separate PR #6233
Sorry, something went wrong.
|
Closing. Made several iterations on this basic approach and not happy with it. Will be investigating other options. |
Sorry, something went wrong.
Checklist
Affected core subsystem(s)
console, process
Description of change
This is a largely speculative refactoring of error handling around process.stdout, process.stderr, and console in an attempt to address the long standing EPIPE error issues (see #831 and https://github.com/cjihrig/node-1/blob/10bc673123f29692b7dae5e306a155ed0210e413/test/known_issues/test-process-external-stdio-close.js and #947).
Within the stdio setup in internal/process/stdio.js, an error handler is set on the default stdout and stderr streams. If any error occurs, the stdout or stderr is replaced with a null stream (equivalent to /dev/null) and the error is forwarded to a new 'stdio-error' event on process which is passed the err and the fd.
The reason for this is straightforward: when process.stdout and process.stderr are Pipes, the EPIPE error is the only way we can detect if the other end of pipe has been closed (see the test-process-external-stdio-close.js test case for an existing example of this). This means that calling console.log() in a child process, for instance, can be a bit problematic (per issue #831).
The effect of this change is that calls to console.log() won't throw or result in a process.on('error') if the stdout pipe happens to be shut down on the other end. The output simply goes to the null stream.
This is a work in progress currently and is intended for discussion and review.
It also includes some refactoring to simplify and modernize some of the code bits. Those are mostly isolated in their own commits.
/cc @vkurchatkin @bnoordhuis @trevnorris