Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

child_process: fire close event from stdio #22892

Closed
wants to merge 1 commit into from
Closed

child_process: fire close event from stdio #22892

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion lib/internal/child_process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,9 @@ ChildProcess.prototype.spawn = function(options) {
// The stream is already cloned and piped, thus close it.
if (stream.type === 'wrap') {
stream.handle.close();
if (stream._stdio && stream._stdio instanceof EventEmitter) {
stream._stdio.emit('close');
}
continue;
}

Expand Down Expand Up @@ -946,7 +949,8 @@ function _validateStdio(stdio, sync) {
acc.push({
type: 'wrap',
wrapType: getHandleWrapType(handle),
handle: handle
handle: handle,
_stdio: stdio
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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') { ?

Copy link
Contributor Author

@koh110 koh110 Sep 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

});
} else if (isArrayBufferView(stdio) || typeof stdio === 'string') {
if (!sync) {
Expand Down
20 changes: 20 additions & 0 deletions test/parallel/test-child-process-server-close.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
'use strict';

const common = require('../common');
const { spawn } = require('child_process');
const net = require('net');

const server = net.createServer((conn) => {
conn.on('close', common.mustCall());

spawn(process.execPath, ['-v'], {
stdio: ['ignore', conn, 'ignore']
}).on('close', common.mustCall());
}).listen(common.PIPE, () => {
const client = net.connect(common.PIPE, common.mustCall());
client.on('data', () => {
client.end(() => {
server.close();
});
});
});