Make Get-ChildItem continue enumeration when encountering error on contained item (#2856) #3806
Conversation
…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.
|
@jeffbi I added link on Issue |
| @@ -13,6 +13,7 @@ | |||
| using System.Globalization; | |||
| using System.IO; | |||
| using System.Linq; | |||
| using System.Collections; | |||
| } | ||
| if (null != switchEvaluator) | ||
| // Internal test code, run only if the | ||
| // "GciEnumerationActionFilename" test hook is set |
There was a problem hiding this comment.
Usually we use single quota.
| if (null != switchEvaluator) | ||
| // Internal test code, run only if the | ||
| // "GciEnumerationActionFilename" test hook is set | ||
| var testActionFilename = InternalTestHooks.GciEnumerationActionFilename; |
There was a problem hiding this comment.
For discussion - We never do that but it seems we should mask test hooks by #if Debug.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@daxian-dbw @lzybkr Could you please clarify should we leave test codes in Release build?
There was a problem hiding this comment.
We do leave test hooks in all builds because we primarily test release builds./
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
The same about single quota.
And please put comments on separate lines before codes.
| filesystemInfo.Name, | ||
| filesystemInfo.FullName, | ||
| false); | ||
| attributeFilter = evaluator.Evaluate(filesystemInfo.Attributes); // expressions |
There was a problem hiding this comment.
Please remove the comment or put on separate line before the code and expand it.
| { | ||
| if (filesystemInfo is FileInfo) | ||
| WriteItemObject(filesystemInfo, filesystemInfo.FullName, false); | ||
| switchAttributeFilter = switchEvaluator.Evaluate(filesystemInfo.Attributes); // switch parameters |
There was a problem hiding this comment.
Please remove the comment or put on separate line before the code and expand it.
| bool hidden = false; | ||
| if (!Force) hidden = (filesystemInfo.Attributes & FileAttributes.Hidden) != 0; | ||
|
|
||
| // if "Hidden" is explicitly specified anywhere in the attribute filter, then override |
There was a problem hiding this comment.
The same about single quota.
| } | ||
|
|
||
| bool hidden = false; | ||
| if (!Force) hidden = (filesystemInfo.Attributes & FileAttributes.Hidden) != 0; |
There was a problem hiding this comment.
Please use full pattern:
if (...)
{
...
}|
|
||
| // 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 |
There was a problem hiding this comment.
Typos - begin with capital letters.
| @@ -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) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
So the test is ok on Windows too?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@iSazonov Thanks for adding the link. I keep thinking that putting it in the title works, but of course it doesn't. |
| var newFilename = InternalTestHooks.GciEnumerationActionRename; | ||
| if (String.IsNullOrEmpty(newFilename)) | ||
| { | ||
| File.Delete(fullName); |
There was a problem hiding this comment.
This test hook frightens me - please think through the security implications of this.
There was a problem hiding this comment.
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.
| /// <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) |
There was a problem hiding this comment.
We can leave the parameter as bool and define two variables internal static bool GciEnumerationActionDelete = false; and internal static bool GciEnumerationActionRename = false;
There was a problem hiding this comment.
I feel like it would be better to not allow both to be set at the same time, which the string property accomplishes.
There was a problem hiding this comment.
I talked about two variables - you can enable hooks at different times.
There was a problem hiding this comment.
Changed. If both are set at the same time, only the delete will be done.
| { | ||
| attributeFilter = evaluator.Evaluate(filesystemInfo.Attributes); // expressions | ||
| filterHidden = evaluator.ExistsInExpression(FileAttributes.Hidden); | ||
| if (filesystemInfo.Name == "c") |
There was a problem hiding this comment.
We need to try to avoid a match with real names. Maybe use Guid-based names for test file names?
There was a problem hiding this comment.
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
| $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" |
There was a problem hiding this comment.
Why we use lower case for "_a" and below for "_c"?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The reason I used lower-case in the variable names was to call out that casing was significant.
| } | ||
|
|
||
| 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) |
Change a GUID-based filename to begin with letter/hex digit 'E'
|
LGTM. @jeffbi Thanks! Great work! |
| if (filesystemInfo.Name == "c283d143-2116-4809-bf11-4f7d61613f92") | ||
| { | ||
| var fullName = Path.Combine(directory.FullName, filesystemInfo.Name); | ||
| File.Delete(fullName); |
There was a problem hiding this comment.
Why not just use filesystemInfo.FullName as the fullName?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I haven't seen an issue, no. I was just trying to avoid one. I'll go ahead and make the change to FullName.
There was a problem hiding this comment.
Just tried on Ubuntu 16.04, filesystemInfo.FullName also works fine 😄
| switchFilterHidden = switchEvaluator.ExistsInExpression(FileAttributes.Hidden); | ||
| if (filesystemInfo.Name == "B1B691A9-B7B1-4584-AED7-5259511BEEC4") | ||
| { | ||
| var fullName = Path.Combine(directory.FullName, filesystemInfo.Name); |
There was a problem hiding this comment.
ditto, maybe use filesystemInfo.FullName instead?
| { | ||
| attributeFilter = evaluator.Evaluate(filesystemInfo.Attributes); // expressions | ||
| filterHidden = evaluator.ExistsInExpression(FileAttributes.Hidden); | ||
| if (filesystemInfo.Name == "c283d143-2116-4809-bf11-4f7d61613f92") |
There was a problem hiding this comment.
It's recommended to avoid using == for string comparison. Please use string.Equals(a, b, InvariantCulture)
| { | ||
| switchAttributeFilter = switchEvaluator.Evaluate(filesystemInfo.Attributes); // switch parameters | ||
| switchFilterHidden = switchEvaluator.ExistsInExpression(FileAttributes.Hidden); | ||
| if (filesystemInfo.Name == "B1B691A9-B7B1-4584-AED7-5259511BEEC4") |
There was a problem hiding this comment.
ditto. I know this is for testing, but code pattern will be copied, so it's better to be consistent.
| if (null != evaluator) | ||
| // Internal test code, run only if one of the | ||
| // 'GciEnumerationAction' test hooks are set. | ||
| if (InternalTestHooks.GciEnumerationActionDelete) |
There was a problem hiding this comment.
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);
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It seems we can put this after IEnumerable<FileSystemInfo> sortedChildList = childList.OrderBy(c => c.Name, StringComparer.CurrentCultureIgnoreCase);
There was a problem hiding this comment.
I'm not sure that solves the problem. Again, we have an enumerable object but enumeration doesn't start until the foreach.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
| } | ||
| if (null != switchEvaluator) | ||
| else if (InternalTestHooks.GciEnumerationActionRename) |
There was a problem hiding this comment.
Same here - may be move this outside the loop. Let's keep it as is.
| @@ -1580,6 +1580,10 @@ public static class InternalTestHooks | |||
| // Simulate 'System.Diagnostics.Stopwatch.IsHighResolution is false' to test Get-Uptime throw | |||
| internal static bool StopwatchIsNotHighResolution; | |||
|
|
|||
| // Will be either "delete" or "rename" during tests. | |||
There was a problem hiding this comment.
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.
| { | ||
| attributeFilter = evaluator.Evaluate(filesystemInfo.Attributes); // expressions | ||
| filterHidden = evaluator.ExistsInExpression(FileAttributes.Hidden); | ||
| if (string.Equals(filesystemInfo.Name, "c283d143-2116-4809-bf11-4f7d61613f92", StringComparison.Ordinal)) |
There was a problem hiding this comment.
The string.Equals check below uses StringComparison.InvariantCulture and this one uses StringComparison.Ordinal. It's better to be consistent.
There was a problem hiding this comment.
D'oh! Leftover artifact, thanks for spotting that.
Fixed.
|
@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.
Fix #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 specific file (file name hard-coded) when encountered during enumeration.