Skip to content
This repository has been archived by the owner on May 4, 2018. It is now read-only.

win: process trees / setsid? #338

Closed
wants to merge 2 commits into from
Closed

win: process trees / setsid? #338

wants to merge 2 commits into from

Conversation

eladb
Copy link

@eladb eladb commented Mar 8, 2012

On Windows, the default behavior for child processes is not to be killed when their parent die.

This change uses Windows Job Objects to implement the stick process trees for Windows.

A test named spawn_tree was added to verify this behavior: the test spawns a child and a grandchild. The child exits after 200ms and the grandchild asserts after 5 seconds. If the process tree is constructed propertly, the
child should kill the grandchild and then the assertion is not hit.

Credits: @saary, @yosefd, iisnode

On Windows, the default behavior for child processes is not to be killed
when their parent die. This is different from the behavior of most platforms.

This change uses [Windows Job Objects](http://msdn.microsoft.com/en-us/library/windows/desktop/ms684161(v=vs.85).aspx)
to implement the same behavior for Windows.

A test named `spawn_tree` was added to verify this behavior: the test spawns
a child and a grandchild. The child exits after 200ms and the grandchild
asserts after 5 seconds. If the process tree is constructed propertly, the
child should kill the grandchild and then the assertion is not hit.

Credits: @saary, @yosefd, [iisnode](https://github.com/tjanczuk/iisnode)
@eladb
Copy link
Author

eladb commented Mar 8, 2012

@eladb
Copy link
Author

eladb commented Mar 8, 2012

After testing this on a MacOSX and on Ubuntu, it seems like the default behavior is actually to keep the grandchild alive. The only difference in behavior as oppose to Windows is that Windows keeps the root process hanging as long as the grandchild is not killed while on the *nix systems, the grandchild is completely detached.

  • I would argue that killing all the child processes when the root process dies is a useful feature (and in my opinion, should be the default). Maybe { setsid: true }, which is currently not used at all should enable this behavior?
  • It seems that the current symptom on Windows is that the exit event is not received on the child process object: this node.js test passes on *nix and fails on Windows. The reason for this is that the child streams are not closed as long as the grandchild is still alive (even though the child died). In child_process.js\maybeExit(), the condition _closesGot < closesNeeded only asserts after the grandchild is exiting.

Any suggestions on how to proceed with this?

@yosefd
Copy link

yosefd commented Mar 8, 2012

I think that behavior on other systems is not good and hence we can propose change in behavior. Can it be controlled from node when running on UNIX or is it something inherent to UNIX?

Sent from my Windows Phone


From: Elad Ben-Israel
Sent: 3/8/2012 8:53 PM
To: Yosef Dinerstein
Subject: Re: [libuv] win: process trees (#338)

I tested this on a MacOSX and on Ubuntu and it seems like the default behavior is actually to keep the grandchild alive. The only difference in behavior as oppose to Windows is that Windows keeps the root process hanging as long as the grandchild is not killed while on the *nix systems, the grandchild is completely detached.

  1. I would argue that killing all the child processes when the root process dies makes more sense.
  2. On Windows, the side-effect of the hanging on node.js is that the exit event is not received this test passes on *nix and fails on Windows.

Any suggestions on how to proceed with this?


Reply to this email directly or view it on GitHub:
#338 (comment)

*
* *nix systems behave like this by default, but in Windows, the following is required:
*
* 1) When the process is initialized, a new job object is created with the attribute KILL_ON_JOB_CLOSE.
Copy link

Choose a reason for hiding this comment

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

Make sure that you don't go over 80 characters per line

@AvianFlu
Copy link
Contributor

AvianFlu commented Mar 8, 2012

@eladb I've taken a different approach to this problem in #329 - I'd be curious what you think.

@eladb
Copy link
Author

eladb commented Mar 11, 2012

@AvianFlu, I like the concept of detached in #329. I would expect detached = 0 to actually constitute an 'attached' child (that is, a child whose lifetime is bound to the lifetime of its parent). However, the current implementation in the pull request is that when detached is 0, we get a partly attached child (that's the current behavior in libuv).

How about we merge #338 and #329 into a single feature such that detached = 1 will indicate completely detached and detached = 0 will indicate completely attached? Do you think anyone actually uses the "partly-attached" mode? [well, I guess since this is the current behavior we must keep it as the default so not to break anyone]. I'm also wondering if this should maybe play along with the almost-deprecated setsid feature.

Also, did you run into the Windows bug where if a grandchild is still alive, the child's streams are not closed and then the exit callback is not called until the grandchild dies?

@eladb
Copy link
Author

eladb commented Mar 12, 2012

OK, closing for now after further investigation as this discusses two different issues:

  1. A feature that implements sticky process trees: this pull req implements the Windows version but if we introduce such a feature (e.g. as setsid: true or detach: false or whatever), we need to introduce it both for WIndows and Unix. I will follow up on this as I think it's a pretty useful feature (at least for scenarios we run into).
  2. A bug in node.js/libuv that causes the parent not to receive the exit event for child processes as long as the grandparent process is still alive. I opened win: exit event not emitted as long as a grandchild still exists nodejs/node-v0.x-archive#2914 with some more data points on this. If the fix will be in libuv, I will open another pull request here.

@eladb eladb closed this Mar 12, 2012
@eladb
Copy link
Author

eladb commented Mar 12, 2012

#343 fixes the bug on Windows

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants