stdio: call _undestroy() inside _destroy for stdout and stderr #39685
Conversation
|
Yeah, I think this is preferable 👍 Might be good to have a few tests though |
|
yep, I plan to add them as soon as we have cleared the approach. |
|
would this be semver-patch or semver-major? |
|
I guess that depends on whether we think people intentionally close stdout/stderr to avoid output – I don’t think it’s a big concern, and I’d say semver-patch is fine, but I also fully understand if we want to be a bit more careful. |
mcollina
added
the
baking-for-lts
mcollina
force-pushed
the
undestroy_stdout_stderr
branch
from
e9ee76b
to
fd98597
Compare
|
Test added |
| if (process.argv[2] === 'child') { | ||
| process.stdout.destroy() | ||
| console.log('out') | ||
| console.error('err') |
There was a problem hiding this comment.
We should also test that
- This applies when
process.stderr.destroy()is also called (otherwise theconsole.errortest is not meaningful) - Explicitly using
process.stdout.write()works - Explicitly using
process.stderr.write()works - Writing after a short timeout works (since
destroy()can, generally speaking, be asynchronous)
|
Not blocking but the whole |
|
You know why on coverage-linux go out of memory? And this process need docs? I'm following this changes to help more on future contributions using this example |
|
|
@addaleax I have filled in the missing tests. |
mcollina
force-pushed
the
undestroy_stdout_stderr
branch
from
50205c8
to
7d82a18
Compare
This change makes `process.stdout` and `process.stderr` to be automatically undestroyed when ended/destrouyed, therefore making it always possible to write/console.log to stdout. Fixes: nodejs#39447
mcollina
force-pushed
the
undestroy_stdout_stderr
branch
from
7d82a18
to
f13ad43
Compare
There was a problem hiding this comment.
Not knowing enough about streams, what it does to the bootstrap and the tests LGTM.
I have a question though, would it be in conflict with 7b2ca4c (part of the WIP #38905) which tries to run the original destroy for the streams during beforeExit because when building the snapshots the fds should be closed? (I am guessing not, because only one of the dummyDestroy and the original destroy would be called?)
|
@joyeecheung in #38905 you might need to call |
This change makes `process.stdout` and `process.stderr` to be automatically undestroyed when ended/destrouyed, therefore making it always possible to write/console.log to stdout. Fixes: #39447 PR-URL: #39685 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
|
Landed in 33f9b7f |
This change makes `process.stdout` and `process.stderr` to be automatically undestroyed when ended/destrouyed, therefore making it always possible to write/console.log to stdout. Fixes: #39447 PR-URL: #39685 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
targos
removed
the
baking-for-lts
Fixes: #39447