Make Get-ChildItem continue enumeration when encountering error on contained item (#2856) by jeffbi · Pull Request #3806 · PowerShell/PowerShell

jeffbi · 2017-05-18T04:23:53Z

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 specific file (file name hard-coded) when encountered during enumeration.

…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.

iSazonov · 2017-05-18T11:09:31Z

@jeffbi I added link on Issue

iSazonov · 2017-05-18T11:10:50Z

src/System.Management.Automation/engine/Utils.cs

 using System.Globalization;
 using System.IO;
 using System.Linq;
+using System.Collections;


Is it really need?

No, removed.

iSazonov · 2017-05-18T11:17:34Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

-                        }
-                        if (null != switchEvaluator)
+                        // Internal test code, run only if the
+                        // "GciEnumerationActionFilename" test hook is set


Usually we use single quota.

iSazonov · 2017-05-18T11:20:31Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

-                        if (null != switchEvaluator)
+                        // Internal test code, run only if the
+                        // "GciEnumerationActionFilename" test hook is set
+                        var testActionFilename = InternalTestHooks.GciEnumerationActionFilename;


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

@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.

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

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

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.

iSazonov · 2017-05-18T11:21:08Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

+                            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


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

iSazonov · 2017-05-18T11:23:23Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

-                                    filesystemInfo.Name,
-                                    filesystemInfo.FullName,
-                                    false);
+                                attributeFilter = evaluator.Evaluate(filesystemInfo.Attributes);  // expressions


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

iSazonov · 2017-05-18T11:23:33Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

                            {
-                                if (filesystemInfo is FileInfo)
-                                    WriteItemObject(filesystemInfo, filesystemInfo.FullName, false);
+                                switchAttributeFilter = switchEvaluator.Evaluate(filesystemInfo.Attributes);  // switch parameters


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

iSazonov · 2017-05-18T11:24:23Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

+                            bool hidden = false;
+                            if (!Force) hidden = (filesystemInfo.Attributes & FileAttributes.Hidden) != 0;
+
+                            // if "Hidden" is explicitly specified anywhere in the attribute filter, then override


The same about single quota.

iSazonov · 2017-05-18T11:25:15Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

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


Please use full pattern:

if (...) { ... }

iSazonov · 2017-05-18T11:32:51Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

+
+                            // 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


Typos - begin with capital letters.

iSazonov · 2017-05-18T11:51:54Z

test/powershell/Modules/Microsoft.PowerShell.Management/Get-ChildItem.Tests.ps1

            $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) {


Why we skip Windows?

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.

So the test is ok on Windows too?

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.

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.

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?

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?

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).

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.

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.

jeffbi · 2017-05-18T15:42:15Z

@iSazonov Thanks for adding the link. I keep thinking that putting it in the title works, but of course it doesn't.

lzybkr · 2017-05-19T18:18:50Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

+                            var newFilename = InternalTestHooks.GciEnumerationActionRename;
+                            if (String.IsNullOrEmpty(newFilename))
+                            {
+                                File.Delete(fullName);


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

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

The test 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.

iSazonov · 2017-05-20T16:58:51Z

src/System.Management.Automation/engine/Utils.cs

+
        /// <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)


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

I feel like it would be better to not allow both to be set at the same time, which the string property accomplishes.

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

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

iSazonov · 2017-05-20T17:01:05Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

                        {
-                            attributeFilter = evaluator.Evaluate(filesystemInfo.Attributes);  // expressions
-                            filterHidden = evaluator.ExistsInExpression(FileAttributes.Hidden);
+                            if (filesystemInfo.Name == "c")


We need to try to avoid a match with real names. Maybe use Guid-based names for test file names?

Fixed. Using GUID-based names.

* Revert to using only type bool for test hooks. * Use two boolean hooks, for delete and rename * Use GUID-based filenames

iSazonov · 2017-05-22T03:49:30Z

test/powershell/Modules/Microsoft.PowerShell.Management/Get-ChildItem.Tests.ps1

-            $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"


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

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.

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

iSazonov · 2017-05-22T03:49:37Z

test/powershell/Modules/Microsoft.PowerShell.Management/Get-ChildItem.Tests.ps1

            $files | Should not be $null
            $files.Count | Should be 6
-            $files.Name.Contains(".F")
+            $files.Name.Contains($item_F)


Maybe "Should Be" ?

No doubt. Fixed.

Change a GUID-based filename to begin with letter/hex digit 'E'

iSazonov · 2017-05-22T05:00:21Z

LGTM.

@jeffbi Thanks! Great work!

daxian-dbw · 2017-05-24T18:20:26Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

+                            if (filesystemInfo.Name == "c283d143-2116-4809-bf11-4f7d61613f92")
+                            {
+                                var fullName = Path.Combine(directory.FullName, filesystemInfo.Name);
+                                File.Delete(fullName);


Why not just use filesystemInfo.FullName as the fullName?

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.

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?

I haven't seen an issue, no. I was just trying to avoid one. I'll go ahead and make the change to FullName.

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

daxian-dbw · 2017-05-24T18:27:34Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

-                            switchFilterHidden = switchEvaluator.ExistsInExpression(FileAttributes.Hidden);
+                            if (filesystemInfo.Name == "B1B691A9-B7B1-4584-AED7-5259511BEEC4")
+                            {
+                                var fullName = Path.Combine(directory.FullName, filesystemInfo.Name);


ditto, maybe use filesystemInfo.FullName instead?

daxian-dbw · 2017-05-24T18:33:11Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

                        {
-                            attributeFilter = evaluator.Evaluate(filesystemInfo.Attributes);  // expressions
-                            filterHidden = evaluator.ExistsInExpression(FileAttributes.Hidden);
+                            if (filesystemInfo.Name == "c283d143-2116-4809-bf11-4f7d61613f92")


It's recommended to avoid using == for string comparison. Please use string.Equals(a, b, InvariantCulture)

daxian-dbw · 2017-05-24T18:35:14Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

                        {
-                            switchAttributeFilter = switchEvaluator.Evaluate(filesystemInfo.Attributes);  // switch parameters
-                            switchFilterHidden = switchEvaluator.ExistsInExpression(FileAttributes.Hidden);
+                            if (filesystemInfo.Name == "B1B691A9-B7B1-4584-AED7-5259511BEEC4")


ditto. I know this is for testing, but code pattern will be copied, so it's better to be consistent.

daxian-dbw · 2017-05-24T18:51:29Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

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


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); }

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.

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

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

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.

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.

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.

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?

daxian-dbw · 2017-05-24T18:54:08Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

+                            }
                        }
-                        if (null != switchEvaluator)
+                        else if (InternalTestHooks.GciEnumerationActionRename)


~~Same here - may be move this outside the loop.~~ Let's keep it as is.

daxian-dbw · 2017-05-24T19:18:43Z

src/System.Management.Automation/engine/Utils.cs

        // Simulate 'System.Diagnostics.Stopwatch.IsHighResolution is false' to test Get-Uptime throw
        internal static bool StopwatchIsNotHighResolution;

+        // Will be either "delete" or "rename" during tests.


The comment should describe where these hooks are used, and used for what.

Better comments on test hook variables. Use filesystemInfo.FullName. Use String.Equals rather than == operator.

daxian-dbw · 2017-05-25T20:14:40Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

                        {
-                            attributeFilter = evaluator.Evaluate(filesystemInfo.Attributes);  // expressions
-                            filterHidden = evaluator.ExistsInExpression(FileAttributes.Hidden);
+                            if (string.Equals(filesystemInfo.Name, "c283d143-2116-4809-bf11-4f7d61613f92", StringComparison.Ordinal))


The string.Equals check below uses StringComparison.InvariantCulture and this one uses StringComparison.Ordinal. It's better to be consistent.

D'oh! Leftover artifact, thanks for spotting that.

Fixed.

daxian-dbw · 2017-05-25T20:17:50Z

@jeffbi thanks for the thorough thinking on the fix and test. One minor comment and it should be good to go once that's addressed.

Use InvariantCulture instead of Ordinal.

msftclas added the cla-already-signed label May 18, 2017

iSazonov reviewed May 18, 2017

View reviewed changes

Jeff Bienstadt added 2 commits May 18, 2017 09:13

Changes per code review.

cd57775

More changes per code review.

170f901

lzybkr reviewed May 19, 2017

View reviewed changes

More changes per code review.

43339c0

The test 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.

iSazonov reviewed May 20, 2017

View reviewed changes

Changes per code review.

2e97d00

* Revert to using only type bool for test hooks. * Use two boolean hooks, for delete and rename * Use GUID-based filenames

iSazonov reviewed May 22, 2017

View reviewed changes

Add "| Should Be" to existing test.

c7e0b56

Change a GUID-based filename to begin with letter/hex digit 'E'

daxian-dbw self-assigned this May 24, 2017

daxian-dbw reviewed May 24, 2017

View reviewed changes

Changes per code review.

fe7dd4b

Better comments on test hook variables. Use filesystemInfo.FullName. Use String.Equals rather than == operator.

daxian-dbw reviewed May 25, 2017

View reviewed changes

Change per code review.

ba536e4

Use InvariantCulture instead of Ordinal.

daxian-dbw approved these changes May 25, 2017

View reviewed changes

daxian-dbw merged commit c29bd7d into PowerShell:master May 26, 2017

jeffbi deleted the dir-2856-simple branch May 26, 2017 01:24

daxian-dbw mentioned this pull request Jun 30, 2017

Behavior changed about filesystemInfo.Attributes in .NET Core preview2/3 causes 2 tests to fail in Get-ChildItem.Tests.ps1 #4145

Closed

jeffbi mentioned this pull request Jul 8, 2017

Remove test hooks and tests for files being deleted or renamed during enumeration #4200

Merged

Conversation

jeffbi commented May 18, 2017 • edited by daxian-dbw Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented May 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw May 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov May 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffbi commented May 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffbi commented May 18, 2017 •

edited by daxian-dbw

Loading

daxian-dbw May 19, 2017 •

edited

Loading

iSazonov May 19, 2017 •

edited

Loading