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

doc: mark accessing IPC channel fd as undefined #17545

Closed
wants to merge 1 commit into from

Conversation

bzoz
Copy link
Contributor

@bzoz bzoz commented Dec 8, 2017

Adds note that accessing the fd of the IPC channel in any other way than process.send, or using the IPC channel with child processes that is not Node.js is undefined.

This adds to what was done in #17460

Fixes: #16491

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

docs

@nodejs-github-bot nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations. labels Dec 8, 2017

*Note*: Accessing the IPC channel fd in any way other than
[`process.send()`][] or using the IPC channel with a child process
that is not a Node.js instance is undefined.
Copy link
Contributor

Choose a reason for hiding this comment

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

What does "undefined" mean? Did you mean "undefined behavior"?

Copy link
Member

Choose a reason for hiding this comment

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

Nit: I'd prefer we remove *Note*: and just let the paragraph/sentence stand on its own. There are already 29 Notes in the doc as it is. We overuse that pattern. And the italicized Note: doesn't really call attention to things IMO.

Copy link
Contributor

@sam-github sam-github Dec 9, 2017

Choose a reason for hiding this comment

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

Using the IPC channel with a non-node child process used to be documented, and basically still is, though it looks like the purpose of the text got lost a bit in various rewrites:

Node.js processes launched with a custom execPath will communicate with the parent process using the file descriptor (fd) identified using the environment variable NODE_CHANNEL_FD on the child process. The input and output on this fd is expected to be line delimited JSON objects.

It now sounds like the env var and the JSON protocol is only to be used by node.js child processes, but that is a bit odd, if so, this information would be entirely implementation detail (like the protocol used internally by the cluster module, which is entirely undocumented). The only purpose to describe the env var and protocol so specifically is so that it can be used by non-node.js child processes. If node explodes when the protocol is abused, that's a bug - though a bug of the "doctor it hurts when I hit myself with a hammer" variety - should be fixed, but not very severe.

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer if we stated explicitly that the IPC channel should not be used if not through process.send()

Copy link
Member

Choose a reason for hiding this comment

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

minor suggestion: I would just drop the *Note*: prefix.

@seishun
Copy link
Contributor

seishun commented Dec 8, 2017

@nodejs/collaborators This is a bit of a philosophical question: is it acceptable for Node.js to crash with an assertion error (or possibly a segfault if it's a release build) if you write some innocuous-looking code that's actually disallowed by a note in the documentation? Or should Node.js turn it into an exception?

@lpinca
Copy link
Member

lpinca commented Dec 8, 2017

Or should Node.js turn it into an exception?

I think this is preferable.

@bzoz
Copy link
Contributor Author

bzoz commented Dec 8, 2017

I've tried fixing #16491 (commit here). While it fixed that issue, it turns out fs.write would still fail on Windows. OTOH using process.stdout.write triggers assert on Linux.

IMHO fixing this by marking it as undefined behavior in the documentation is the way to go at this point. There is nothing preventing us from making Node.js fire an exception instead of crashing in another PR and keeping this note. Or even at some point in the future, fixing this on all platforms and removing this note from documentation. But right now users are trying to access the IPC channel and it explodes for undocumented reasons.

@Trott
Copy link
Member

Trott commented Dec 8, 2017

I'm OK with this doc note if we open an issue for it where the preferable behavior is described and attention is called to this doc note, explaining that it will need to be removed or changed once the preferable behavior is implemented. (EDIT: Maybe just remove the "Fixes" here so that issue stays open?)

@BridgeAR
Copy link
Member

@bzoz can you elaborate in what way it can not be turned into a exception right now / why that is not ideal right now?

@bzoz
Copy link
Contributor Author

bzoz commented Dec 21, 2017

Updated, removed Fixes , PTAL.

@bzoz
Copy link
Contributor Author

bzoz commented Dec 21, 2017

@BridgeAR I guess that this is rather easy on Windows, making libuv return error instead of firing an assert. I'm not sure about Linux, it seems like it works but then Node dies when exiting:

node: ../deps/uv/src/unix/core.c:881: uv__io_stop: Assertion `loop->watchers[w->fd] == w' failed.

It is definitely possible to fix. We can even change the implementation to use normal named pipe instead of this IPC pipe that is causing problems. But it all will take time testing and implementing, for now lets document this.

@BridgeAR
Copy link
Member

@bzoz there seems to be a strong preference to try to throw an error instead of having this documentation change. I understand that it might be difficult to get it working everywhere but maybe it would be best to just open a PR that works on Windows and ask someone else to look into the Linux?

@seishun
Copy link
Contributor

seishun commented Dec 21, 2017

According to @bnoordhuis, libuv works as intended (#16491 (comment)). So the fix would be in Node.js proper, and thus would most likely be the same for both Windows and Linux.

@bzoz
Copy link
Contributor Author

bzoz commented Dec 21, 2017

@seishun that comment is reffering to the the fact that libuv complains about garbage in the IPC chanell. This has to be changed in libuv, since it is the libuv that fires an assert when unexpected data is received.

@BridgeAR I don't know when I'll have time to do this, and it will be only a partial solution. I will get back to this at some point, but for now lets document what is the current state.

@BridgeAR
Copy link
Member

@bzoz I will abstain from giving a +-1 and so far everyone else did the same. So I personally have the feeling this is not going to land as is.

@seishun
Copy link
Contributor

seishun commented Dec 21, 2017

@bzoz If you disagree with that comment, why? If not, why do you think the assert has to be changed in libuv anyway?

@bzoz
Copy link
Contributor Author

bzoz commented Dec 21, 2017

@BridgeAR Fixing this would be better, but IMHO it will not be an easy fix and not one that we can expect to be made soon.

@seishun If libuv detects that the IPC pipe is misused, it will terminate the process. Asserts like this one will have to be changed to returning an error.

@BridgeAR
Copy link
Member

BridgeAR commented Jan 5, 2018

@nodejs/collaborators PTAL

Adds note that accessing the fd of the IPC channel in any other way
than process.send, or using the IPC channel with child processes that
is not Node.js is not supported.
@bzoz
Copy link
Contributor Author

bzoz commented Feb 15, 2018

Updated, PTAL

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@BridgeAR BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed tsc-agenda Issues and PRs to discuss during the meetings of the TSC. tsc-review labels Feb 16, 2018
@BridgeAR
Copy link
Member

I removed this from the TSC agenda again as I was the one who put it there.

I feel the changed text is good to land as is.

@BridgeAR
Copy link
Member

@BridgeAR
Copy link
Member

Landed in ea1a076 🎉

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 17, 2018
Adds note that accessing the fd of the IPC channel in any other way
than process.send, or using the IPC channel with child processes that
is not Node.js is not supported.

PR-URL: nodejs#17545
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@BridgeAR BridgeAR closed this Feb 17, 2018
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
Adds note that accessing the fd of the IPC channel in any other way
than process.send, or using the IPC channel with child processes that
is not Node.js is not supported.

PR-URL: #17545
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
Adds note that accessing the fd of the IPC channel in any other way
than process.send, or using the IPC channel with child processes that
is not Node.js is not supported.

PR-URL: #17545
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
Adds note that accessing the fd of the IPC channel in any other way
than process.send, or using the IPC channel with child processes that
is not Node.js is not supported.

PR-URL: #17545
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
Adds note that accessing the fd of the IPC channel in any other way
than process.send, or using the IPC channel with child processes that
is not Node.js is not supported.

PR-URL: nodejs#17545
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 7, 2018
Adds note that accessing the fd of the IPC channel in any other way
than process.send, or using the IPC channel with child processes that
is not Node.js is not supported.

PR-URL: #17545
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 7, 2018
Adds note that accessing the fd of the IPC channel in any other way
than process.send, or using the IPC channel with child processes that
is not Node.js is not supported.

PR-URL: #17545
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 9, 2018
Adds note that accessing the fd of the IPC channel in any other way
than process.send, or using the IPC channel with child processes that
is not Node.js is not supported.

PR-URL: #17545
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2018
MylesBorins pushed a commit that referenced this pull request Aug 16, 2018
Adds note that accessing the fd of the IPC channel in any other way
than process.send, or using the IPC channel with child processes that
is not Node.js is not supported.

PR-URL: #17545
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accidentally writing garbage to the IPC channel blows up parent in debug build
9 participants