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

Adding Kill(bool entireProcessTree) -- terminates a process tree #34147

Open
wants to merge 57 commits into
base: master
from

Conversation

@bgribaudo
Copy link

bgribaudo commented Dec 18, 2018

[Implements API approved by #26234]

Summary

When Kill(bool entireProcessTree)'s argument is false, behaves identical to Kill(); when true, terminates the current process and all descendant processes (children, grandchildren, etc.).

Platform Implementation Status

Implemented for Windows, OSX and Linux; not implemented for UAP, UnknownUnix or FreeBSD.

Notes

Potential low-hanging fruit: Behind-the-scenes, the logic in this PR enumerates child processes. This functionality could be leveraged to easily implement #25855.

Windows-specific logic inspired by msbuild's NativeMethodsShared KillTree.

[This PR replaces #31827]

bgribaudo added some commits Aug 17, 2018

@danmosemsft

This comment has been minimized.

Copy link
Member

danmosemsft commented Jan 8, 2019

@krwq @wtgodbe you are area owners of Process, can one of you please signoff.

@tmds does the treatment of Unix processes look correct?

Thanks for your patience @bgribaudo .. usually PR's go through a lot more quickly! I have a feeling we will discover more edge cases after this is committed.

}
}

private bool IsInTreeOf(Process processOfInterest)

This comment has been minimized.

@krwq

krwq Jan 10, 2019

Member

nit: I'd call it IsDescendant/IsDescendantOf/IsDescendantOrSelf

This comment has been minimized.

@bgribaudo

bgribaudo Jan 11, 2019

Does IsSelfOrDescendant work? Rename made; can adjust if you'd like.

bgribaudo added some commits Jan 10, 2019

catch (XunitException) when (times > 1)
{
await Task.Delay(delayInMilliseconds);
delayInMilliseconds *= 2;

This comment has been minimized.

@krwq

krwq Jan 15, 2019

Member

I'd cap it at 1s or some other reasonable value so that if someone reuses it in the future and increases times to say 20 they won't have to wait hours to finish. I.e.: delayInMiliseconds = Math.Min(1000, delayInMiliseconds * 2)

public void Kill_EntireProcessTree_True_ProcessNotStarted_ThrowsInvalidOperationException()
{
var process = new Process();
Assert.Throws<InvalidOperationException>(() => process.Kill(entireProcessTree: true));

This comment has been minimized.

@krwq

krwq Jan 15, 2019

Member

out of curiosity - not expecting any fixes - what's the issue on Uap? I'd presume you could start say cmd.exe and then inside of it run something else and check if both died later (not sure if checking if it died is possible though)

This comment has been minimized.

@bgribaudo

bgribaudo Jan 16, 2019

I didn't implement on UAP because I thought UAP doesn't give access to parent process IDs. (I don't see an API to access the parent PID at https://docs.microsoft.com/en-us/uwp/win32-and-com/win32-apis.)

Does that sound correct or am I missing something? :-)

This comment has been minimized.

@krwq

krwq Jan 16, 2019

Member

Sounds good, I wouldn't worry about UAP yet, if we need to fix it we can fix it separately 😄

@krwq

krwq approved these changes Jan 15, 2019

Copy link
Member

krwq left a comment

LGTM from implementation point of view, couple of stylistic comments - please fix them

[Fact]
[PlatformSpecific(TestPlatforms.Windows)]
[SkipOnTargetFramework(~TargetFrameworkMonikers.Uap)]
public void Kill_EntireProcessTree_ThrowsPlatformNotSupportedException()

This comment has been minimized.

@krwq

krwq Jan 15, 2019

Member

I think I'm missing one more test:

  • try killing self - although if it's somehow broken in the future it might be a pain to debug so perhaps make it an OuterLoop test
@bgribaudo

This comment has been minimized.

Copy link

bgribaudo commented Jan 16, 2019

@dotnet-bot please test Windows x64 debug build

@@ -63,11 +63,15 @@ public TimeSpan PrivilegedProcessorTime
}
}

/// <summary>Gets parent process ID</summary>
private int ParentProcessId =>
throw new PlatformNotSupportedException();

This comment has been minimized.

@tmds

tmds Jan 16, 2019

Member

Maybe create an issue for TODO?

}
}

private bool IsSelfOrDescendant(Process processOfInterest)

This comment has been minimized.

@tmds

tmds Jan 16, 2019

Member

IsSelfOrParentOf ?

This comment has been minimized.

@krwq

krwq Jan 16, 2019

Member

Parent suggest imediate parent, descendant suggests the tree - this is the name XPath is using as well (descendant-or-self)

This comment has been minimized.

@krwq

krwq Jan 16, 2019

Member

I don't mind adding Of though

{
return predicate();
}
catch (Exception e) when (e is InvalidOperationException || e is Win32Exception)

This comment has been minimized.

@tmds

tmds Jan 16, 2019

Member

This is a very broad try catch.
An alternative would be to:

  • not catch Win32Exceptions, e.g. permission issues should bubble up
  • make ParentProcessId a nullable int, that is null when the process has exited.

This comment has been minimized.

@danmosemsft

danmosemsft Jan 16, 2019

Member

It is also a bit friendlier for debugging if we try to not throw 1st chance exceptions in a "fairly normal" case like "already exited"


private IEnumerable<Exception> KillTree()
{
List<Exception> exceptions = new List<Exception>();

This comment has been minimized.

@tmds

tmds Jan 16, 2019

Member

It would be nice if the implementation would treat these cases the same:
Process instance knows the Process has exited
OS returns ESRCH because the process no longer exist
Currently one gives a InvalidOperationException (on HaveNonExitedId) that is swallowed and the other adds a Win32Exception exception to the list.

This comment has been minimized.

@krwq

krwq Jan 16, 2019

Member

I'd suggest to file issue for that (or perhaps 1 joint issue for all remaining feedback) and address it separately - this PR is already large enough as is.


private IEnumerable<Exception> KillTree()
{
List<Exception> exceptions = new List<Exception>();

This comment has been minimized.

@tmds

tmds Jan 16, 2019

Member

Instead of creating an aggregated list of exceptions, an alternative would be to stop on the first Process that causes an issue.
It may be simpler for the user to deal with a single exception.

}
catch (Win32Exception e)
{
exceptions.Add(e);

This comment has been minimized.

@tmds

tmds Jan 16, 2019

Member

Does it make sense to continue with this Process instance when Stop throws Win32Exception.

{
// Stop but don't kill the process. Keeps additional children from being started but leaves the process alive
// so that its children can be enumerated.
Stop();

This comment has been minimized.

@tmds

tmds Jan 16, 2019

Member

When Stop returns, the process may be stopping (not yet fully stopped).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment