Open bgribaudo opened 6 years ago
If implementing this for Windows note that processes expose their parent PID but this can represent a parent that has terminated and the PID may have been reused. The correct way (according to Win32 folks) is to also check the start time of the parent process is not after that of the child. See https://github.com/Microsoft/msbuild/blob/b499c93e95f440b98967b8d5edd910ee8556f504/src/Shared/NativeMethodsShared.cs#L1146
If a common use case would be to terminate process trees, then exposing GetParentProcessId() might lead callers to the "pit of failure" since they will not necessarily know to check the start time.
It also suggests that perhaps we should have an API to terminate a process tree "correctly"
If a common use case would be to terminate process trees, then exposing GetParentProcessId() might lead callers to the "pit of failure" since they will not necessarily know to check the start time.
@danmosemsft , good point! For my purposes, I wouldn't be opposed to dropping GetParentProcessId(). The nice thing about GetParentProcess() and GetChildProcesses() is that they can factor in the start time check.
It also suggests that perhaps we should have an API to terminate a process tree "correctly"
I like the idea. Maybe something like:
Kill(bool entireProcessTree = false); // false = existing behavior of Kill()
Can you think of use cases that would make it desirable to also offer an option to recursively CloseMainWindow() on the entire process tree?
It seems to me that GetParentProcess() and GetChildProcesses() could lead to the pit of failure as well, since the Process object when initialized is keyed by the process ID, which can be repurposed. It does not get a handle to the process until it needs one. So GetParentProcess() eg could give you a Process object that when you ultimately use it could act on an unrelated process.
If the scenario is killing a tree, can we just add Kill(bool includeChildren = false)
only?
I haven't personally had a scenario for CloseMainWindow() recursively. That can be added later if a scenario emerges.
If this makes sense please update the top comment (and title) as that's what the API review looks at.
Thanks, @danmosemsft! I created a new issue (#26234) for the kill all proposal. I left this as a separate issue as an alternative/complement to the new issue. If this issue doesn't gain traction soon (it's fine with me if it doesn't), I should probably close it.
I ran into a need for this, too. If choosing between this and https://github.com/dotnet/corefx/issues/26234, I would prefer dotnet/corefx#26234.
Either way, I think this is a good API. It appears there is a real need for working with process trees. Process tree killing has been implemented in several places already -- in MSBuild, ASP.NET Core, and the CLI. Would be nice to have one, common version of it in corefx.
@joperezr perhaps you could guide this and/or https://github.com/dotnet/corefx/issues/26234 to api-ready-for-review if appropriate?
Hi @joperezr (or @danmosemsft)! Is there anything I could do to help further this along?
Should
public Process[] GetChildProcesses(bool includeAllDescendants = false)
be
public IEnumerable<Process> GetChildProcesses(bool includeAllDescendants = false)
?
It seems the real scenario is killing a process tree. We're generally hesitant to add policy with complex policy where the behavior is hard to predict (although I personally believe killing the process is reasonably well defined as "kill as much as possible and keep going").
The trouble with exposing process navigation APIs that walking up often fails due to security concerns (walking down usually seem to succeed though).
My concern is that if the primary scenario is killing a tree, I'd really prefer this to be a single method call as this has a much higher chance of being reliable and a custom walk with exception handling.
@JeremyKuhne, do you own process?
@wtgodbe, @krwq own Process (see owners list)
I suggest adding Process.Kill(includeDescendants = false)
(not sure whether it should return void or not, but for safety descendants should be excluded by default) but not adding parent/child accessors because there is no other scenario at the moment and they are likely to have subtle behaviors.
Process.Kill(includeDescendants = false)
(or the equivalent) meets my needs. The idea of adding child & parent navigation (this issue) was an attempt to come up with a more generic way to provide what I needed to manually do the kill all descendants.
So, maybe move forward with dotnet/corefx#26234 and let this issue wait until someone presents a use scenario that specifically needs what it suggests?
@bgribaudo sounds good, could you please update your top post with the reduced proposal? API review looks at that - should be able to review againnext week
@danmosemsft, thanks! Would you like me to do that on this issue or over on dotnet/corefx#26234?
@bgribaudo oh, good point. Let's leave this aside then. I'll mark as api-needs-work while we wait for scenarios. Or you can close if you want.
Thanks, @danmosemsft!
Stumbled across this while looking for a an API to get parent process.
An application we're porting to .NET Core changes behaviour based on what parent process launched it. We were using p/invoke on ntdll.dll but that is obviously not cross-platform. Our current choice is to runtime test around the call and to choose a good default behaviour for other platforms while emitting a warning.
Valid use case for wanting to get children: We have a program that allows users to launch sub-processes that are out of our control and may themselves launch further sub-processes. I would like to provide a view where they can see all the launched processes to monitor their state.
Giving us the parent or the children will allow us to do this.
I guess the status of this is -- needs clear scenarios?
https://github.com/dotnet/diagnostics/issues/2977 is one, but perhaps not particularly compelling. Does anyone have others? If there are significant scenarios, this is a quite reasonable API to add IMO.
Does anyone have others?
@danmoseley What else do you want to know about the use case I posted on Apr 14, 2021 with 5 thumbs ups? Our program manages third party executables and we would like to show the user what sub-processes the parent executables have launched.
@phxnsharp ungh -- reading too many issues today. I see also @atruskie's scenario there.
Do either of you have feedback on the API shape?
@tmds do you have input from the Linux/Unix side here -- are there any subtle differences between Unix and Windows expectations?
@krwq you expressed concerns earlier -- do those remain? With respect to security issues, I actually think it's not much different to using the existing Process API's -- you will only see what you have rights to see. We don't enforce any security boundary, just have to choose whether to throw exceptions or eat errors in each case. And the implementation of Process.Kill had to deal with this too.
cc @dotnet/area-system-diagnostics-process for any other thoughts.
@danmoseley My requirement is that I can find all the sub-processes in a tree called from a known parent process. In an idealistic sense, having it deal with platform dependent idiosyncrasies instead of us having to do it would be wonderful. But I completely understand if that is out of scope for what dotnet is trying to do. At this point merely being able to do it would be a big win.
As for the other pitfall of the Process class only taking and ID and only creating a Handle on use, I would argue that that is a bug that is larger in scope than just the proposed GetParentProcess and GetChildProcesses APIs. Any API that returns a Process could fall prey to that bug and it should probably be fixed regardless of whether the new APIs are added (record the ID and the start time and throw if you later find the start time changed).
In short:
Thank you!
Thanks to the previous implementation of Kill(bool entireProcessTree)
we already have proven implementations of IsParentOf(Process possibleChildProcess)
and private int ParentProcessId
(the latter could be a method) for each OS.
Kill()
is able to terminate processes before it enumerates their children, so the outcome is well defined: when it returns there will be zero children remaining. Does it matter that what is returned from GetChildProcesses(bool includeAllDescendants)
is not so well defined, because it may not include child processes that were spawned during the enumeration? It may also included processes that have since terminated, or even that have changed their parent (not sure possible circumstances of that).
Sounds fine for our purpose. Thanks.
If ParentProcessId
is included it should be a property, similar to the other info exposed by Process
.
It can be cached, and invalidated when Refresh
is called
If
ParentProcessId
is included it should be a property, similar to the other info exposed byProcess
. It can be cached, and invalidated whenRefresh
is called
@tmds, are you proposing that the parent process id be loaded + cached by default for every Process
instance, or just that it be cached if the property is read?
Right now, Process
only needs to spend the effort figuring out the parent if the property is read, so the associated (small, presumably) cost isn't paid for the vast majority of users to who don't need paternal info.
public int GetParentProcessId() { ... } // returns parent process Id currently associated with this process public Process GetParentProcess() { … } // returns parent process currently associated with this process````
Is there a significant need for directly exposing the parent process ID from the child? Thinking that most of the time if someone gets the parent process ID directly, they'll just turn around and create a Process
instance using that ID--which scenario is satisfied by the second quoted line.
Wondering if the first quoted line should be removed from the proposal (so still offer to return a [validated] Process
instance for the parent, but not directly offer the parent process id).
or just that it be cached if the property is read
Similar to the other properties, cached when read.
Wondering if the first quoted line should be removed from the proposal (so still offer to return a [validated] Process instance for the parent, but not directly offer the parent process id).
If you want to lay out the tree. The most efficient way would be to call GetChildProcesses(true)
and then figure out the parent child relationship from the ParentProcessId
.
Without ParentProcessId
you'd need to recursively call GetChildProcesses(false)
.
The issue with ParentProcessId
is that between the time you retrieve the Id and when you use it, the Id may have been recycled. You can only rely on it if you control the lifetime of the parent process.
Given that I'm inclined to propose just GetParentProcessId() (and GetChildProcesses(bool))
The issue with ParentProcessId is that between the time you retrieve the Id and when you use it, the Id may have been recycled. You can only rely on it if you control the lifetime of the parent process.
Doesn't the Process
instance returned by GetParentProcessId()
have the same issue?
If you want to reconstitute the parent-child relationship from GetChildProcesses(includeAllDescendants: true)
return value, ParentProcessId
is useful.
I would like to add a voice to say: getting the parent process Id, even with the recycling issues on windows is much prefered to the current state of having to add a ton of special cases for unix/windows (not to mention that this introduces compile define or PInvoke complexities)
Even in the case where the PPID was recycled, that PPID might have been stored in a table somewhere for checking later and the user would still benefit from knowing the now-recycled PPID.
An example use case: My Process X spawns a process A.exe that then spawns processes B.exe (1), B.exe (2) and B.exe (3) and then closes itself. I can record the PID of my A instance and the start/end time. Later on, If I want to clean up, I can scan for Processes that have name B and PPID matching the recorded PID.
I know this sounds convoluted, but I'm dealing with this very scenario (Unity opens Python opens Unity). 🫠
Any user dealing with process trees in windows should be familiar with the PPID recycling issue.
An implementation of GetParentProcessId()
should return even an outdated Id.
However, I would agree with GetParentProcess()
returning null if the parent is no longer accessible. or its start time is after the child's. This would even be semantically correct, as the orphaned process would now be at the root of the process tree.
A result of this would be 2 methods:
GetParentProcess()
GetParentProcessId()
Users looking to get a hydrated Process object would likely select the GetParentProcess method. I don't think the API should concern itself on whether they would for some reason select to store the Id and get the process later. Especially since the user is already exposed to this issue through Process.ProcessId.
@danmoseley, where do you think this stands? Do you think it is ready for me to make cleanup tweaks to the proposal then go through API review?
Idea is to propose:
public Process GetParentProcess() { … }
public Process[] GetChildProcesses() { ... } // returns a best-effort attempt list of immediate child processes (same children as would be affected if Kill(entireProcessTree: false) were called instead of this method
Additional options, if there is interest, could be to also expose either or both of the following:
bool IsParentOf(Process process) { ... }
public Process[] GetChildProcesses(bool includeAllDescendants = false) { ... } // adds param to return entire descendant process tree when set to true...wondering if this capability is needed now; if not, can always be added later as the API signature change just involves adding a param with a default
@bgribaudo I think that would be ideal - if you agree you could edit the top post to make it the proposal.
I should say I am not on this team anymore and the area owners @dotnet/area-system-diagnostics-process would need to mark api-ready-for-review if they agree..
Thanks, @danmoseley. Proposal revised.
@dotnet/area-system-diagnostics-process, is this something that would be ready for review?
The odd thing about this is i can't find any external library that attemped to do it either. Is it such a niche concept to want to query the model of the process in an app?
Either way, I hope this moves forward into discussion at least.
i can't find any external library
PowerShell uses custom code to get ParentId for Get-Process cmdlet and we will be happy to use standard .Net API.
There is https://github.com/dotnet/runtime/issues/21941 with another proposal to get a property:
namespace System.Diagnostics
{
public class Process : Component
{
public Process ParentProcess { get; }
}
}
I wonder how a child process could get another parent one over time.
i can't find any external library
PowerShell uses custom code to get ParentId for Get-Process cmdlet and we will be happy to use standard .Net API.
Right you are! Thank you for that hint, I've looked at the code for the System.Management.Automation library for Powershell and the matching NuGet as a reference point/stopgap while this is being discussed.
For those interested, beyond the above mentioned NuGet, the code sits mostly in here - https://github.com/PowerShell/PowerShell/blob/3dc95ced87d38c347a9fc3a222eb4c52eaad4615/src/System.Management.Automation/engine/ProcessCodeMethods.cs
Not sure it qualifies as a compelling use case, but I landed on this thread when trying to figure out how to get the name of the parent process in a program called TestDummy:
Console.WriteLine($"Hello {parentHere}, this is {AppConfig.ProductName}");
Definitely not important enough for my purposes, to bother with any p-invoke solution to the problem, but I did go to the trouble of looking it up and reading this entire thread, so, if this feature ever gets added, I will use it.
A while ago I wrote up a little helper to provide this functionality for me on Windows.
For Linux you can just read /proc/TARGET_PID/status
then just look for "PPid:\t"
My rough implementation for windows can be found here SnapshotRelationsImpl.cs, it uses Vanara.PInvoke.Kernel32
for my own convenience.
Edit: A drawback for the Windows version is that it enumerates all processes. Though is still considerably faster than a WMI query. I don't know of a faster way to do this on Windows.
Issue
.Net Standard does not expose a means to identify the parent or child processes of a given process.
Rationale
When this request was originally proposed, the use case identified was for killing a tree of processes. That use case was subsequently satisfied by providing
Kill((bool entireProcessTree))
.Later feedback received on this issue suggests that there are separate use cases for exposing parent and/or child process details (see below comments).
Proposal
Why
Get*
methods instead of properties? A process's parent can change. Instead of caching this value and so potentially returning incorrect (out of date) values, it seems desirable to compute and return the then-correct value each time it is requested. Per the API design guidelines, methods are preferred over properties when "calling the member twice in succession results in different results."