-
-
Notifications
You must be signed in to change notification settings - Fork 34.2k
console: _undestroy() the stream if it is stdio #39670
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
Conversation
This change matches our bootstrap code that never closes stdout or stdeer. As the file descriptors are never closed, calling `process.stdout.destroy()` should not prevent `console.log()` for working. Fixes: nodejs#39447
|
This PR is tagged as draft because it missing tests and docs. |
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 don't understand what this does nor why it's needed.
I'm a against _undestroy in general. The only reason we have it is because of Socket.connect which I believe we have consensus is a bad idea and would deprecate if we could. It's exact behavior is unclear to me, e.g. what happens if the stream is "destroyed" but haven't finished destroying before it is undestroyed. That seems like a data race.
|
I can help with tests and docs after reviews? |
addaleax
left a comment
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.
What @ronag said is 💯.
If you destroy process.stdout, you remove the ability to write to process.stdout – that’s literally what you asked for.
|
Also, if you do want to change behavior, I’d suggest not destroying stdio streams when they are the target of a |
|
The problem that #39447 is facing is that they are closing stdout and then using (The actual mechanism that we use is really up for debase, |
|
I understand – but then they should not be closing stdout. |
|
I agree. However most frontend developers sees |
|
@mcollina Then let’s maybe just make destroying the stdio streams a no-op? As in, |
|
SGTM. We might have to modify |
|
What if console.log uses |
I’d really like to keep orthogonality and avoid introducing special-casing for the built-in (There’s also technical problems with this – |
|
Here is the alternative implementation: #39685. |
This change matches our bootstrap code that never closes stdout or
stdeer. As the file descriptors are never closed, calling
process.stdout.destroy()should not preventconsole.log()forworking.
Fixes: #39447