child_process: watch child_process stdin pipe peer close event#62353
child_process: watch child_process stdin pipe peer close event#62353Tseian wants to merge 2 commits intonodejs:mainfrom
Conversation
nodejs-github-bot
added
c++
Tseian
changed the title
Tseian
force-pushed
the
main
branch
3 times, most recently
from
071fa18 to
4cedb07
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #62353 +/- ##
==========================================
+ Coverage 89.67% 89.68% +0.01%
==========================================
Files 676 676
Lines 206693 206772 +79
Branches 39576 39599 +23
==========================================
+ Hits 185346 185453 +107
+ Misses 13482 13445 -37
- Partials 7865 7874 +9
🚀 New features to boost your workflow:
|
Tseian
force-pushed
the
main
branch
2 times, most recently
from
0ae2360 to
09463c0
Compare
Tseian
changed the title
Tseian
force-pushed
the
main
branch
2 times, most recently
from
9e25a20 to
78acd67
Compare
Tseian
changed the title
Tseian
force-pushed
the
main
branch
3 times, most recently
from
e704d96 to
8cc7608
Compare
Tseian
changed the title
|
@nodejs-github-bot retest |
|
@jasnell @ronag @jbunton-atlassian Hi everyone, Could you please have a code review for this PR? |
| args.GetReturnValue().Set(err); | ||
| } | ||
|
|
||
| void PipeWrap::WatchPeerClose(const FunctionCallbackInfo<Value>& args) { |
There was a problem hiding this comment.
Rather than exposing two functions, can this expose a single function with a single bool argument? e.g. watchPeerClose(true, callback) / watchPeerClose(false)
There was a problem hiding this comment.
OK, Got it. I'll commit it soon.
| wrap->peer_close_watching_ = false; | ||
| wrap->peer_close_cb_.Reset(); | ||
| } | ||
| args.GetReturnValue().Set(err); |
There was a problem hiding this comment.
Is the err return value actually used on the JavaScript side? If it is, I'm not seeing where.
There was a problem hiding this comment.
Indeed, it's not currently used in the JS side. Do you mean that if it's not used in the JS side, we shouldn't return a value?
I saw that Bind and Connect both return values, and I assumed this was a convention.
src/pipe_wrap.cc
Outdated
| // Stop listening and release the JS callback to prevent memory leaks. | ||
| wrap->peer_close_watching_ = false; | ||
| wrap->peer_close_cb_.Reset(); | ||
| args.GetReturnValue().Set(uv_read_stop(wrap->stream())); |
There was a problem hiding this comment.
Likewise here, the return value does not appear to be used at all.
3 participants
Issue
#25131
Description
Watch pipe peer close(EOF/HUP) event, only support for unix. Once the event is detected, a JS callback is triggered to execute, which in turn triggers a method to destroy the socket. Eventually child_process.stdin.on('close') will be triggered.
Before
child_process.stdin.on('close') will not be triggered if the readable end of the pipe has been closed.
After
child_process.stdin.on('close') will be triggered if the readable end of the pipe has been closed.
Changes
Test