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.

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

child_process: fix stdio sockets creation #18701

Closed

Conversation

santigimeno
Copy link
Member

readable and writable properties can be passed directly to the
net.Socket constructor. This change also avoids an unnecessary call
to read(0) on the stdin socket. This behavior was disclosed when
trying to merge libuv@1.19.0 and specifically this commit:
libuv/libuv@fd04939.

Refs: libuv/libuv#1655

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

child_process

`readable` and `writable` properties can be passed directly to the
`net.Socket` constructor. This change also avoids an unnecessary call
to `read(0)` on the `stdin` socket. This behavior was disclosed when
trying to merge `libuv@1.19.0` and specifically this commit:
libuv/libuv@fd04939.

Refs: libuv/libuv#1655
@nodejs-github-bot nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Feb 10, 2018
@BridgeAR
Copy link
Member

@santigimeno please always trigger a CI after creating a PR :-)

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

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 10, 2018
@santigimeno
Copy link
Member Author

@BridgeAR yeah, I had done so but forgot to post it here: https://ci.nodejs.org/job/node-test-pull-request/13083/. Anyway, thanks for the heads up.

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM.

By any chance did you test with that libuv commit? It would be nice to be able to re-land that.

@santigimeno
Copy link
Member Author

By any chance did you test with that libuv commit? It would be nice to be able to re-land that.

Yeah. Before re-landing that commit we would need adding the changes @vtjnash proposed here and here (or something similar) so UV_WRITABLE_PIPE and UV_READABLE_PIPE are indeed handled on the libuv side.

vtjnash added a commit to vtjnash/libuv that referenced this pull request Feb 13, 2018
Windows is better about already doing this, so this will make the
behavior of these flags more consistent across platforms.
It also is just better to set these flags to reflect the actual
mode of the stream, rather than guessing at it based on typical usage.

Refs: libuv#1655
Refs: nodejs/node#18701
@BridgeAR
Copy link
Member

BridgeAR commented Feb 16, 2018

Landed in b74a6da 🎉

@BridgeAR BridgeAR closed this Feb 16, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 16, 2018
`readable` and `writable` properties can be passed directly to the
`net.Socket` constructor. This change also avoids an unnecessary call
to `read(0)` on the `stdin` socket. This behavior was disclosed when
trying to merge `libuv@1.19.0` and specifically this commit:
libuv/libuv@fd04939.

PR-URL: nodejs#18701
Refs: libuv/libuv#1655
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
`readable` and `writable` properties can be passed directly to the
`net.Socket` constructor. This change also avoids an unnecessary call
to `read(0)` on the `stdin` socket. This behavior was disclosed when
trying to merge `libuv@1.19.0` and specifically this commit:
libuv/libuv@fd04939.

PR-URL: #18701
Refs: libuv/libuv#1655
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
`readable` and `writable` properties can be passed directly to the
`net.Socket` constructor. This change also avoids an unnecessary call
to `read(0)` on the `stdin` socket. This behavior was disclosed when
trying to merge `libuv@1.19.0` and specifically this commit:
libuv/libuv@fd04939.

PR-URL: #18701
Refs: libuv/libuv#1655
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
santigimeno pushed a commit to libuv/libuv that referenced this pull request Feb 25, 2018
`UV_READABLE_PIPE` and `UV_WRITABLE_PIPE` flags should be honored on
unices.
Windows is better about already doing this, so this will make the
behavior of these flags more consistent across platforms.
It also is just better to set these flags to reflect the actual
mode of the stream, rather than guessing at it based on typical usage.

Refs: #1655
Refs: nodejs/node#18701
PR-URL: #1741
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
`readable` and `writable` properties can be passed directly to the
`net.Socket` constructor. This change also avoids an unnecessary call
to `read(0)` on the `stdin` socket. This behavior was disclosed when
trying to merge `libuv@1.19.0` and specifically this commit:
libuv/libuv@fd04939.

PR-URL: nodejs#18701
Refs: libuv/libuv#1655
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 7, 2018
`readable` and `writable` properties can be passed directly to the
`net.Socket` constructor. This change also avoids an unnecessary call
to `read(0)` on the `stdin` socket. This behavior was disclosed when
trying to merge `libuv@1.19.0` and specifically this commit:
libuv/libuv@fd04939.

PR-URL: #18701
Refs: libuv/libuv#1655
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
rvagg pushed a commit that referenced this pull request Aug 16, 2018
`readable` and `writable` properties can be passed directly to the
`net.Socket` constructor. This change also avoids an unnecessary call
to `read(0)` on the `stdin` socket. This behavior was disclosed when
trying to merge `libuv@1.19.0` and specifically this commit:
libuv/libuv@fd04939.

PR-URL: #18701
Refs: libuv/libuv#1655
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2018
jbunton-atlassian added a commit to jbunton-atlassian/node that referenced this pull request Dec 19, 2018
Since commit 1d90700 child processes
have not correctly emitted a close event for stdin.

This change triggers a zero-byte read from the Socket constructor for
any child process sockets. That allows the close event to be detected.

Refs: nodejs#18701
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. child_process Issues and PRs related to the child_process subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants