child_process: create proper public API for `channel` by addaleax · Pull Request #30165 · nodejs/node

addaleax · 2019-10-29T20:00:45Z

Instead of exposing the C++ bindings object as subprocess.channel
or process.channel, provide the “control” object that was
previously used internally as the public-facing variant of it.

This should be better than returning the raw pipe object, and
matches the original intention (when the channel property was
first added) of providing a proper way to .ref() or .unref()
the channel.

Refs: #9322
Refs: #9313

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

nodejs-github-bot · 2019-11-01T11:52:28Z

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

addaleax · 2019-11-01T11:53:26Z

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2081/

@nodejs/child_process

addaleax · 2019-11-02T19:34:48Z

@vsemozhetbyt If it makes things easier for you, you can feel free to push to my branches directly – your suggestions are always helpful and I trust you to know how the docs should ideally look like :)

vsemozhetbyt · 2019-11-02T20:33:18Z

@addaleax Thank you) Personally, I am not so sure about my nitpicking myself, and recently I usually review in occasions not so convenient for git operations. But I am grateful for your lingering encouragement)

bnoordhuis · 2019-11-04T14:29:27Z

Is refExplicitlySet necessary? Are these *Counted methods? It's the user's responsibility to pair ref and unref calls, balancing internal ref/unref calls is our responsibility. I'm not sure I see why they're needed.

@bnoordhuis Yeah, I’m not a fan of this either, but without this tests are failing and I wouldn’t know a better solution … the issue isn’t so much that the calls might end up unbalanced, it’s that currently, a process.channel.unref() call for example from userland is expected to unref the channel even if there are event listeners that would otherwise keep it ref()’ed.

bnoordhuis · 2019-11-04T14:34:05Z

Oh, and for a bit of meta discussion: I added the control object as a fix for a bug, I didn't otherwise put a lot of thought into it.

Since you're thinking of promoting it to public API: do you think it's the best possible interface?

addaleax · 2019-11-05T18:33:59Z

Oh, and for a bit of meta discussion: I added the control object as a fix for a bug, I didn't otherwise put a lot of thought into it.

Since you're thinking of promoting it to public API: do you think it's the best possible interface?

I think it’s fine … or, rather, I think having an API of the shape { ref(), unref(), get fd() } seems okay to me for now, and if we ever need something else I don’t think there’s anything in the way of adding that? I just don’t want to have the raw Pipe object exposed here…

nodejs-github-bot · 2019-11-05T22:16:17Z

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

bnoordhuis · 2019-11-06T12:47:56Z

parallel/test-child-process-pipe-dataflow failure on Windows looks related.

edit: although it could also be #25988.

nodejs-github-bot · 2019-11-06T12:53:01Z

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

addaleax · 2019-11-30T17:14:01Z

@lundibundi Addressed your comments and rebased, PTAL

nodejs-github-bot · 2019-11-30T17:14:56Z

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

BridgeAR · 2019-12-25T20:06:19Z

This needs a rebase.

BridgeAR · 2020-01-02T16:03:48Z

Ping @addaleax

Instead of exposing the C++ bindings object as `subprocess.channel` or `process.channel`, provide the “control” object that was previously used internally as the public-facing variant of it. This should be better than returning the raw pipe object, and matches the original intention (when the `channel` property was first added) of providing a proper way to `.ref()` or `.unref()` the channel. Refs: nodejs#9322 Refs: nodejs#9313

nodejs-github-bot · 2020-01-03T14:09:56Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28155/ ✔️

Instead of exposing the C++ bindings object as `subprocess.channel` or `process.channel`, provide the “control” object that was previously used internally as the public-facing variant of it. This should be better than returning the raw pipe object, and matches the original intention (when the `channel` property was first added) of providing a proper way to `.ref()` or `.unref()` the channel. PR-URL: #30165 Refs: #9322 Refs: #9313 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

BridgeAR · 2020-01-03T15:34:43Z

Landed in e65bed1 🎉

addaleax added child_process Issues and PRs related to the child_process subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Oct 29, 2019

nodejs-github-bot added the process Issues and PRs related to the process subsystem. label Oct 29, 2019

addaleax added the review wanted PRs that need reviews. label Nov 1, 2019

vsemozhetbyt reviewed Nov 2, 2019

View reviewed changes

Comment thread doc/api/child_process.md Outdated

Comment thread doc/api/child_process.md Outdated

bnoordhuis reviewed Nov 4, 2019

View reviewed changes

addaleax force-pushed the child-process-channel-public branch from a1a949b to 8e5f915 Compare November 5, 2019 22:15

Fishrock123 approved these changes Nov 19, 2019

View reviewed changes

lundibundi reviewed Nov 23, 2019

View reviewed changes

Comment thread doc/api/child_process.md Outdated

Comment thread doc/api/process.md Outdated

Comment thread lib/internal/process/stdio.js Outdated

addaleax force-pushed the child-process-channel-public branch from f3b2b3b to b47e13e Compare November 30, 2019 17:13

lundibundi approved these changes Nov 30, 2019

View reviewed changes

Comment thread lib/internal/child_process.js Outdated

bnoordhuis approved these changes Dec 1, 2019

View reviewed changes

jasnell approved these changes Dec 1, 2019

View reviewed changes

addaleax force-pushed the child-process-channel-public branch from b47e13e to ef418e1 Compare January 3, 2020 00:53

addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed review wanted PRs that need reviews. labels Jan 3, 2020

This comment has been minimized.

Sign in to view

BridgeAR closed this Jan 3, 2020

addaleax deleted the child-process-channel-public branch January 4, 2020 03:22

BethGriggs mentioned this pull request Mar 10, 2020

2020-04-21, Version 14.0.0 (Current) #32181

Merged

ehmicky mentioned this pull request May 23, 2024

Document [sub]process.channel.refCounted() and [sub]process.channel.unrefCounted() #53128

Closed

Uh oh!

Conversation

addaleax commented Oct 29, 2019

Checklist

Uh oh!

nodejs-github-bot commented Nov 1, 2019

Uh oh!

addaleax commented Nov 1, 2019

Uh oh!

Uh oh!

Uh oh!

addaleax commented Nov 2, 2019

Uh oh!

vsemozhetbyt commented Nov 2, 2019

Uh oh!

bnoordhuis Nov 4, 2019

Choose a reason for hiding this comment

Uh oh!

addaleax Nov 5, 2019

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Nov 4, 2019

Uh oh!

addaleax commented Nov 5, 2019

Uh oh!

nodejs-github-bot commented Nov 5, 2019

Uh oh!

bnoordhuis commented Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Nov 6, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

addaleax commented Nov 30, 2019

Uh oh!

nodejs-github-bot commented Nov 30, 2019

Uh oh!

Uh oh!

BridgeAR commented Dec 25, 2019

Uh oh!

BridgeAR commented Jan 2, 2020

Uh oh!

This comment has been minimized.

nodejs-github-bot commented Jan 3, 2020 • edited by BridgeAR Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented Jan 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

bnoordhuis commented Nov 6, 2019 •

edited

Loading

nodejs-github-bot commented Jan 3, 2020 •

edited by BridgeAR

Loading