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

IPC direct writing fails on Windows #17405

Closed
elibarzilay opened this issue Nov 30, 2017 · 8 comments
Closed

IPC direct writing fails on Windows #17405

elibarzilay opened this issue Nov 30, 2017 · 8 comments
Labels
child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations.

Comments

@elibarzilay
Copy link

  • Version: 9.2.0
  • Platform: Windows
  • Subsystem: child_process

Using a fie descriptor directly to send IPC messages is broken on
Windows:

  • x.js:

    require("child_process").spawn("node", ["y.js"],
                                   {stdio: ["ignore", "pipe" , "pipe", "ipc"]})
      .on("message", m => console.log("got a message:", m));
  • y.js:

    require('fs').createWriteStream(null, {fd: 3})
      .write(`{"m": "Test"}\n`, "utf8");

On Linux, this works fine, but on Windows I'm getting:

Assertion failed: avail >= sizeof(ipc_frame.header), file src\win\pipe.c, line 1593

If I change y.js to use process.send it works fine again. (There's
no reason to avoid it, but we want to get IPC messages from a python
subprocess, which fails in the same way.)

@addaleax addaleax added child_process Issues and PRs related to the child_process subsystem. duplicate Issues and PRs that are duplicates of other issues or PRs. labels Nov 30, 2017
@addaleax
Copy link
Member

I think this is the same issue as #16491?

@elibarzilay
Copy link
Author

@addaleax, that issue talks about sending junk, but I'm sending a json
string. Also, the docs say that

The input and output on this fd is expected to be line delimited JSON
objects.

and it works on linux.

Is there any difference in the expected text on Windows? Is it
documented anywhere?

In any case, if there is no difference then this is a different problem,
and if there is, then this turns into a documentation issue in the line
I quoted above. So I think that it's not a duplicate.

@addaleax addaleax removed the duplicate Issues and PRs that are duplicates of other issues or PRs. label Dec 1, 2017
@bnoordhuis bnoordhuis added the doc Issues and PRs related to the documentations. label Dec 1, 2017
@bnoordhuis
Copy link
Member

The documentation needs an update. I've added labels. PR welcome.

Is there any difference in the expected text on Windows?

Quite. Libuv uses a binary protocol on Windows.

Is it documented anywhere?

No, and it shouldn't be, it's internal.

@elibarzilay
Copy link
Author

@bnoordhuis: is there any reason for a binary protocol? (Practical
curiosity, since we're dealing with a subprocess that can be remote and
therefore a natural way to do it is to still pass json in lines of
text.)

But back to the problem, if the intention is to not rely on even the
possibility of a text-based channel, and if the actual representation is
internal, then the proper fix to the docs is to remove that "line
delimited JSON objects" sentence and replace it by a comment that says
that one must use libuv to send messages from a non-node subprocess.
No?

(At least I assume that if it's internal to the library, then it does
provide some way to use it. I haven't actually looked into it since it
looks like an overkill when we just need to send simple messages.)

@bnoordhuis
Copy link
Member

is there any reason for a binary protocol?

Simplicity and efficiency. Libuv doesn't want to be in the business of parsing a complex protocol like JSON.

then the proper fix to the docs is to remove that "line delimited JSON objects" sentence

It wouldn't surprise me if that line predates the Windows port. Perhaps add an "on UNIX" qualifier.

@elibarzilay
Copy link
Author

@bnoordhuis But at least the simplicity+efficiency reason sounds like
something that would likely be applicable to non-Windows too, right?

@bzoz
Copy link
Contributor

bzoz commented Dec 4, 2017

I’ve done some investigating regarding #16491. I even made a fix for that issue. The problem is, while it fixed using console.log, it still does not support fs.wrtie - and there seems to be no easy way fixing that.

But also, it turns out it does not work on Linux too. If you try to use process.stdout, assert will be triggered:

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

So, I think we should remove that line for all platforms.

@elibarzilay, you can try using normal named pipe for communication. Or, you can try using libuv (maybe https://github.com/saghul/pyuv?) to send your messages.

bzoz added a commit to JaneaSystems/node that referenced this issue Dec 4, 2017
IPC messages are more complicated than a simple pipe passing JSON
objects separated by new line. This removes inaccurate notes about
implementation from the documentation.

Fixes: nodejs#16491
Fixes: nodejs#17405
@elibarzilay
Copy link
Author

@bzoz -- thanks for the doc fix. Re my communication problem, I plan on just using an additional FD, so there's no need for a named pipe...

MylesBorins pushed a commit that referenced this issue Dec 12, 2017
IPC messages are more complicated than a simple pipe passing JSON
objects separated by new line. This removes inaccurate notes about
implementation from the documentation.

PR-URL: #17460
Fixes: #16491
Fixes: #17405
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Dec 12, 2017
IPC messages are more complicated than a simple pipe passing JSON
objects separated by new line. This removes inaccurate notes about
implementation from the documentation.

PR-URL: #17460
Fixes: #16491
Fixes: #17405
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this issue Dec 20, 2017
IPC messages are more complicated than a simple pipe passing JSON
objects separated by new line. This removes inaccurate notes about
implementation from the documentation.

PR-URL: #17460
Fixes: #16491
Fixes: #17405
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this issue Dec 20, 2017
IPC messages are more complicated than a simple pipe passing JSON
objects separated by new line. This removes inaccurate notes about
implementation from the documentation.

PR-URL: #17460
Fixes: #16491
Fixes: #17405
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 a pull request may close this issue.

4 participants