child_process: check for closed pipes after creating socket#38716
child_process: check for closed pipes after creating socket#38716seangoedecke wants to merge 1 commit intonodejs:mainfrom
Conversation
After the socket is created, we trigger an empty read to make sure we catch stdio events like closing the stdin file descriptor. Otherwise those events will go undetected until the next stream write. Fixes: nodejs#25131
github-actions
bot
added
child_process
|
Hey @seangoedecke, sorry your PR was not reviewed for so long! Any chance you would be able to rebase on top of |
| stream.handle : null, i > 0); | ||
|
|
||
| // Trigger an empty read to check for a closed pipe | ||
| // or the close event will go undetected until next write |
There was a problem hiding this comment.
| // or the close event will go undetected until next write | |
| // or the close event will go undetected until next write. |
| const fs = require('fs'); | ||
|
|
||
| if (process.argv[2] === 'child') { | ||
| fs.closeSync(0); |
There was a problem hiding this comment.
Any reason to use closeSync(0) rather than process.stdin.end()? If so, can you add a comment?
There was a problem hiding this comment.
as far as I understand the closeSync(0) is used because it needs to immediately close the standard input stream synchronously, however it's really good question why we cannot asynchronous handling of the end of the input stream?
|
I build a debug out node from this PR to test the repoduced example, it does not resolve the problem. So can this PR be closed? |
|
This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open. |
4 participants
After the socket is created, this PR now triggers an empty read to make sure we catch stdio events like closing the stdin file descriptor. Otherwise those events will go undetected until the next stream write.
Fixes: #25131