child_process: fire close event from stdio #22892

koh110 · 2018-09-17T07:49:45Z

Event of close does not fire when giving for stdio option that type is wrap.
For example, if we set event listener to net.server and giving child_process, that events never fire.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

addaleax · 2018-09-17T14:02:12Z

lib/internal/child_process.js

@@ -379,6 +379,9 @@ ChildProcess.prototype.spawn = function(options) {

      if (i > 0 && this.pid !== 0) {
        this._closesNeeded++;
+        if (stream._events && stream._events.close) {
+          stream.socket.addListener('close', stream._events.close);
+        }


I think this would lead to the event handler being called with a wrong this value, and I think this might not work if there is more than one listener (because stream._events.close would be an array then).

Could we use the listener in the lines below, and call stream.emit('close') from there?

I see. It might not work when events is an array.

I want to fix.
createSocket create new Socket object and sharing only handle of old socket.
Therefore, we can't call old close events.
How do i fix it best ?

https://github.com/nodejs/node/blob/5a3e445304f3920590b2ae1d1237fe7615572a44/lib/internal/child_process.js#L274
https://github.com/nodejs/node/blob/5a3e445304f3920590b2ae1d1237fe7615572a44/lib/internal/child_process.js#L377

Therefore, we can't call old close events.

I’m not sure I understand this – Can you explain more? i.e. what wouldn’t work if we add stream.emit('close'); in the callback below?

I was misunderstanding. It could use stream.emit('close'), if new stream object have old stream object instead of having _events.

I am going to fix.

if (stream._stdio) { stream._stdio.emit('close'); }

handle: handle, - _events: events + _stdio: stdio

koh110 · 2018-09-21T13:05:39Z

@addaleax Hi! I fixed this code. Can you review it ?

addaleax · 2018-09-21T17:31:37Z

lib/internal/child_process.js

@@ -379,6 +379,9 @@ ChildProcess.prototype.spawn = function(options) {

      if (i > 0 && this.pid !== 0) {
        this._closesNeeded++;
+        if (stream._stdio) {
+          stream._stdio.emit('close');
+        }


Shouldn’t this go in the stream.socket.on('close', () => { handler below?

@addaleax I could do it. I fixed and push this code. Please check it.

addaleax · 2018-09-22T10:07:00Z

CI: https://ci.nodejs.org/job/node-test-pull-request/17367/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1556/

addaleax

LGTM but we probably need to address one thing before this can finally be merged :)

addaleax · 2018-09-24T21:00:04Z

lib/internal/child_process.js

@@ -926,7 +929,8 @@ function _validateStdio(stdio, sync) {
      acc.push({
        type: 'wrap',
        wrapType: getHandleWrapType(handle),
-        handle: handle
+        handle: handle,
+        _stdio: stdio


So … the code above (https://github.com/nodejs/node/pull/22892/files#diff-f45eb699237c2e38dc9b49b588933c11R923) reads like stdio could be:

A native stream handle

An object with a handle property which is a native stream handle

An object with a _handle property which is a native stream handle

The path that is typically taken for e.g. net.Sockets would be the third one, I think. We should probably look into deprecating at least the first two, but … all that’s a different story and not really for this PR.

What matters is that stdio does not need to be any kind of particular object; most of the time, it’s a net.Socket or similar, but it could also just be { handle: someNativeHandle }. Even though that’s a bad idea imo, the code above points to this being a possibility. This means that the stream._stdio.emit() call above might fail, because emit() might not be available?

I guess a simple solution would be to turn the if (stream._stdio) { conditional into if (stream._stdio && typeof stream._stdio.emit === 'function') { ?

I see. I will fix other PR.

I came up with a way when object is pushed. How about this?
Should it be judged when emit()?

const a = { type: 'wrap', wrapType: getHandleWrapType(handle), handle: handle }; if (typeof stdio.emit === 'function') { a._stdio = stdio; } acc.push(a);

Or how about if (stream._stdio instanceof EventEmitter)?

@koh110 I’m not sure. instanceof EventEmitter is probably fine, too?

I would prefer for the _stdio property to be present consistently, though – either it’s always there or always missing.

@addaleax Yeah, you right. I completely agree. I was wrong.

I would prefer for the _stdio property to be present consistently,

I feel that if (stream._stdio && stream._stdio instanceof EventEmitter) { is more meaningful than if(stream._stdio && typeof stream._stdio.emit === 'function') {. EventEmitter may be changed to WritableStream. What do you think about it?

koh110 · 2018-09-30T03:25:26Z

@addaleax I fixed it. Check please.

koh110 · 2018-10-19T05:13:27Z

@nodejs/child_process It needs another approval. Please review it.

koh110 · 2018-11-12T05:08:22Z

Anyone else approve ?

Trott · 2018-11-14T05:12:09Z

@nodejs/child_process Can someone review this?

Trott · 2018-11-15T03:36:30Z

(Also: @addaleax Can you confirm this LGTY still?)

addaleax · 2018-11-24T07:40:26Z

CI: https://ci.nodejs.org/job/node-test-pull-request/18918/

Trott · 2018-11-25T02:46:49Z

Test fails on Windows in CI:

00:36:32 not ok 64 parallel/test-child-process-server-close
00:36:32   ---
00:36:32   duration_ms: 0.152
00:36:32   severity: fail
00:36:32   exitcode: 1
00:36:32   stack: |-
00:36:32     internal/child_process.js:372
00:36:32         throw errnoException(err, 'spawn');
00:36:32         ^
00:36:32     
00:36:32     Error: spawn ENOTSUP
00:36:32         at ChildProcess.spawn (internal/child_process.js:372:11)
00:36:32         at spawn (child_process.js:543:9)
00:36:32         at Server.net.createServer (c:\workspace\node-test-binary-windows\test\parallel\test-child-process-server-close.js:12:3)
00:36:32         at Server.emit (events.js:189:13)
00:36:32         at TCP.onconnection (net.js:1507:8)
00:36:32   ...

Trott

Test not passing on Windows...

koh110 · 2018-11-28T10:59:45Z

@Trott I fixed test. I think dir command might be too heavy on Windows.

Travis is failed, but it is only API rate limit. I passed this lint in local.

++node -p 'JSON.parse(process.argv[1])[0].url' '{
  "message": "API rate limit exceeded for 35.239.89.243. (But here'\''s the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)",
  "documentation_url": "https://developer.github.com/v3/#rate-limiting"
}'

Would you execute CI?

Trott · 2018-11-30T06:06:21Z

CI: https://ci.nodejs.org/job/node-test-pull-request/19076/

koh110 · 2019-01-07T11:50:57Z

@Trott I fixed fluky test. I think server would close before client.
Would you execute CI?

Trott · 2019-01-07T19:33:03Z

CI: https://ci.nodejs.org/job/node-test-pull-request/19972/

koh110 · 2019-01-08T11:15:20Z

@Trott I am sorry for taking up so much of your time. Please execute CI.

I found out common.mustCall should not be used in data event.

Sometimes this code returns v10.0.0 and \n.

spawn(process.execPath, ['-v'], {
  stdio: ['ignore', conn, 'ignore']
});

I removed common.mustCall from handler of data event. I pray for passing tests 🙏

gireeshpunathil · 2019-01-08T11:33:45Z

CI: https://ci.nodejs.org/job/node-test-pull-request/19990/

koh110 · 2019-01-16T03:54:55Z

some test not passing but i think that it unconnected this fix.
Would it be good to do anything?

free-bsd
it may be another test
https://ci.nodejs.org/job/node-test-commit-freebsd/23253/nodes=freebsd11-x64/console
arm-fanned
it may be another test
https://ci.nodejs.org/job/node-test-binary-arm/5348/RUN_SUBSET=1,label=pi1-docker/console
windows
it seems error of building
https://ci.nodejs.org/job/node-compile-windows/23609/label=win-vs2017-x86/console

Trott · 2019-01-17T05:17:27Z

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/20176/

koh110 · 2019-01-17T13:56:44Z

Why does it throw ENOTSUP on windows... 😢
it seems windows couldn't do node -v just vs2017.

koh110 · 2019-03-01T18:29:26Z

Windows accept only UV_TTY and UV_NAMED_PIPE...

node/deps/uv/src/win/process-stdio.c

Lines 408 to 418 in 0109e12

    
           if (stream->type == UV_TTY) { 
        
             stream_handle = ((uv_tty_t*) stream)->handle; 
        
             crt_flags = FOPEN | FDEV; 
        
           } else if (stream->type == UV_NAMED_PIPE && 
        
                      stream->flags & UV_HANDLE_CONNECTION) { 
        
             stream_handle = ((uv_pipe_t*) stream)->handle; 
        
             crt_flags = FOPEN | FPIPE; 
        
           } else { 
        
             stream_handle = INVALID_HANDLE_VALUE; 
        
             crt_flags = 0; 
        
           }

koh110 · 2019-03-01T18:34:08Z

Whould stream->type be checked before spawn?

Trott · 2019-03-01T19:02:18Z

Maybe someone on @nodejs/platform-windows can help out or at least give a nudge in the right direction?

bzoz · 2019-03-04T09:07:01Z

Passing sockets as stdio handles is not supported by libuv under Windows. The internet says it is possible, but that would require some nontrivial changes to libuv.

Maybe the test can use named pipes?

koh110 · 2019-03-07T15:29:41Z

Yes! Named pipes seems working!! Thank you so much! @Trott @bzoz
I will add document Passing sockets is not supported Windows with other PR. And I will challenge to commit libuv if I can.

This commit skipped over my code. So, i modified a few line. Could you review and CI again? @addaleax

addaleax · 2019-03-07T15:33:15Z

CI: https://ci.nodejs.org/job/node-test-pull-request/21302/

@Trott Can your request-changes be dismissed if CI is green?

Trott · 2019-03-07T16:13:44Z

@Trott Can your request-changes be dismissed if CI is green?

Yes.

CI is green

addaleax · 2019-03-07T16:42:18Z

Landed in 1133e0b, thanks for the PR! 🎉

PR-URL: #22892 Reviewed-By: Anna Henningsen <[email protected]>

PR-URL: nodejs#22892 Reviewed-By: Anna Henningsen <[email protected]>

PR-URL: #22892 Reviewed-By: Anna Henningsen <[email protected]>

PR-URL: #26604 Refs: #22892 (comment) Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Bartosz Sosnowski <[email protected]>

PR-URL: nodejs#26604 Refs: nodejs#22892 (comment) Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Bartosz Sosnowski <[email protected]>

PR-URL: #26604 Refs: #22892 (comment) Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Bartosz Sosnowski <[email protected]>

nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Sep 17, 2018

addaleax reviewed Sep 17, 2018

View reviewed changes

koh110 force-pushed the close_event branch from 5a3e445 to d06e548 Compare September 17, 2018 22:57

addaleax reviewed Sep 21, 2018

View reviewed changes

koh110 force-pushed the close_event branch from d06e548 to ba9c6f2 Compare September 22, 2018 04:15

addaleax approved these changes Sep 24, 2018

View reviewed changes

koh110 force-pushed the close_event branch from ba9c6f2 to 294fb6d Compare September 29, 2018 15:24

addaleax approved these changes Nov 24, 2018

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 24, 2018

Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 25, 2018

Trott previously requested changes Nov 25, 2018

View reviewed changes

koh110 force-pushed the close_event branch from 294fb6d to 03c7988 Compare November 28, 2018 10:22

koh110 force-pushed the close_event branch from 03c7988 to a2b250f Compare December 8, 2018 10:39

koh110 force-pushed the close_event branch from a2b250f to f8f1d10 Compare January 8, 2019 10:26

child_process: fire close event from stdio

6fe220a

koh110 force-pushed the close_event branch from f8f1d10 to 6fe220a Compare March 7, 2019 15:06

addaleax closed this Mar 7, 2019

addaleax pushed a commit that referenced this pull request Mar 7, 2019

child_process: fire close event from stdio

1133e0b

PR-URL: #22892 Reviewed-By: Anna Henningsen <[email protected]>

koh110 deleted the close_event branch March 12, 2019 10:27

koh110 mentioned this pull request Mar 12, 2019

doc: add Note of options.stdio into child_process #26604

Closed

4 tasks

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 12, 2019

child_process: fire close event from stdio

8e94cb2

PR-URL: nodejs#22892 Reviewed-By: Anna Henningsen <[email protected]>

BridgeAR mentioned this pull request Mar 13, 2019

2019-03-14, Version 11.12.0 (Current) #26644

Merged

BridgeAR pushed a commit that referenced this pull request Mar 14, 2019

child_process: fire close event from stdio

8c864de

PR-URL: #22892 Reviewed-By: Anna Henningsen <[email protected]>

child_process: fire close event from stdio #22892

child_process: fire close event from stdio #22892

Conversation

koh110 commented Sep 17, 2018 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koh110 commented Sep 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Sep 22, 2018

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koh110 Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koh110 commented Sep 30, 2018

koh110 commented Oct 19, 2018

koh110 commented Nov 12, 2018

Trott commented Nov 14, 2018

Trott commented Nov 15, 2018

addaleax commented Nov 24, 2018

Trott commented Nov 25, 2018

Trott left a comment • edited Loading

Choose a reason for hiding this comment

koh110 commented Nov 28, 2018 • edited Loading

Trott commented Nov 30, 2018

koh110 commented Jan 7, 2019

Trott commented Jan 7, 2019

koh110 commented Jan 8, 2019 • edited Loading

gireeshpunathil commented Jan 8, 2019

koh110 commented Jan 16, 2019

Trott commented Jan 17, 2019

koh110 commented Jan 17, 2019 • edited Loading

koh110 commented Mar 1, 2019

koh110 commented Mar 1, 2019

Trott commented Mar 1, 2019

bzoz commented Mar 4, 2019

koh110 commented Mar 7, 2019

addaleax commented Mar 7, 2019

Trott commented Mar 7, 2019

addaleax commented Mar 7, 2019

koh110 commented Sep 17, 2018 •

edited

Loading

koh110 Sep 25, 2018 •

edited

Loading

Trott left a comment •

edited

Loading

koh110 commented Nov 28, 2018 •

edited

Loading

koh110 commented Jan 8, 2019 •

edited

Loading

koh110 commented Jan 17, 2019 •

edited

Loading