Change Get-FileHash to close file handles before writing output #12474

HumanEquivalentUnit · 2020-04-23T23:58:43Z

PR Summary

Fix #12473 where Get-FileHash holds the hashed file handles open unnecessarily long.
Change is to hash the file contents, then dispose the file stream, then write the results, in that order.

PR Context

Get-ChildItem | Get-FileHash | Rename-Item -NewName {$_.Hash} throws because the file is still open when trying to be renamed, this PR tries to fix that by closing the file handle before Get-FileHash writes output.

PR Checklist

Fix the use case `Get-FileHash | Rename-Item` by changing to `using () {}` and closing the hashed file before writing the output result.

msftclas · 2020-04-23T23:58:56Z

All CLA requirements met.

vexx32

Looks good to me! Just a couple questions on the test. 😊

test/powershell/Modules/Microsoft.PowerShell.Utility/Get-FileHash.Tests.ps1

Copy the test document first, then remove the copy after, so it won't affect other tests. Assumes the test drive is writable.

vexx32

Looks good, nice work! 😊

vexx32 · 2020-04-24T02:54:14Z

@PoshChan please retry macos

PoshChan · 2020-04-24T02:55:08Z

@vexx32, successfully started retry of PowerShell-CI-macOS

HumanEquivalentUnit · 2020-04-24T03:13:27Z

NB. The failing PowerShell-CI-static-analysis test is about URLs in Markdown in the PowerShell 6 CHANGELOG, and is unrelated to these changes.

src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetHash.cs

Add more documentation comments. Remove Algorithm argument from ComputeFileHash method.

Add param descriptions.

src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetHash.cs

Move bytehash var into it. Update how it returns success/failure state.

src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetHash.cs

iSazonov · 2020-04-24T06:05:04Z

@HumanEquivalentUnit Please rebase to get a fix for CI-static.

Remove hashCompletedSuccessfully variable and replace with a return value calculated from hash variable.

Remove blank line before closing brace.

src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetHash.cs

test/powershell/Modules/Microsoft.PowerShell.Utility/Get-FileHash.Tests.ps1

Change the test logic to try the rename, and then Test-Path the expected new name. Make the context and description clearer.

HumanEquivalentUnit · 2020-04-24T07:06:19Z

@HumanEquivalentUnit Please rebase to get a fix for CI-static.

OK, I followed https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/syncing-a-fork and think it has merged the updated markdown files. Fingers crossed.

Syncing a fork - GitHub Help
Sync a fork of a repository to keep it up-to-date with the upstream repository.

HumanEquivalentUnit · 2020-04-24T07:58:40Z

PowerShell-CI-macos check failure is a traceroute one, and not related to this PR; could one of you re-run it? Not sure I can.

@PoshChan please retry macos

test/powershell/Modules/Microsoft.PowerShell.Utility/Get-FileHash.Tests.ps1

Co-Authored-By: Ilya <darpa@yandex.ru>

vexx32 · 2020-04-24T17:51:50Z

@PoshChan please retry macos

PoshChan · 2020-04-24T17:52:16Z

@vexx32, successfully started retry of PowerShell-CI-macOS

HumanEquivalentUnit · 2020-04-30T16:34:20Z

Thank you for reviewing and improving it, @iSazonov !

ghost · 2020-05-19T18:09:42Z

🎉v7.1.0-preview.3 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

ghost · 2020-06-11T21:02:37Z

🎉v7.0.2 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

…rShell#12474) # Conflicts: # src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetHash.cs

HumanEquivalentUnit added 2 commits April 24, 2020 00:42

Add test for not holding the file open.

4cbc200

Close file before writing output to pipeline.

63b2960

Fix the use case `Get-FileHash | Rename-Item` by changing to `using () {}` and closing the hashed file before writing the output result.

ghost assigned iSazonov Apr 23, 2020

vexx32 reviewed Apr 24, 2020

View reviewed changes

test/powershell/Modules/Microsoft.PowerShell.Utility/Get-FileHash.Tests.ps1 Outdated Show resolved Hide resolved

HumanEquivalentUnit added 2 commits April 24, 2020 03:01

Update close filehandle test

48ca8d9

Copy the test document first, then remove the copy after, so it won't affect other tests. Assumes the test drive is writable.

Tidy up comment

1430510

vexx32 approved these changes Apr 24, 2020

View reviewed changes

HumanEquivalentUnit marked this pull request as ready for review April 24, 2020 03:10

iSazonov reviewed Apr 24, 2020

View reviewed changes

src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetHash.cs Outdated Show resolved Hide resolved

iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Apr 24, 2020

iSazonov added this to the 7.0.x-Servicing-Consider milestone Apr 24, 2020

HumanEquivalentUnit added 3 commits April 24, 2020 06:26

Move work into dedicated ComputeFileHash method

b6e6fae

Fix CodeFactor reports.

fda9a2c

Add more documentation comments. Remove Algorithm argument from ComputeFileHash method.

Fix CodeFactor reports.

767447e

Add param descriptions.

iSazonov reviewed Apr 24, 2020

View reviewed changes

src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetHash.cs Outdated Show resolved Hide resolved

src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetHash.cs Outdated Show resolved Hide resolved

Fix ComputeFileHash method bugs

a0cfdd4

Move bytehash var into it. Update how it returns success/failure state.

HumanEquivalentUnit marked this pull request as draft April 24, 2020 06:00

iSazonov reviewed Apr 24, 2020

View reviewed changes

src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetHash.cs Outdated Show resolved Hide resolved

HumanEquivalentUnit added 2 commits April 24, 2020 07:09

Simplify ComputeHash return value

445928f

Remove hashCompletedSuccessfully variable and replace with a return value calculated from hash variable.

Fix CodeFactor report.

176de14

Remove blank line before closing brace.

iSazonov reviewed Apr 24, 2020

View reviewed changes

src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetHash.cs Show resolved Hide resolved

iSazonov reviewed Apr 24, 2020

View reviewed changes

test/powershell/Modules/Microsoft.PowerShell.Utility/Get-FileHash.Tests.ps1 Outdated Show resolved Hide resolved

HumanEquivalentUnit added 3 commits April 24, 2020 07:29

Inline the hash variable declaration.

3bfa812

Improve the file handle being held open tests

e93eba0

Change the test logic to try the rename, and then Test-Path the expected new name. Make the context and description clearer.

Merge remote-tracking branch 'upstream/master'

94bd3d7

Fix bug Test-Path on the wrong path

bce6acd

HumanEquivalentUnit marked this pull request as ready for review April 24, 2020 07:54

iSazonov reviewed Apr 24, 2020

View reviewed changes

test/powershell/Modules/Microsoft.PowerShell.Utility/Get-FileHash.Tests.ps1 Outdated Show resolved Hide resolved

HumanEquivalentUnit and others added 2 commits April 24, 2020 18:15

Remove SilentlyContinue from file close test

838afb9

Co-Authored-By: Ilya <darpa@yandex.ru>

Update path test to use Should -Exist

212c19d

vexx32 approved these changes Apr 24, 2020

View reviewed changes

iSazonov approved these changes Apr 24, 2020

View reviewed changes

iSazonov requested review from SteveL-MSFT and daxian-dbw April 24, 2020 18:05

iSazonov changed the title ~~Change Get-FileHash to close file handles before writing output - fixes #12473~~ Change Get-FileHash to close file handles before writing output Apr 30, 2020

iSazonov merged commit 9e3b340 into PowerShell:master Apr 30, 2020

ghost mentioned this pull request May 19, 2020

Get-FileHash holds the file open #12473

Closed

TravisEz13 modified the milestones: 7.0.x-Servicing-Consider, 7.0.2 Jun 8, 2020

silijon pushed a commit to SkyKick/PowerShell that referenced this pull request Jul 2, 2020

Change Get-FileHash to close file handles before writing output (Powe…

359cdee

…rShell#12474) # Conflicts: # src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetHash.cs

TravisEz13 mentioned this pull request Oct 12, 2022

Back-port to 7.0: Update list of PowerShell team members in release tools #18266

Merged

22 tasks

Change Get-FileHash to close file handles before writing output #12474

Change Get-FileHash to close file handles before writing output #12474

Uh oh!

Conversation

HumanEquivalentUnit commented Apr 23, 2020

PR Summary

PR Context

PR Checklist

Uh oh!

msftclas commented Apr 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vexx32 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vexx32 left a comment

Choose a reason for hiding this comment

Uh oh!

vexx32 commented Apr 24, 2020

Uh oh!

PoshChan commented Apr 24, 2020

Uh oh!

HumanEquivalentUnit commented Apr 24, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iSazonov commented Apr 24, 2020

Uh oh!

Uh oh!

Uh oh!

HumanEquivalentUnit commented Apr 24, 2020 • edited by unfurl-links bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HumanEquivalentUnit commented Apr 24, 2020

Uh oh!

Uh oh!

vexx32 commented Apr 24, 2020

Uh oh!

PoshChan commented Apr 24, 2020

Uh oh!

HumanEquivalentUnit commented Apr 30, 2020

Uh oh!

ghost commented May 19, 2020

Uh oh!

ghost commented Jun 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

msftclas commented Apr 23, 2020 •

edited

Loading

HumanEquivalentUnit commented Apr 24, 2020 •

edited by unfurl-links bot

Loading