Skip to content

Replace ProcessModule.FileName with Environment.ProcessPath and remove PSUtils.GetMainModule #15012

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.

Sign up for GitHub

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

Jump to bottom
Merged
adityapatwardhan merged 3 commits into from
Jun 3, 2021
Merged

Replace ProcessModule.FileName with Environment.ProcessPath and remove PSUtils.GetMainModule #15012

adityapatwardhan merged 3 commits into from
Jun 3, 2021

Conversation

@Fs00
Copy link
Contributor

@Fs00 Fs00 commented Mar 13, 2021 •
edited
Loading

PR Summary

This PR replaces all occurrences of PsUtils.GetMainModule(currentProcess).FileName with the new Environment.ProcessPath API as suggested in #14332 (comment).
Furthermore, all the other places in which a ProcessModule needs to be retrieved have been changed to use Process.MainModule .NET API directly instead of going through PSUtils.GetMainModule (which has been removed).

PR Context

In the comment linked above, @iSazonov reported that calling PsUtils.GetMainModule(currentProcess).FileName was particularly expensive (27ms). The new Environment.ProcessPath API has been implemented to tackle this problem and there is even an official .NET analyzer to recommend switching to it (dotnet/roslyn-analyzers#4909).
While making that change, I noticed that PSUtils.GetMainModule wrapped Process.MainModule in a retry loop to workaround a .NET Framework 2.0 (!) BCL issue, therefore it shouldn't be needed at all nowadays.

Fixes #13790

PR Checklist

Fs00
Fs00 commented Mar 13, 2021
View reviewed changes
src/System.Management.Automation/security/SecuritySupport.cs
hostname = string.Concat("PowerShell_", processModule.FileName, "_",
processModule.FileVersionInfo.ProductVersion);
hostname = string.Concat("PowerShell_", Environment.ProcessPath, "_",
currentProcess.MainModule.FileVersionInfo.ProductVersion);
Copy link
Contributor Author

@Fs00 Fs00 Mar 13, 2021

Choose a reason for hiding this comment

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

In a further PR I will remove the need to use Process.MainModule.FileVersionInfo here to avoid loading System.Diagnostics.FileVersionInfo.dll assembly on startup on Windows.

@iSazonov iSazonov added the CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log label Mar 13, 2021
iSazonov
iSazonov approved these changes Mar 13, 2021
View reviewed changes
@Fs00 @iSazonov
Fix potential NRE in Get-Process cmdlet
8d6af64 
Co-authored-by: Ilya <darpa@yandex.ru>
TravisEz13
TravisEz13 reviewed Mar 15, 2021
View reviewed changes
Copy link
Member

@TravisEz13 TravisEz13 left a comment

Choose a reason for hiding this comment

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

I looks okay to me.

@ghost ghost added the Review - Needed The PR is being reviewed label Mar 23, 2021
@ghost
Copy link

ghost commented Mar 23, 2021

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@Fs00
Copy link
Contributor Author

Fs00 commented Mar 30, 2021

I see that this PR has been already approved, is anything else needed to get this merged?

@TravisEz13
Copy link
Member

TravisEz13 commented Mar 31, 2021

We should have approval from one of the code owners either @adityapatwardhan or @daxian-dbw

@ghost ghost removed the Review - Needed The PR is being reviewed label Mar 31, 2021
@TravisEz13 TravisEz13 added the WG-Engine core PowerShell engine, interpreter, and runtime label Mar 31, 2021
@ghost ghost added the Review - Needed The PR is being reviewed label Apr 8, 2021
@ghost
Copy link

ghost commented Apr 8, 2021

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@adityapatwardhan adityapatwardhan merged commit 088eb6d into PowerShell:master Jun 3, 2021
@ghost ghost removed the Review - Needed The PR is being reviewed label Jun 3, 2021
@adityapatwardhan adityapatwardhan added this to the 7.2.0-preview.7 milestone Jun 3, 2021
@adityapatwardhan
Copy link
Member

adityapatwardhan commented Jun 3, 2021

@Fs00 Thank you for your contribution!

@Fs00
Copy link
Contributor Author

Fs00 commented Jun 3, 2021

You're welcome!👍🏻

@Fs00 Fs00 deleted the env-processpath branch June 3, 2021 20:24
@ghost
Copy link

ghost commented Jun 17, 2021

🎉v7.2.0-preview.7 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request Jun 17, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov iSazonov iSazonov approved these changes

@TravisEz13 TravisEz13 TravisEz13 approved these changes

@adityapatwardhan adityapatwardhan adityapatwardhan approved these changes

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw daxian-dbw is a code owner

@PaulHigin PaulHigin Awaiting requested review from PaulHigin

Assignees

@adityapatwardhan adityapatwardhan

Labels

CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log WG-Engine core PowerShell engine, interpreter, and runtime

Projects

None yet

Milestone

7.2.0-preview.7

Development

Successfully merging this pull request may close these issues.

Use Environment.ProcessPath API

4 participants

@Fs00 @TravisEz13 @adityapatwardhan @iSazonov