-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: swallow stdout/stderr error events #9470
Conversation
This is a step in making `console` safe, swallows EPIPE and other errors by registering an empty 'error' event listener. Also fixes writes to stdout/stderr not working after `end()` is called, even though we already override `destroy()` so the stream is never actually closed. Refs: nodejs#831 Fixes: nodejs#947 Fixes: nodejs#9403
// get() { | ||
// return true; | ||
// } | ||
// }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nodejs/streams this affected some other tests but I wasn't able to tell why.
=== release test-http-chunk-problem ===
Path: parallel/test-http-chunk-problem
assert.js:85
throw new assert.AssertionError({
^
AssertionError: '8c206a1a87599f532ce68675536f0b1546900d7a' == 'bafcbe972b6e69cd415ded38cb995f1bca983929'
at cp.exec (/Users/Jeremiah/Documents/node/test/parallel/test-http-chunk-problem.js:55:20)
at ChildProcess.exithandler (child_process.js:202:7)
at emitTwo (events.js:106:13)
at ChildProcess.emit (events.js:191:7)
at maybeClose (internal/child_process.js:877:16)
at Socket.<anonymous> (internal/child_process.js:334:11)
at emitOne (events.js:96:13)
at Socket.emit (events.js:188:7)
at Pipe._handle.close [as _onclose] (net.js:501:12)
&
=== release test-stdin-pipe-large ===
Path: parallel/test-stdin-pipe-large
assert.js:85
throw new assert.AssertionError({
^
AssertionError: 131072 === 1048576
at Socket.child.stdout.on.common.mustCall (/Users/Jeremiah/Documents/node/test/parallel/test-stdin-pipe-large.js:22:10)
at Socket.<anonymous> (/Users/Jeremiah/Documents/node/test/common.js:421:15)
at emitNone (events.js:91:20)
at Socket.emit (events.js:185:7)
at endReadableNT (_stream_readable.js:974:12)
at _combinedTickCallback (internal/process/next_tick.js:74:11)
at process._tickCallback (internal/process/next_tick.js:98:9)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(The current patch works, but I think maybe having destroyed = true
by default always would work better?) 9I don't think we can re-set it after destroy()
and not hit what it is needed to skip.
er = er || new Error('process.stdout cannot be closed.'); | ||
stdout.emit('error', er); | ||
}; | ||
if (stdout.isTTY) { | ||
process.on('SIGWINCH', () => stdout._refreshSize()); | ||
} | ||
stdout.on('error', () => {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure this is a good idea. If my program writes stdout to a file and that fails, it should definitely not pass silently – if this is about making console
safe, maybe something more specific to console
would be a better idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, perhaps. I'd like to have a bit more discussion, I don't know if throwing from stdio on process is a good idea either. There are a lot of libraries that do raw writes that could be affected by e.g. EPIPE
from an outside source, and that doesn't really make sense to crash on IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced that we should do this in the first place. I'm with @addaleax.
Even if we land, a comment on why that handler is there is 100% needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another -1. Hiding failure != safety.
I'm generally 👎 on haltering how the stream internals work for specific cases, as they make code brittle, and can cause failures in unrelated areas of the codebase, as you discovered. The problem of this fix is that after https://github.com/nodejs/node/blob/master/lib/internal/process/stdio.js#L12 is emitted, that stream should not be used again. If we want to achieve this, we need to "refresh" the I agree that Side note, maybe we should all sit together at the next collaborator summit and discuss some goals on stdout/stderr, as the current situation is very complex. |
While I don't disagree with this, I'm not really sure there are other reliable ways of doing that that catch all cases. (That is, things that need to not print newlines need to use
I'm going to disagree here, I feel like overriding this specifically says "tell the user the operation you just did cannot be done but do not actually close (i.e. move on)". Do you have a different interpretation of that? Alternatively, would it work to not fix the second "bug" then and always emit these errors, but still not throw them? (I could live with that I think, but I still think it is rather confusing.)
I unsure how much overhead re-allocating would be, maybe I could try it.
I think guaranteeing safety there will cause much confusion that stdio itself is not. I don't like that inconsistency one bit.
Agreed, will you be at the Collab summit in Austin? |
I think this is the lesser of two evils so I'm generally 👍 on this. |
I'd started down this path a while back and opted not to pursue it for the same concerns that have been expressed already. I believe that @bnoordhuis had suggested at some point having stdout/stderr automatically reopen to |
@@ -8,26 +8,64 @@ function setupStdio() { | |||
function getStdout() { | |||
if (stdout) return stdout; | |||
stdout = createWritableStdioStream(1); | |||
|
|||
Object.defineProperty(stdout._writableState, 'ended', { | |||
set() {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
value: true
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mutually exclusive with getters and still overwritable.
@jasnell are you talking about the state manipulation or general errors from stdio? |
I'm referring most specifically to intercepting the errors from stdio. I'm +1 on making |
I find it interesting that in my head this difference seems completely consistent – stdio are streams and should behave more or less like all other I/O streams (it’s weird enough that you can’t |
Another option would be to add a That differs more from browsers, but we aren't guaranteeing any consistency there anyways. |
@Fishrock123 that might be a very good idea. Even |
I think that would probably defeat the purpose. |
Maybe I did not understand: can you make an example on how you would use |
If we make only console safe, we need a way to write via the console that does no formatting and does not append a newline. That is, make alternatives to using |
Ok, I'm on board. |
I'm having trouble understanding the goal of this from the commit message and PR conversation.
How is ignoring errors "safe"? If I write some code that writes to stdout with console.log, are you taking the position its OK if that output never gets written to stdout, and my code never knows that it didn't?
Why do we do that? Shouldn't end mean... "end"? |
I much refer going the Also, I'm not particularly fond of the idea, but it may be worthwhile considering a flag that would allow users to opt-out of console safety in the off chance that this kind of change breaks existing code that is dependent on failures. |
Gawd no, not more flags. No fan of adding non-standard console methods either. The people pushing for these changes should present a convincing argument why the currents fails-when-kaput behavior is so bad that it needs changing.
That's a different kind of safe, it's to block an attacker from redirecting stdout or stderr to a socket that they control. |
lol.. as I said, I'm not particularly a fan of the idea. |
Our console is already non-standard and we haven't worked to make it standard, so... shrug.
It fails user expectations pretty badly. See also #831 |
Can you enumerate the expectations? Is the only user expectations here the one in #831? Its hard to read the minds of users, but it looks like that specific user thinks of node's That's fair, that is one user groups "expectation". But node doesn't have a dev tools console, and node's So basically, this user expects to use There are many more complaints of people exiting before the event loop drains, and reporting that their output did not actually hit screen or disk. Those people expect console to be reliable, and this "safe" option is going the other way. I'm thoroughly -1 on this based on #481, but if there are other "user expectations" you can link to, I'd like to see them. |
fwiw, there are cases where console.log will actually |
@jasnell justification for what, this PRs approach of discarding all errors? Or justification for making sure errors are emitted, so the user can decide to ignore them if they want, or know that they failed to write to stdout if they want that? I think its justification for the latter, probably obvious from my last comment. |
Justification for making console writes safe for whatever definition of safe you want. |
Everyone wants safe, its self evidently good! :-) And so not such a useful word... some think silently discarding user output is safe, some think its unsafe. Would be nice to have a better word, but I'm out of ideas ATM. |
The expectation is that |
@Fishrock123 who's expectation? Please respond to #9470 (comment) You are in danger of just repeating the same claim that users expect console.log to silently discard data, over and over. Back it up. I am a user, too, and have fair number of code bases where we very carefully let the event loop drain so that data from console.log is guaranteed not to be truncated by process.exit, it is certainly not our expectation that console.log silently discard data. |
I believe the idea is that not all (or even perhaps most) Node.js users are aware that carefully allowing the event loop to drain in order to capture all of the console.log output is something they're supposed to do. |
@jasnell That is tangential: I'm saying we go through some trouble already to make sure we get all output from console.log, so sabotaging our efforts by making console.log unsafe (silently discarding output) is not meeting our expectations. The issue tracker is full of people complaining about calling console.log but not getting the output, for various reasons. There is one issue where someone mistakenly thought console.log functions like it does in a web browser, an understandable mistake, but a mistake non-the-less. I suggest that pretty much everyone who comes to node from any other env other than browser-side js thinks (rightly, up to this PR) that console.log is the equivalent of ruby/C/C++/python/java/etc/etc's output routines, not a browser debug facility. This PR will conflict with their expectations. |
At the very least it is my expectation, and those issues imply it is other also. It's quite ridiculous that piping into e.g. |
(Spoilers: I use Node too.) |
I think I could write similar code in pretty much any language except the browser, which does not even have stdout or pipes, and has a radically different So, what you call "facepalm" is what pretty much every other programming language does. @Fishrock123 I know its your expectation, and I respect that, and even understand why you might have that expectation, and certainly understand why a browser developer would have that expectation. |
That may be the case for Python, Ruby and Node because they freely use exceptions as part of their standard library, so it doesn’t really apply to “pretty much every other programming”. In C or C++, you could just not check for errors and be fine with that.
That browser developers have an easy path into the back-end side is one of the main reasons for Node’s success IMHO.
I would expect |
we should respect the fact that people accustomed to IMHO this difference is not solvable. They should be different, because the runtime env is completely different. I'm ok in a
I agree with you, but I want to retain the possibility for |
@mcollina Yeah, totally agreeing, I think the |
A C prog would get SIGPIPEd, and would terminate with a status indicating that, unless it ignored SIGPIPE, the equivalent of choosing to do C++ iostreams I haven't used much, don't know how errors are signalled, but a c++ prog would also get SIGPIPEd unless it arranged to ignore it. AFAICT, with this PR, the following node program would never exit: function write() {
console.log("hello");
setImmediate(write)
}
write(); when called like Even if desireable, I don't think there is a way for console.log to ignore only errors related to the buffers it wrote to the process.stdout stream, without causing errors for all buffers written to the stream to be ignored. That doesn't seem to fit the stream model. |
What would you think of something like addaleax@78beb44? It’s kind of an ad-hoc solution but it does work. |
@addaleax I'm very 👎 on anything that touches streams to fix a specific issue somewhere else. |
@mcollina Yeah, don’t disagree. (But I wonder whether this is actually a deficiency in the streams API that should maybe be addressed there.) Anyway, addaleax/node@299f647 seems like a better way, and one that I would feel comfortable PRing. (Edit: #9744) |
@addaleax Streams are a rats nest I don't feel qualified to comment on, but your code does seem to work to my non-expert eyes. Also, I don't agree that console should behave like it does in the browser :-), I think it should behave like it does in languages that have stdout! If I could change history, I would not have console be named 'console' at all, I'd have it be named something that makes it clear that it is not a browser debug API, and let npm user-land provide a whatwg/ECMA/whoever console API if they want/need one for code that can run in node or browsers. But I can't change the past, and changing the name of console would break the world, so better to document, and move on, IMO. |
Fixes: nodejs#831 Fixes: nodejs#947 Ref: nodejs#9470
@Fishrock123 I’ve just landed #9744 – seems okay to close this? |
ping @Fishrock123 |
Closing given that #9744 landed |
Checklist
make -j8 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected core subsystem(s)
process, console
Description of change
This is a step in making
console
safe, swallows EPIPE and othererrors by registering an empty 'error' event listener.
Also fixes writes to stdout/stderr not working after
end()
is called,even though we already override
destroy()
so the stream is neveractually closed.
Refs: #831
Fixes: #947
Fixes: #9403
Mentioning some people from issue #831 "
console
should be safe": @vkurchatkin, @benjamingr, @nodejs/streams(This patch was made live during https://www.twitch.tv/nodesource/v/98991703 if you'd like to see me working on this in retrospect. :P)