Skip to content

Process.GetProcessById bugfix - #64723

Merged
jozkee merged 2 commits into
dotnet:mainfrom
epeshk:63937
Feb 24, 2022
Merged

Process.GetProcessById bugfix#64723
jozkee merged 2 commits into
dotnet:mainfrom
epeshk:63937

Conversation

@epeshk

@epeshk epeshk commented Feb 3, 2022

Copy link
Copy Markdown
Contributor

@ghost ghost added area-System.Diagnostics.Process community-contribution Indicates that the PR has been added by a community member labels Feb 3, 2022
@ghost

ghost commented Feb 3, 2022

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

Issue Details

#63937

Author: epeshk
Assignees: -
Labels:

area-System.Diagnostics.Process

Milestone: -

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stephentoub

Copy link
Copy Markdown
Member

Thanks for working on this. Can you please add a test that would have failed before and will now succeed?

@epeshk

epeshk commented Feb 15, 2022

Copy link
Copy Markdown
Contributor Author

I've added test for this

@jozkee
jozkee merged commit f97919d into dotnet:main Feb 24, 2022
@danmoseley

Copy link
Copy Markdown
Contributor

Thanks for the contribution @epeshk

@ghost ghost locked as resolved and limited conversation to collaborators Mar 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Diagnostics.Process community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants