- 
                Notifications
    You must be signed in to change notification settings 
- Fork 5.2k
Process.GetProcessById bugfix #64723
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
Conversation
| Tagging subscribers to this area: @dotnet/area-system-diagnostics-process Issue Details
 | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamsitnik, do you know why we fall back to the IndexOf here for the !IsRemoteMachine case?  I'm wondering why the using block below can't just contain return !processHandle.IsInvalid && !HasExited(processHandle, ref signaled, out _);.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to continue working on this code to get rid of this fallback in most of cases. This fallback needed because it's not always possible to open process handle, e.g. corner cases is Idle process (pid = 0) and csrss processes. We probably should fall back only if processId == 0 or if error code of OpenProcess is ERROR_ACCESS_DENIED but it may be a controversial topic, so I decide to implement it in another PR, separated from bugfix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Thanks for working on this. Can you please add a test that would have failed before and will now succeed? | 
| I've added test for this | 
| Thanks for the contribution @epeshk | 
#63937