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

Expose Parent/Child Process Information via System.Diagnostics.Process #24423

Open
bgribaudo opened this issue Dec 12, 2017 · 41 comments
Open

Expose Parent/Child Process Information via System.Diagnostics.Process #24423

bgribaudo opened this issue Dec 12, 2017 · 41 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Diagnostics.Process
Milestone

Comments

@bgribaudo
Copy link
Contributor

bgribaudo commented Dec 12, 2017

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

public partial class Process : ....    {
  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 ideas. If there is interest, could also expose either or both of the following:
  public bool IsParentOf(Process process) { ... }
  public Process[] GetChildProcesses(bool includeAllDescendants = false) { ... } // adds param to return entire descendant pr    }

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."

@danmoseley
Copy link
Member

danmoseley commented Dec 12, 2017

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

@danmoseley
Copy link
Member

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"

@bgribaudo
Copy link
Contributor Author

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?

@danmoseley
Copy link
Member

danmoseley commented Jan 4, 2018

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.

@bgribaudo
Copy link
Contributor Author

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.

@natemcmaster
Copy link
Contributor

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.

@danmoseley
Copy link
Member

@joperezr perhaps you could guide this and/or https://github.com/dotnet/corefx/issues/26234 to api-ready-for-review if appropriate?

@bgribaudo
Copy link
Contributor Author

Hi @joperezr (or @danmosemsft)! Is there anything I could do to help further this along?

@iSazonov
Copy link
Contributor

Should

    public Process[] GetChildProcesses(bool includeAllDescendants = false) 

be

    public IEnumerable<Process> GetChildProcesses(bool includeAllDescendants = false) 

?

@terrajobst
Copy link
Member

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?

@danmoseley
Copy link
Member

@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.

@krwq
Copy link
Member

krwq commented Jun 26, 2018

  • Kill which allows to kill the subtree - IMO it is a good idea.
  • Children - I do not have anything against but I think we should understand scenarios first so that we do not create an API which will be used only in one/two specific context (if that's the case I'd rather only have those two APIs)
  • For exposing parent I do not think it is a good idea - parent should control and know the sub-processes but not necessarily the other way around (parent process can always give its pid as an argument when starting child process) - I'm not convinced we should have it - especially after the pitfall @danmosemsft mentioned.

@bgribaudo
Copy link
Contributor Author

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?

@danmoseley
Copy link
Member

@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

@bgribaudo
Copy link
Contributor Author

bgribaudo commented Jun 27, 2018

@danmosemsft, thanks! Would you like me to do that on this issue or over on dotnet/corefx#26234?

@danmoseley
Copy link
Member

@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.

@bgribaudo
Copy link
Contributor Author

Thanks, @danmosemsft!

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@atruskie
Copy link

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.

@phxnsharp
Copy link

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.

@danmoseley
Copy link
Member

I guess the status of this is -- needs clear scenarios?

dotnet/diagnostics#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.

@phxnsharp
Copy link

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.

@danmoseley
Copy link
Member

@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.

@phxnsharp
Copy link

@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:

  • Add GetParentProcess() and GetChildProcesses()
  • On Windows:
    • Make GetParentProcess() validate the start time such that you don't return an invalid parent process
    • Make the constructor take an ID and a Start Time, and validate that the start time hasn't changed when you later instantiate the handle.

Thank you!

@danmoseley
Copy link
Member

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).

@phxnsharp
Copy link

Sounds fine for our purpose. Thanks.

@tmds
Copy link
Member

tmds commented May 2, 2022

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

@bgribaudo
Copy link
Contributor Author

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

@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.

@bgribaudo
Copy link
Contributor Author

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).

@tmds
Copy link
Member

tmds commented May 2, 2022

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).

@danmoseley
Copy link
Member

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))

@tmds
Copy link
Member

tmds commented May 10, 2022

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.

@MHDante
Copy link

MHDante commented Sep 30, 2022

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.

@bgribaudo
Copy link
Contributor Author

@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

@danmoseley
Copy link
Member

@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..

@bgribaudo
Copy link
Contributor Author

Thanks, @danmoseley. Proposal revised.

@dotnet/area-system-diagnostics-process, is this something that would be ready for review?

@ransagy
Copy link

ransagy commented Dec 4, 2022

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.

@iSazonov
Copy link
Contributor

iSazonov commented Dec 5, 2022

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.

@iSazonov
Copy link
Contributor

iSazonov commented Dec 5, 2022

There is #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.

@ransagy
Copy link

ransagy commented Dec 5, 2022

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

@jwdonahue
Copy link

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.

@JustArion
Copy link

JustArion commented Jun 27, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Diagnostics.Process
Projects
None yet
Development

No branches or pull requests