8000 Make Get-ChildItem continue enumeration when encountering error on contained item (#2856) by jeffbi · Pull Request #3806 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]
Skip to content

Make Get-ChildItem continue enumeration when encountering error on contained item (#2856) #3806

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

Merged
merged 8 commits into from
May 26, 2017
Merged

Make Get-ChildItem continue enumeration when encountering error on contained item (#2856) #3806

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c65e618
Make Get-ChildItem continue enumeration when encountering error on co…
May 17, 2017
cd57775
Changes per code review.
May 18, 2017
170f901
More changes per code review.
May 19, 2017
43339c0
More changes per code review.
May 19, 2017
2e97d00
Changes per code review.
May 21, 2017
c7e0b56
Add "| Should Be" to existing test.
May 22, 2017
fe7dd4b
Changes per code review.
May 25, 2017
ba536e4
Change per code review.
May 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/System.Management.Automation/engine/Utils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1580,6 +1580,11 @@ public static class InternalTestHooks
// Simulate 'System.Diagnostics.Stopwatch.IsHighResolution is false' to test Get-Uptime throw
internal static bool StopwatchIsNotHighResolution;

// Used in the FileSystemProvider to simulate deleting a file during enumeration in Get-ChildItem
internal static bool GciEnumerationActionDelete = false;
// Used in the FileSystemProvider to simulate renaming a file during enumeration in Get-ChildItem
internal static bool GciEnumerationActionRename = false;

/// < 10000 ;summary>This member is used for internal test purposes.</summary>
public static void SetTestHook(string property, bool value)
{
Expand Down
100 changes: 67 additions & 33 deletions src/System.Management.Automation/namespaces/FileSystemProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1659,51 +1659,85 @@ private void Dir(
return;
}

bool attributeFilter = true;
bool switchAttributeFilter = true;
bool filterHidden = false; // "Hidden" is specified somewhere in the expression
bool switchFilterHidden = false; // "Hidden" is specified somewhere in the parameters

if (null != evaluator)
// Internal test code, run only if one of the
// 'GciEnumerationAction' test hooks are set.
if (InternalTestHooks.GciEnumerationActionDelete)
Copy link
Member

Choose a reason for hiding this comment

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

Checking these 2 testing flags in the loop seems too expensive to me. How about do the testing trick before the foreach loop? Like:

var fullName = Path.Combine(directory.FullName, "c283d143-2116-4809-bf11-4f7d61613f92")
if (File.Exist(fullName))  // <--- even better, use our internal `ItemExist` method which calls native API
{
      File.Delete(fullName);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm concerned about hoisting this out of the loop. The point is to have a file go away while the enumeration is in progress. If we do the test outside the loop I don't think we've met the requirement---we've acquired the enumerator but have not yet started the enumeration. That's why we've chosen files that will not be the first in the enumeration.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems we can put this after IEnumerable<FileSystemInfo> sortedChildList = childList.OrderBy(c => c.Name, StringComparer.CurrentCultureIgnoreCase);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure that solves the problem. Again, we have an enumerable object but enumeration doesn't start until the foreach.

Copy link
Collaborator
@iSazonov iSazonov May 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think we get already the list in foreach (IEnumerable<FileSystemInfo> childList in target) - so after that 'IEnumerable sortedChildList = childList.OrderBy(c => c.Name, StringComparer.CurrentCultureIgnoreCase);' we can safely delete/remove test file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

target is just a List of IEnumerable objects, containing only 1 or 2 entries---an IEnumerable for directory names and an IEnumerable for file names. I don't think the act of getting a reference to an enumerable object from a list causes enumeration to begin.

Copy link
Member
@daxian-dbw daxian-dbw May 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Again, we have an enumerable object but enumeration doesn't start until the foreach.

@jeffbi This is a very good point. Let's keep it as is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I catched an enumaration issue second time in last week - need to go to school. :-)

The last question is whether to make a test for such normal runtime error?

{
attributeFilter = evaluator.Evaluate(filesystemInfo.Attributes); // expressions
filterHidden = evaluator.ExistsInExpression(FileAttributes.Hidden);
if (string.Equals(filesystemInfo.Name, "c283d143-2116-4809-bf11-4f7d61613f92", StringComparison.InvariantCulture))
{
File.Delete(filesystemInfo.FullName);
}
}
if (null != switchEvaluator)
else if (InternalTestHooks.GciEnumerationActionRename)
{
switchAttributeFilter = switchEvaluator.Evaluate(filesystemInfo.Attributes); // switch parameters
switchFilterHidden = switchEvaluator.ExistsInExpression(FileAttributes.Hidden);
if (string.Equals(filesystemInfo.Name, "B1B691A9-B7B1-4584-AED7-5259511BEEC4", StringComparison.InvariantCulture))
{
var newFullName = Path.Combine(directory.FullName, "77efd2bb-92aa-4ad3-979a-18936a4bd565");
File.Move(filesystemInfo.FullName, newFullName);
}
}

bool hidden = false;
if (!Force) hidden = (filesystemInfo.Attributes & FileAttributes.Hidden) != 0;

// if "Hidden" is explicitly specified anywhere in the attribute filter, then override
// default hidden attribute filter.
// if specification is to return all containers, then do not do attribute filter on
// the containers.
bool attributeSatisfy =
((attributeFilter && switchAttributeFilter) ||
((returnContainers == ReturnContainers.ReturnAllContainers) &&
((filesystemInfo.Attributes & FileAttributes.Directory) != 0)));

if (attributeSatisfy && (filterHidden || switchFilterHidden || Force || !hidden))
try
{
if (nameOnly)
bool attributeFilter = true;
bool switchAttributeFilter = true;
// 'Hidden' is specified somewhere in the expression
bool filterHidden = false;
// 'Hidden' is specified somewhere in the parameters
bool switchFilterHidden = false;

if (null != evaluator)
{
WriteItemObject(
filesystemInfo.Name,
filesystemInfo.FullName,
false);
attributeFilter = evaluator.Evaluate(filesystemInfo.Attributes);
filterHidden = evaluator.ExistsInExpression(FileAttributes.Hidden);
}
else
if (null != switchEvaluator)
{
switchAttributeFilter = switchEvaluator.Evaluate(filesystemInfo.Attributes);
switchFilterHidden = switchEvaluator.ExistsInExpression(FileAttributes.Hidden);
}

bool hidden = false;
if (!Force)
{
if (filesystemInfo is FileInfo)
WriteItemObject(filesystemInfo, filesystemInfo.FullName, false);
hidden = (filesystemInfo.Attributes & FileAttributes.Hidden) != 0;
}

// If 'Hidden' is explicitly specified anywhere in the attribute filter, then override
// default hidden attribute filter.
// If specification is to return all containers, then do not do attribute filter on
// the containers.
bool attributeSatisfy =
((attributeFilter && switchAttributeFilter) ||
((returnContainers == ReturnContainers.ReturnAllContainers) &&
((filesystemInfo.Attributes & FileAttributes.Directory) != 0)));

if (attributeSatisfy && (filterHidden || switchFilterHidden || Force || !hidden))
{
if (nameOnly)
{
WriteItemObject(
filesystemInfo.Name,
filesystemInfo.FullName,
false);
}
else
WriteItemObject(filesystemInfo, filesystemInfo.FullName, true);
{
if (filesystemInfo is FileInfo)
WriteItemObject(filesystemInfo, filesystemInfo.FullName, false);
else
WriteItemObject(filesystemInfo, filesystemInfo.FullName, true);
}
}
}
catch (System.IO.FileNotFoundException ex)
{
WriteError(new ErrorRecord(ex, "DirIOError", ErrorCategory.ReadError, directory.FullName));
}
catch (UnauthorizedAccessException ex)
{
WriteError(new ErrorRecord(ex, "DirUnauthorizedAccessError", ErrorCategory.PermissionDenied, directory.FullName));
}
}// foreach
}// foreach

Expand Down
68 changes: 55 additions & 13 deletions test/powershell/Modules/Microsoft.PowerShell.Management/Get-ChildItem.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,18 @@ Describe "Get-ChildItem" -Tags "CI" {

BeforeAll {
# Create Test data
$null = New-Item -Path $TestDrive -Name "a" -ItemType "File" -Force
$null = New-Item -Path $TestDrive -Name "B" -ItemType "File" -Force
$null = New-Item -Path $TestDrive -Name "c" -ItemType "File" -Force
$null = New-Item -Path $TestDrive -Name "D" -ItemType "File" -Force
$null = New-Item -Path $TestDrive -Name "E" -ItemType "Directory" -Force
$null = New-Item -Path $TestDrive -Name ".F" -ItemType "File" -Force | %{$_.Attributes = "hidden"}
$item_a = "a3fe710a-31af-4834-bc29-d0b584589838"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why we use lower case for "_a" and below for "_c"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original filenames were a, B, c, D, E, and .F, in that letter casing, which appeared to have been a deliberate choice. To avoid disrupting any existing tests, I elected to keep the same letter casing in the GUID filenames.

It does look like I made a copy/paste error when I was generating GUID-based filenames. The filename for $Item_E should have begun with the letter/hex digit E. It now does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I used lower-case in the variable names was to call out that casing was significant.

$item_B = "B1B691A9-B7B1-4584-AED7-5259511BEEC4"
$item_c = "c283d143-2116-4809-bf11-4f7d61613f92"
$item_D = "D39B4FD9-3E1D-4DD5-8718-22FE2C934CE3"
$item_E = "EE150FEB-0F21-4AFF-8066-AF59E925810C"
$item_F = ".F81D8514-8862-4227-B041-0529B1656A43"
$null = New-Item -Path $TestDrive -Name $item_a -ItemType "File" -Force
$null = New-Item -Path $TestDrive -Name $item_B -ItemType "File" -Force
$null = New-Item -Path $TestDrive -Name $item_c -ItemType "File" -Force
$null = New-Item -Path $TestDrive -Name $item_D -ItemType "File" -Force
$null = New-Item -Path $TestDrive -Name $item_E -ItemType "Directory" -Force
$null = New-Item -Path $TestDrive -Name $item_F -ItemType "File" -Force | %{$_.Attributes = "hidden"}
}

It "Should list the contents of the current folder" {
Expand All @@ -34,32 +40,68 @@ Describe "Get-ChildItem" -Tags "CI" {

It "Should list files in sorted order" {
$files = Get-ChildItem -Path $TestDrive
$files[0].Name | Should Be "E"
$files[1].Name | Should Be "a"
$files[2].Name | Should Be "B"
$files[3].Name | Should Be "c"
$files[4].Name | Should Be "D"
$files[0].Name | Should Be $item_E
$files[1].Name | Should Be $item_a
$files[2].Name | Should Be $item_B
$files[3].Name | Should Be $item_c
$files[4].Name | Should Be $item_D
}

It "Should list hidden files as well when 'Force' parameter is used" {
$files = Get-ChildItem -path $TestDrive -Force
$files | Should not be $null
$files.Count | Should be 6
$files.Name.Contains(".F")
$files.Name.Contains($item_F) | Should Be $true
}

It "Should list only hidden files when 'Hidden' parameter is used" {
$files = Get-ChildItem -path $TestDrive -Hidden
$files | Should not be $null
$files.Count | Should be 1
$files[0].Name | Should Be ".F"
$files[0].Name | Should Be $item_F
}
It "Should give .sys file if the fullpath is specified with hidden and force parameter" -Skip:(!$IsWindows){
$file = Get-ChildItem -path "$env:SystemDrive\\pagefile.sys" -Hidden
$file | Should not be $null
$file.Count | Should be 1
$file.Name | Should be "pagefile.sys"
}
It "Should continue enumerating a directory when a contained item is deleted" {
$Error.Clear()
[System.Management.Automation.Internal.InternalTestHooks]::SetTestHook("GciEnumerationActionDelete", $true)
$result = Get-ChildItem -Path $TestDrive -ErrorAction SilentlyContinue
[System.Management.Automation.Internal.InternalTestHooks]::SetTestHook("GciEnumerationActionDelete", $false)
if ($IsWindows)
{
$Error.Count | Should BeExactly 0
$result.Count | Should BeExactly 5
}
else
{
$Error.Count | Should BeExactly 1
$Error[0].FullyQualifiedErrorId | Should BeExactly "DirIOError,Microsoft.PowerShell.Commands.GetChildItemCommand"
$Error[0].Exception | Should BeOfType System.Io.FileNotFoundException
$result.Count | Should BeExactly 4
}
}
It "Should continue enumerating a directory when a contained item is renamed" {
$Error.Clear()
[System.Management.Automation.Internal.InternalTestHooks]::SetTestHook("GciEnumerationActionRename", $true)
$result = Get-ChildItem -Path $TestDrive -ErrorAction SilentlyContinue
[System.Management.Automation.Internal.InternalTestHooks]::SetTestHook("GciEnumerationActionRename", $false)
if ($IsWindows)
{
$Error.Count | Should BeExactly 0
$result.Count | Should BeExactly 4
}
else
{
$Error.Count | Should BeExactly 1
$Error[0].FullyQualifiedErrorId | Should BeExactly "DirIOError,Microsoft.PowerShell.Commands.GetChildItemCommand"
$Error[0].Exception | Should BeOfType System.Io.FileNotFoundException
$result.Count | Should BeExactly 3
}
}
}

Context 'Env: Provider' {
Expand Down
0