Skip to content

Conversation

@mcollina
Copy link
Member

@mcollina mcollina commented Aug 5, 2021

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: #39447

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
@mcollina
Copy link
Member Author

mcollina commented Aug 5, 2021

This PR is tagged as draft because it missing tests and docs.

@nodejs-github-bot nodejs-github-bot added console Issues and PRs related to the console subsystem. needs-ci PRs that need a full CI run. labels Aug 5, 2021
Copy link
Member

@ronag ronag left a 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.

@ktfth
Copy link

ktfth commented Aug 6, 2021

I can help with tests and docs after reviews?

Copy link
Member

@addaleax addaleax left a 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.

@addaleax
Copy link
Member

addaleax commented Aug 6, 2021

Also, if you do want to change behavior, I’d suggest not destroying stdio streams when they are the target of a pipeline() call.

@mcollina
Copy link
Member Author

mcollina commented Aug 6, 2021

The problem that #39447 is facing is that they are closing stdout and then using console.log(), not seeing the output. What this PR does is to make console.log() always log, independently of what process.stdout state is.

(The actual mechanism that we use is really up for debase, _undestroy() is not nice).

@addaleax
Copy link
Member

addaleax commented Aug 6, 2021

I understand – but then they should not be closing stdout.

@mcollina
Copy link
Member Author

mcollina commented Aug 6, 2021

I agree. However most frontend developers sees console.log() as special vs process.stdout, this create a mismatch between Node.js and the frontend world.

@addaleax
Copy link
Member

addaleax commented Aug 6, 2021

@mcollina Then let’s maybe just make destroying the stdio streams a no-op? As in, process.stdout.destroy() lets people continue to write to it? I think that makes sense anyway.

@mcollina
Copy link
Member Author

mcollina commented Aug 6, 2021

@mcollina Then let’s maybe just make destroying the stdio streams a no-op? As in, process.stdout.destroy() lets people continue to write to it? I think that makes sense anyway.

Might be a good option. WDYT @ronag?

@ronag
Copy link
Member

ronag commented Aug 6, 2021

SGTM. We might have to modify finished to not wait for willEmitClose. Though I think it might already ignore it if emitClose: false.

@ronag
Copy link
Member

ronag commented Aug 6, 2021

What if console.log uses process._rawDebug and bypasses stdout?

@addaleax
Copy link
Member

addaleax commented Aug 6, 2021

What if console.log uses process._rawDebug and bypasses stdout?

I’d really like to keep orthogonality and avoid introducing special-casing for the built-in Console vs user-created Console instances.

(There’s also technical problems with this – process._rawDebug() also works on a best-effort basis, and may not always succeed when the OS buffer for the output stream is full.)

@mcollina
Copy link
Member Author

mcollina commented Aug 6, 2021

Here is the alternative implementation: #39685.

@mcollina mcollina closed this Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

console Issues and PRs related to the console subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

stream.pipeline does not invoke callback when error happens

5 participants