doc: mark accessing IPC channel fd as undefined

[bzoz]

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

[seishun]

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

[Trott]

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.

[sam-github]

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.

[mcollina]

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

[jasnell]

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

[seishun]

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

Or should Node.js turn it into an exception?

I think this is preferable.

[bzoz]

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]

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]

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

Updated, removed Fixes , PTAL.

[bzoz]

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

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

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]

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

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

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

[bzoz]

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

@nodejs/collaborators PTAL

[bzoz]

Updated, PTAL

[mcollina]

LGTM

[BridgeAR]

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]

CI https://ci.nodejs.org/job/node-test-commit-light/290/

[BridgeAR]

Landed in ea1a076 🎉