Repository navigation
Behavior changed about filesystemInfo.Attributes in .NET Core preview2/3 causes 2 tests to fail in Get-ChildItem.Tests.ps1 #4145
Description
In 2.0.0-preview1 .NET Core, filesystemInfo.Attributes raises an FileNotFoundException exception when the file is deleted or renamed during the enumration.
In 2.0.0-preview2/3 .NET Core, filesystemInfo.Attributes returns -1 without raising any exception.
This behavior change was by design -- to align with the current behavior on Full .NET. For detailed information, please see https://github.com/dotnet/corefx/issues/20456.
This behavior change causes the following tests to fail in test/powershell/Modules/Microsoft.PowerShell.Management/Get-ChildItem.Tests.ps1 (they will be marked as pending for now):
- "Should continue enumerating a directory when a contained item is deleted"
- "Should continue enumerating a directory when a contained item is renamed"
These 2 tests were added in PR #3806, which assumes that the FileNotFoundException will be thrown in case the file is deleted or renamed during enumeration. Since the behavior has changed in .NET Core, the fix needs to be revisited.
/cc jeffbi
Activity
daxian-dbw commentedon Jun 30, 2017
Since PR #3806 was to fix #2856, I set the properties of this issue similar to 2856.
jeffbi commentedon Jul 4, 2017
What I'm seeing on both platforms is that the exception is no longer thrown, but
FilesystemInfo.Attributesis not returning -1. Instead it returns the attributes the file had before it was deleted, so there is no real indication that the file was deleted.After the item is written to the pipeline, when it is displayed, the
Modeproperty is null.Modeis not one of the regular properties ofFileSystemInfoand is added after the provider writes the object.I think it might be worth keeping the provider code as is (I don't think the
try/catchis doing us any harm) and change the test so that it ensures that the deleted file is included in the result ofGet-ChildItem.iSazonov commentedon Jul 4, 2017
It seems we should refresh to get -1 https://github.com/dotnet/corefx/blob/801dde95a5eac06140d0ac633ac3f9bfdd25aca5/src/System.IO.FileSystem/src/System/IO/FileSystemInfo.Unix.cs#L264
jeffbi commentedon Jul 4, 2017
I had fetched the latest source (commit ece27ff), done a new
Start-PSBootStrapandStart-PSBuild -clean. Is there something else I need to do?iSazonov commentedon Jul 5, 2017
I meant that we should call Refresh() or Exists() to get fresh FileAttributes.
jeffbi commentedon Jul 5, 2017
Ilya (@iSazonov) You were exactly right. I hadn't realized there was a
Refresh()method onFilesystemInfo.Assuming we want to continue with the current behavior, i.e. writing an error message and continuing the enumeration, I'm preparing a PR to do that.
daxian-dbw commentedon Jul 5, 2017
That means calling
Refresh()orExists()on every file system item. How expensive is that?iSazonov commentedon Jul 6, 2017
Yes, it is expensive. I think we should avoid this. Now we haven't exception so maybe silently ignore this?
jeffbi commentedon Jul 6, 2017
If we don't refresh and check for -1, we should end up displaying the file as if it had not (yet) been deleted. Whether that's the right thing may depend on how the cmdlet is being used.
iSazonov commentedon Jul 6, 2017
The file system is constantly changing, so perhaps this is the expected behavior. We can also skip new files or changing their size if it happens later we read first the list.
jeffbi commentedon Jul 6, 2017
I quite agree. So long as our users understand that the file system is this dynamic, then this should be the expected behavior.
So my take is that we leave the cmdlet code as is, with no additional call to
Refresh(), and modify the tests to verify that the cmdlet does not show an error but instead retrieves the file information it can.daxian-dbw commentedon Jul 6, 2017
I quite agree that we should keep the
try/catch. However, (btw I could be totally wrong) should we keep the test hooks? Here are my thoughts:catchblocksjeffbi Ilya (@iSazonov) what are your thoughts?
jeffbi commentedon Jul 6, 2017
If we remove the test hooks, the tests themselves don't make much sense. That being said, I'm in favor of removing the test hooks and the tests.
iSazonov commentedon Jul 7, 2017
I agree to remove the test hooks and tests:
daxian-dbw commentedon Jul 7, 2017
Thanks jeffbi and Ilya (@iSazonov), then let's remove the test hooks and the corresponding tests.
SteveL-MSFT commentedon Aug 1, 2017
Dongbo Wang (@daxian-dbw) is there anything more to do with this one?
daxian-dbw commentedon Aug 1, 2017
Nothing needs to be done. This issue was not closed when merging #4200