win: Close child pipes when child exits (+ test)

[eladb]

The reason this is needed on Windows is that the pipe handles that are passed to the child process via CreateProcess must be inheritable and therefore automatically passed to grandchildren (and further). This causes
the pipes to remain open even if the child process exits, because there are open file handles on that pipe.

The impact on users is that they cannot trust that the streams will be closed when the child process exits. node.js for example, waits for these close events before emitting the exit event on the child process (see
nodejs/node-v0.x-archive#2914).

On unix systems, the file descriptors of the pipes are not inherited by descendents of the child process and therefore, when the child dies, the pipes are closed automatically.

There doesn't seem to be a way to tell Windows either to destroy the pipes or not to inherit the handles. This could be done from the child process but the child process cannot be trusted to do it.

Some alternatives we tried:

• Reverse the roles of the named pipe (sending the server handle to the child process as stdio instead of the client handle) -- same behavior.
• Try to use a different way to identify that a client was closed (since we use async I/O, we don't get a 0-bytes read indication on child closed (seems like this could have worked but obviously we can't do synchronous read from the pipe).
• Many more variations of DuplicateHandle, SetHandleInformation, Get|SetStdHandle and more.

This also reproduces on a stand-alone Windows program.

PS. another side effect of the handle inheritence is that each level in the child process hierarchy has 3 more open file handles (first level has 3, second has 6, third has 9, etc). Could have some perf impact on deep process
trees.

Contributed: @yosefd @saary

[eladb]

@igorzi @tjanczuk I would appreciate any feedback you might have on this.

[yosefd]

Looks good!

[piscisaureus]

The reason this is needed on Windows is that the pipe handles that are passed to the child process via CreateProcess must be inheritable and therefore automatically passed to grandchildren (and further). This causes the pipes to remain open even if the child process exits, because there are open file handles on that pipe.
The impact on users is that they cannot trust that the streams will be closed when the child process exits. node.js for example, waits for these close events before emitting the exit event on the child process (see nodejs/node-v0.x-archive#2914).

On unix systems, the file descriptors of the pipes are not inherited by descendents of the child process and therefore, when the child dies, the pipes are closed automatically.

The situation on unix is mostly the same as on windows. On unix spawning a child process is done via fork() and exec(). A child process will inherit all file descriptors except those explicitly marked with FD_CLOEXEC. See http://blogs.msdn.com/b/oldnewthing/archive/2011/12/16/10248328.aspx for some background information.

There doesn't seem to be a way to tell Windows either to destroy the pipes or not to inherit the handles. This could be done from the child process but the child process cannot be trusted to do it.
Some alternatives we tried:
Reverse the roles of the named pipe (sending the server handle to the child process as stdio instead of the client handle) -- same behavior.
Try to use a different way to identify that a client was closed (since we use async I/O, we don't get a 0-bytes read indication on child closed (seems like this could have worked but obviously we can't do synchronous read from the pipe).
Many more variations of DuplicateHandle, SetHandleInformation, Get|SetStdHandle and more.
This also reproduces on a stand-alone Windows program.
PS. another side effect of the handle inheritence is that each level in the child process hierarchy has 3 more open file handles (first level has 3, second has 6, third has 9, etc). Could have some perf impact on deep process
trees.

Sometimes it is desirable that the child process inherits the parent's stdio handles. If the grandchild inherits the child's stdio (e.g. so that child and grandchild share the same stdout) this would just cut the line.

I think a better solution would be:

• Disable inheritance for stdio handles when node starts up.
• Don't wait for all the stdio pipes to close before emitting 'exit' in node. I don't recall why we're waiting for it in the first place. @ry @bnoordhuis you remember?

BTW, this is only an issue when you spawn a process that subsequently spawns another child process and then exits itself. It seems reasonable to spawn these "detached" child processes with inheritance off, and with NULL stdio handles. This is about the same as people would do on unix (they would close file descriptors 0, 1, 2, and reopen them as /dev/null).

[piscisaureus]

@eladb Is the "middle" child a node process? If so, can you try to apply this patch and see if it fixes your problem?

https://gist.github.com/2024391

[eladb]

@piscisaureus The tests here are just regular libuv tests, not node. I have the same tests written in node, so we will run them tomorrow with this patch and update.

We were also thinking about making the pipes non-inheritable at the child level, but the use case this addresses is actually when one spawns non-node long-running children and they spawn their own child processes, so we can't really control their handle inheritance policy.

Dealing with this at the node level makes sense (and worked for our case). My thinking was that this is an opportunity to iron out a behavioral difference between the platforms at the libuv level, because evidently there is in this case. I am curious now what's the exact difference between Windows and unix when it comes to breaking the pipe when the child exits...

[piscisaureus]

@eladb This looks like a "bug" in your third-party child process to me. We will probably change the 'exit' event semantics so you can properly detect when the immediate child exits and close the pipes within node.

I am curious, if the immediate child is not node, then what is it?

[eladb]

@piscisaureus The situation we ran into is not a child deliberately exiting (in order to daemonize it's grandchild or something), it is a fault that caused it to crash, leaving that grandchild alive and never notifying its parent. It's a sad and helpless situation... :-)

The node 'exit' event semantics change sounds good. As long as the parent process can tell that one of his children exited, I think it is consistent and allows the user to deal with it. Making stdio handles non-inheritable at the node level also makes sense, regardless.

The immediate child processes we have in our case are node, .NET programs, mongodb, stuff like that. All of which may spawn extra child processes.

[bnoordhuis]

Don't wait for all the stdio pipes to close before emitting 'exit' in node. I don't recall why we're waiting for it in the first place. @ry @bnoordhuis you remember?

Probably to make some tests pass. :-)

[eladb]

@ry Any idea what happened with nodejs/node-v0.x-archive#818? Seems like this was already discussed at some point.

[eladb]

@piscisaureus Here's the node.js test that reproduces this issue: https://gist.github.com/2029393

The gist you posted doesn't seem to resolve this. Looking at the process tree, it seems that the handles are still inherited by the grandchild. From initial investigation of this, it looks like uv_pipe_open is called only on one of the tree handles, nevertheless, even this one is inherited. Weird...

Test results

node-2.6.12 is node v2.6.12
node-343 is node with #343
node-2024391 is node patched with the gist

windows

> node-2.6.12 test.js
elapsed time to exit: 2189ms
AssertionError: expecting exit event to be emitted after less than 2s since child exits after 500ms

> node-343 test.js
elapsed time to exit: 768ms

> node-2024391 test.js
elapsed time to exit: 2262ms
AssertionError: expecting exit event to be emitted after less than 2s since child exits after 500ms

unix (macosx)

$ node-2.6.12 test.js
elapsed time to exit: 568ms

node-343 and node-2024391 are windows-only patches, so their results will be the same

[eladb]

We found an issue with this fix in node: using close_pipe will not clean up any handles at upper layers and this can cause an "invalid handle" error to crash the process in case someone tries to read/write from the pipe after it has been closed.

This can be fixed at the libuv layer by using uv_close instead. However, since the general approach seems to be to fix this at the node layer anyway, we moved the pipe closing to the node layer (see anodejs/node@e8e4604), which also cleans up the handle references.

[eladb]

Closing. Fixed in nodejs/node-v0.x-archive#2944