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

Expand Process.Kill to Optionally Kill a Process Tree #26234

Closed
bgribaudo opened this issue Jan 9, 2018 · 21 comments

Comments

@bgribaudo
Copy link
Contributor

@bgribaudo bgribaudo commented Jan 9, 2018

Issue

.Net Standard does not provide a means to kill a process tree (that is, a given process and all of its child/descendant processes).

In the Windows world, this can be done by assembling the list of descendant processes to terminate using WMI's Win32_Process management object, performance counters or P/Invoke, then killing each one, then killing the parent/root process. However, since .Net Standard doesn't provide a way to enumerate child/descendant processes, it's impossible for a user to directly implement this approach in a cross-platform way using only .Net Standard.

If the ability to enumerate child processes were added (as #25855 proposes), there'd still be complexities involved with using the child process list to kill a process tree as care is needed to ensure that unrelated processes aren't inadvertently terminated because of process ID reuse. This proposal saves the developer from needing to worry about such complexities and instead provides a simple, one-line way to terminate a process tree.

Rationale

Sometimes, it's necessary to end a process tree. :-)

For example, Microsoft.DotNet.Tools.Test.Utilities.ProcessExtension relies on different code paths to handle killing a process tree on Windows and Linux. Both code paths involve invoking one or more command-line processes. If this proposal were implemented, presumably this logic could be replaced with a single-line kill all method call followed possibly by a call to WaithForExit(int milliseconds). No multiple code paths. No need to invoke command-line processes.

Another example: A project I'm currently working on uses integration tests that use Process to call dotnet run to start a server process. When the test has completed, it's possible to use Process's Kill() to end the parent dotnet process. However, this leaves child processes still running—which causes problems.

Proposal

Extend Process as follows:

Original (ignore—left for reference purposes):

    public class Process : ....    {
        public void Kill(bool entireProcessTree = false); // false = existing behavior of Kill()
    }

Revision (ignore—left for reference purposes):

    public enum ProcessGroup { 
            CurrentProcess, 
            EntireTree 
    }

    public class Process : ....    {
        public void Kill(ProcessGroup group = ProcessGroup.CurrentProcess) { } // default = existing behavior of Kill()
    }

Approved Version:

    public class Process : ....    {
        public void Kill(bool entireProcessTree); 
    }

With this change, Kill will behave exactly as it does at current if it is called with no arguments or with false as its argument. However, when called with true as its argument, in a platform-appropriate way all descendant processes will be killed, followed by the process represented by the current Process instance.

[edited by @danmosemsft to add c# formatting only]

@danmosemsft

This comment has been minimized.

Copy link
Member

@danmosemsft danmosemsft commented May 11, 2018

Implementation note for Windows: #25855 (comment)

@krwq

This comment has been minimized.

Copy link
Member

@krwq krwq commented Jun 26, 2018

I think it should be an enum not a bool. I'd expose at least three flags (even if DirectChildren will be outvoted I'd still vote for making it an enum):

  • OnlySelf
  • EntireTree
  • DirectChildren
@bgribaudo

This comment has been minimized.

Copy link
Contributor Author

@bgribaudo bgribaudo commented Jun 27, 2018

Good thought about the enum, @krwq! I like how it makes it easier to add variations of behavior down the road. For example, if it becomes desirable to do 'Entire Tree + Wait for Exit'.

@krwq

This comment has been minimized.

Copy link
Member

@krwq krwq commented Jun 28, 2018

@bgribaudo let's update the proposal with enum if you feel this is a good idea. What about something like this for now (DirectChildren can wait until someone requests it and likely will be a part of implementation of EntireTree anyway):

enum ProcessGroup
{
    OnlySelf, // or CurrentProcess perhaps
    EntireTree
}
@bgribaudo

This comment has been minimized.

Copy link
Contributor Author

@bgribaudo bgribaudo commented Jun 29, 2018

@krwq, done. Thanks!

@GrabYourPitchforks

This comment has been minimized.

Copy link
Member

@GrabYourPitchforks GrabYourPitchforks commented Jul 17, 2018

Implementation note on Windows: using Task Manager's "kill entire tree" functionality does two interesting things: (a) it won't kill the taskmgr process itself even if it's a child of the tree to be killed; and (b) it keeps going even in the face of failure, choosing to report a single "something went wrong" error at the very end.

@terrajobst

This comment has been minimized.

Copy link
Member

@terrajobst terrajobst commented Jul 17, 2018

Video

We don't see the value in an enum as it seems unlikely that we'd do more than the two options. All examples we could come up with would likely result in more parameters. The usability of booleans on call sides have been solved in C# with named parameters.

We shouldn't make the parameter optional as there is no way this will ever be used by the compiler as we'd still have the parameterless version (which cannot be removed as that would be a binary breaking change).

@danmosemsft

This comment has been minimized.

Copy link
Member

@danmosemsft danmosemsft commented Jul 17, 2018

@bgribaudo it took a while, but this is approved. Any interest in offering a PR, so it can be in 3.0?

@danmosemsft

This comment has been minimized.

Copy link
Member

@danmosemsft danmosemsft commented Jul 17, 2018

For clarity this is what we agreed:

    public class Process : ....    {
        public void Kill(bool entireProcessTree); 
    }
@bgribaudo

This comment has been minimized.

Copy link
Contributor Author

@bgribaudo bgribaudo commented Jul 26, 2018

Thanks, @danmosemsft! Edited the top question so that it reflects the approved version.

@danmosemsft

This comment has been minimized.

Copy link
Member

@danmosemsft danmosemsft commented Jul 26, 2018

So @bgribaudo do you want to take a shot at the implementation? Even just one of Windows and Linux if you want.

@bgribaudo

This comment has been minimized.

Copy link
Contributor Author

@bgribaudo bgribaudo commented Jul 26, 2018

Implementation Notes

Microsoft.Build.Utilities.ProcessExtensions looks like a great starting place. Implementation may be as simple as copying-and-pasting the relevant code, tweaking + adding relevant tests.

aspnet Common's Microsoft.Extensions.Internal.ProcessHelper/CLI's Microsoft.DotNet.Tools.Test.Utilities.ProcessExtensions show an alternative approach for terminating a Windows process tree that uses taskkill. However, this logic does not work on universally on Windows because not all editions of Windows ship with taskkill (for example Windows Nano). Due to this limitation, ProcessExtensions's approach seems preferable.

@rainersigwald

This comment has been minimized.

Copy link
Contributor

@rainersigwald rainersigwald commented Jul 26, 2018

I'm not aware of any problems with MSBuild's P/Invoke approach after the .NET Core port. Note that we only call it from https://github.com/Microsoft/msbuild/blob/cc451ea752ef6cfb04d9a3abab05f28f9a2fec41/src/Utilities/ProcessExtensions.cs#L14-L45 which has some cross-platform options that seem to be behaving ok.

@bgribaudo

This comment has been minimized.

Copy link
Contributor Author

@bgribaudo bgribaudo commented Jul 26, 2018

So @bgribaudo do you want to take a shot at the implementation? Even just one of Windows and Linux if you want.

Yes, I'll be glad to give it a try. :-)

@danmosemsft

This comment has been minimized.

Copy link
Member

@danmosemsft danmosemsft commented Jul 26, 2018

Yes I suggest copying https://github.com/Microsoft/msbuild/blob/master/src/Utilities/ProcessExtensions.cs#L14. If you have a Mac, or can host a Linux VM, either should be fine for testing Unix behavior.

@danmosemsft

This comment has been minimized.

Copy link
Member

@danmosemsft danmosemsft commented Jul 26, 2018

For the tests, I would take a look through the existing tests for Process/ProcessStartInfo etc and copy the pattern. Make sure you save your work before you test for bugs in your implementation of killing the process tree 😆

@bgribaudo

This comment has been minimized.

Copy link
Contributor Author

@bgribaudo bgribaudo commented Aug 13, 2018

@danmosemsft, do you have any ideas on how to do something like

Process root = CreateProcess(() =>
{
Process child1 = CreateProcess(() =>
{
Process child2 = CreateProcess(() =>
{
Process child3 = CreateProcess(() => SuccessExitCode);
child3.Start();
Assert.True(child3.WaitForExit(WaitInMS));
return child3.ExitCode;
});
child2.Start();
Assert.True(child2.WaitForExit(WaitInMS));
return child2.ExitCode;
});
child1.Start();
Assert.True(child1.WaitForExit(WaitInMS));
return child1.ExitCode;
});
root.Start();
but where the code that starts the root process (line 241) can get ahold of the process IDs of child 1, 2 & 3?

Unfortunately, I can't simply define variables for the PIDs in that outer scope then assign to those variables from within each child's create Func because "Field marshaling is not supported by RemoteInvoke".

Context:
Currently, I'm trying to use platform-specific test code to build up a small tree of processes for test purposes. However, doing something like the above would be preferable because (it appears to be) simple and cross platform.

@krwq

This comment has been minimized.

Copy link
Member

@krwq krwq commented Aug 13, 2018

@bgribaudo I think you can communicate PIDs back to the main process through i.e. AnonymousPipe (that will require setting handle inheritance but I'd check first if this doesn't happen by default). One platform which might be slightly trickier to test than others is UWP - I think it should be fine to skip testing there for now (please file an issue on that if you decide to skip it).

Other option is to simply print it out to stdout and make sure all of the child processes reprint their subprocesses stdouts.

@danmosemsft

This comment has been minimized.

Copy link
Member

@danmosemsft danmosemsft commented Aug 14, 2018

@bgribaudo RemoteInvoke can take input parameters, so long as they can be converted easily to strings.
Example: https://github.com/dotnet/corefx/blob/master/src/System.IO.FileSystem/tests/FileStream/LockUnlock.cs#L174

It passes them as command line parameters. On return, you can only get the return code, so for more, you'd have to use a file or pipe etc.

See src\CoreFx.Private.TestUtilities\src\System\Diagnostics\RemoteExecutorTestBase.cs for all the overloads.

@bgribaudo

This comment has been minimized.

Copy link
Contributor Author

@bgribaudo bgribaudo commented Aug 14, 2018

Thanks, @krwq & @danmosemsft!

@danmosemsft

This comment has been minimized.

Copy link
Member

@danmosemsft danmosemsft commented Feb 2, 2019

done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.