Skip to content

Support long module path #57335

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
15 commits merged into from
Aug 15, 2021
Merged

Support long module path #57335

15 commits merged into from
Aug 15, 2021

Conversation

@adamsitnik
Copy link
Member

@adamsitnik adamsitnik commented Aug 13, 2021

It's #33505 that I would like to include in 6.0, please ignore the PR until all System.Diagnostics.Process tests are green.

@adamsitnik adamsitnik added this to the 6.0.0 milestone Aug 13, 2021
@ghost
Copy link

ghost commented Aug 13, 2021

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

Issue Details

It's #33505 that I would like to include in 6.0, please ignore the PR until all System.Diagnostics.Process tests are green.

Author: adamsitnik
Assignees: -
Labels:

area-System.Diagnostics.Process
Milestone: 6.0.0

stephentoub
stephentoub reviewed Aug 13, 2021
View reviewed changes
@adamsitnik adamsitnik requested a review from stephentoub August 13, 2021 13:04
@adamsitnik adamsitnik requested a review from stephentoub August 15, 2021 09:32
@ghost
Copy link

ghost commented Aug 15, 2021

Hello @adamsitnik!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 225acfe into dotnet:main Aug 15, 2021
pentp
pentp reviewed Aug 15, 2021
View reviewed changes
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs
while ((length = Interop.Kernel32.GetModuleBaseName(processHandle, moduleHandle, chars, chars.Length)) == chars.Length)
{
char[] toReturn = chars;
chars = ArrayPool<char>.Shared.Rent(length * 2);
Copy link
Contributor

@pentp pentp Aug 15, 2021

Choose a reason for hiding this comment

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

Why is it better to rent a new array first before returning the old one?

Copy link
Member

@stephentoub stephentoub Aug 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The order of renting and returning doesn't matter. But what does matter is the array being returned isn't referenced by the variable used for returning in the finally block. If it is, and the Return call throws after having stored the array, it could double return.

@adamsitnik adamsitnik mentioned this pull request Aug 15, 2021
Closed
@adamsitnik adamsitnik deleted the modulePathFix branch August 16, 2021 09:38
@adamsitnik adamsitnik mentioned this pull request Aug 16, 2021
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Sep 15, 2021
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoub stephentoub stephentoub approved these changes

+1 more reviewer

@pentp pentp pentp left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Diagnostics.Process

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@adamsitnik @pentp @stephentoub @nxtn