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 1 commit
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
Next Next commit
Make Get-ChildItem continue enumeration when encountering error on co… 
…ntained item (#2856)

Added try/catch within the enumeration loop to allow the enumeration to continue after encountering an error such as an item within the directory being deleted or renamed.

To assist in testing, two new internal test hooks have been added which cause Get-ChildItem to either delete or rename a file when encountered during enumeration. To facilitate this, the SetTestHook method has been modified to accept any type of value rather than only boolean.
  • Loading branch information
Jeff Bienstadt committed May 18, 2017
commit c65e618ce3dd0cc2a11a1a43dc087d7a4353d003
8 changes: 7 additions & 1 deletion src/System.Management.Automation/engine/Utils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
using System.Globalization;
using System.IO;
using System.Linq;
using System.Collections;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it really need?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, removed.

using System.Collections.ObjectModel;
using System.Collections.Generic;
using System.Collections.Concurrent;
Expand Down Expand Up @@ -1580,8 +1581,13 @@ public static class InternalTestHooks
// Simulate 'System.Diagnostics.Stopwatch.IsHighResolution is false' to test Get-Uptime throw
internal static bool StopwatchIsNotHighResolution;

// Name of a file to either delete or rename during directory enumeration.
internal static string GciEnumerationActionFilename = null;
// New name of the above file when renaming. Used only when GciEnumerationActionFilename is set.
internal static string GciEnumerationActionRename = null;

/// <summary>This member is used for internal test purposes.</summary>
public static void SetTestHook(string property, bool value)
public static void SetTestHook(string property, object value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can leave the parameter as bool and define two variables internal static bool GciEnumerationActionDelete = false; and internal static bool GciEnumerationActionRename = false;

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 feel like it would be better to not allow both to be set at the same time, which the string property accomplishes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I talked about two variables - you can enable hooks at different times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed. If both are set at the same time, only the delete will be done.

{
var fieldInfo = typeof(InternalTestHooks).GetField(property, BindingFlags.Static | BindingFlags.NonPublic);
if (fieldInfo != null)
Expand Down
99 changes: 64 additions & 35 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,80 @@ 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)
{
attributeFilter = evaluator.Evaluate(filesystemInfo.Attributes); // expressions
filterHidden = evaluator.ExistsInExpression(FileAttributes.Hidden);
}
if (null != switchEvaluator)
// Internal test code, run only if the
// "GciEnumerationActionFilename" test hook is set
Copy link
Collaborator

Choose a reason for hiding this comment

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

Usually we use single quota.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

var testActionFilename = InternalTestHooks.GciEnumerationActionFilename;
Copy link
Collaborator

Choose a reason for hiding this comment

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

For discussion - We never do that but it seems we should mask test hooks by #if Debug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iSazonov I think I need to revert this change. Wrapping that code in #if debug is causing the error condition to not occur, and thus the tests and CI to fail.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@daxian-dbw @lzybkr Could you please clarify should we leave test codes in Release build?

Copy link
Contributor

Choose a reason for hiding this comment

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

We do leave test hooks in all builds because we primarily test release builds./

Copy link
Member
@daxian-dbw daxian-dbw •
edited
Loading

Choose a reason for hiding this comment

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

I would love to have those test hooks readonly in the release build, so they cannot be messed around to change powershell behavior. Problem is that some tests won't be able to run in a release build, which is undesired.

if (filesystemInfo.Name == testActionFilename)
{
switchAttributeFilter = switchEvaluator.Evaluate(filesystemInfo.Attributes); // switch parameters
switchFilterHidden = switchEvaluator.ExistsInExpression(FileAttributes.Hidden);
var fullName = Path.Combine(directory.FullName, filesystemInfo.Name);
var newFilename = InternalTestHooks.GciEnumerationActionRename;
if (String.IsNullOrEmpty(newFilename))
{
File.Delete(fullName);
Copy link
Contributor

Choose a reason for hiding this comment

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

This test hook frightens me - please think through the security implications of this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can hard code the test file names and call InternalTestHooks.GciEnumerationAction to only enable/disable the hook.

Copy link
Member

Choose a reason for hiding this comment

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

Why not just use filesystemInfo.FullName as the 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.

That was a safety choice. What actually triggers the error is invoking filesystemInfo.Attributes further down in the code. I wanted to avoid invoking any properties that might involve lazy evaluation, to avoid having the error occur while running test-hook code.

If we can be sure that using filesystemInfo.FullName won't cause the file system to be hit I would be happy to use it instead.

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

Choose a reason for hiding this comment

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

I changed it to filesystemInfo.FullName and the test passed on windows and OSX. I didn't try on Linux. Are you seeing issue with filesystemInfo.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 haven't seen an issue, no. I was just trying to avoid one. I'll go ahead and make the change to FullName.

Copy link
Member

Choose a reason for hiding this comment

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

Just tried on Ubuntu 16.04, filesystemInfo.FullName also works fine 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

}
else
{
var newFullName = Path.Combine(directory.FullName, newFilename);
File.Move(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;
bool filterHidden = false; // "Hidden" is specified somewhere in the expression
bool switchFilterHidden = false; // "Hidden" is specified somewhere in the parameters
Copy link
Collaborator

Choose a reason for hiding this comment

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

The same about single quota.
And please put comments on separate lines before codes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed


if (null != evaluator)
{
WriteItemObject(
filesystemInfo.Name,
filesystemInfo.FullName,
false);
attributeFilter = evaluator.Evaluate(filesystemInfo.Attributes); // expressions
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove the comment or put on separate line before the code and expand it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

filterHidden = evaluator.ExistsInExpression(FileAttributes.Hidden);
}
else
if (null != switchEvaluator)
{
if (filesystemInfo is FileInfo)
WriteItemObject(filesystemInfo, filesystemInfo.FullName, false);
switchAttributeFilter = switchEvaluator.Evaluate(filesystemInfo.Attributes); // switch parameters
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove the comment or put on separate line before the code and expand it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

7802 switchFilterHidden = switchEvaluator.ExistsInExpression(FileAttributes.Hidden);
}

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

Choose a reason for hiding this comment

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

Please use full pattern:

if (...)
{
    ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed


// if "Hidden" is explicitly specified anywhere in the attribute filter, then override
Copy link
Collaborator

Choose a reason for hiding this comment

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

The same about single quota.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

// default hidden attribute filter.
// if specification is to return all containers, then do not do attribute filter on
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typos - begin with capital letters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

// 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
22 changes: 22 additions & 0 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 @@ -60,6 +60,28 @@ Describe "Get-ChildItem" -Tags "CI" {
$file.Count | Should be 1
$file.Name | Should be "pagefile.sys"
}
It "Should continue enumerating a directory when a contained item is deleted" -Skip:($IsWindows) {
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 skip Windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the error condition doesn't appear on Windows. Unlike Unix, deleting or renaming a file does not have an adverse effect on the enumeration---the file is listed in its original form.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So the test is ok on Windows too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. On Windows the test will fail because the enumeration will succeed with no errors emitted in the middle of the process, and $Error.Count will be zero.

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

Choose a reason for hiding this comment

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

We need to change those tests so that they work the same on all platforms.
In other words these tests now check the test hook not Get-ChildItem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the underlying behavior is not the same on all platforms. On Unix, if a file is deleted or renamed during enumeration over the result of DirectoryInfo.EnumerateFiles()an exception is thrown when attempting to access a property on the FileSystemInfo object. On Windows, no exception is thrown, the property is successfully accessed, and the enumeration goes happily on.

The test hook code does not force an error. It sets up the condition which may or may not trigger an error. If the same condition triggers an error on one platform but not on another, how are the tests for how the error is handled expected to behave the same on both platforms?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I think I just realized what you meant. You want to change the test so that if ($IsWindows) the error count is zero, otherwise it is one. Is that right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Original Issue:

Expected
Get-ChildItem should skip inaccessible paths (with error) and continue enumerating files and paths that are accessible.

Our tests must be simple as that expected: dir without throw return expected list (different on different platforms).

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 hope this is what you're looking for. On Unix the tests check that the error was emitted and that the deleted/renamed item was left out of the list. On Windows the tests check that no error was emitted and that the list is complete.

I have also removed the #if DEBUG to allow the CI checks to succeed. I would rather the hook code be in debug-only, but the CI and Start-PSBuildbuilds seem to be release builds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's a version in which the hook now only allow the test script to select what actions to take, but the hook code internally is hard-coded to use specific file names.

$Error.Clear()
[System.Management.Automation.Internal.InternalTestHooks]::SetTestHook("GciEnumerationActionFilename", "c")
$result = Get-ChildItem -Path $TestDrive -ErrorAction SilentlyContinue
[System.Management.Automation.Internal.InternalTestHooks]::SetTestHook("GciEnumerationActionFilename", $null)
$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" -Skip:($IsWindows) {
$Error.Clear()
[System.Management.Automation.Internal.InternalTestHooks]::SetTestHook("GciEnumerationActionFilename", "B")
[System.Management.Automation.Internal.InternalTestHooks]::SetTestHook("GciEnumerationActionRename", "Z")
$result = Get-ChildItem -Path $TestDrive -ErrorAction SilentlyContinue
[System.Management.Automation.Internal.InternalTestHooks]::SetTestHook("GciEnumerationActionRename", $null)
[System.Management.Automation.Internal.InternalTestHooks]::SetTestHook("GciEnumerationActionFilename", $null)
$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