8000 Behavior changed about `filesystemInfo.Attributes` in .NET Core preview2/3 causes 2 tests to fail in `Get-ChildItem.Tests.ps1` · Issue #4145 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]
Skip to content

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

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

Jump to bottom
Closed
daxian-dbw opened this issue Jun 30, 2017 · 17 comments
Closed

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

daxian-dbw opened this issue Jun 30, 2017 · 17 comments
Labels
Resolution-Fixed The issue is fixed. WG-Engine-Providers built-in PowerShell providers such as FileSystem, Certificates, Registry, etc.
Milestone
6.0.0-GA

Comments

@daxian-dbw
Copy link
Member

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
@daxian-dbw daxian-dbw added the WG-Engine-Providers built-in PowerShell providers such as FileSystem, Certificates, Registry, etc. label Jun 30, 2017
@daxian-dbw daxian-dbw added this to the 6.0.0-HighPriority milestone Jun 30, 2017
@daxian-dbw
Copy link
Member Author

Since PR #3806 was to fix #2856, I set the properties of this issue similar to 2856.

@daxian-dbw daxian-dbw mentioned this issue Jun 30, 2017
Merged
@jeffbi
Copy link
Contributor
jeffbi commented Jul 4, 2017 •
edited
Loading

What I'm seeing on both platforms is that the exception is no longer thrown, but FilesystemInfo.Attributes is 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 Mode property is null. Mode is not one of the regular properties of FileSystemInfo and 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/catch is doing us any harm) and change the test so that it ensures that the deleted file is included in the result of Get-ChildItem.

@iSazonov
Copy link
Collaborator
iSazonov commented 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
Copy link
Contributor
jeffbi commented Jul 4, 2017

I had fetched the latest source (commit ece27ff), done a new Start-PSBootStrap and Start-PSBuild -clean. Is there something else I need to do?

@iSazonov
Copy link
Collaborator
iSazonov commented Jul 5, 2017 •
edited
Loading

I meant that we should call Refresh() or Exists() to get fresh FileAttributes.

@jeffbi
Copy link
Contributor
jeffbi commented Jul 5, 2017

@iSazonov You were exactly right. I hadn't realized there was a Refresh() method on FilesystemInfo.

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
Copy link
Member Author

That means calling Refresh() or Exists() on every file system item. How expensive is that?

@iSazonov
Copy link
Collaborator
iSazonov commented Jul 6, 2017

Yes, it is expensive. I think we should avoid this. Now we haven't exception so maybe silently ignore this?

@jeffbi
Copy link
Contributor
jeffbi commented 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
Copy link
Collaborator
iSazonov commented 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
Copy link
Contributor
jeffbi commented 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
Copy link
Member Author

I don't think the try/catch is doing us any harm

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:

  1. the root cause of the original issue is gone (no exception now),
  2. the test hooks won't give us code coverage for the catch blocks
  3. we pay a tax of doing two checks for every file system item.

@jeffbi @iSazonov what are your thoughts?

@jeffbi
Copy link
Contributor
jeffbi commented 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
Copy link
Collaborator
iSazonov commented Jul 7, 2017

I agree to remove the test hooks and tests:

  1. CoreFX wants to be compatible with .Net Framework so no exception will be in future (but we have still try/catch). Maybe we should just add a comment about it.
  2. "Race conditions" for file system is normal (multiple processes add, remove and change files, permissions and attributes) and we don't need to test and to worry about this.

@daxian-dbw
Copy link
Member Author

Thanks @jeffbi and @iSazonov, then let's remove the test hooks and the corresponding tests.

@jeffbi jeffbi mentioned this issue Jul 7, 2017
Merged
@SteveL-MSFT
Copy link
Member

@daxian-dbw is there anything more to do with this one?

@daxian-dbw
Copy link
Member Author

Nothing needs to be done. This issue was not closed when merging #4200

@daxian-dbw daxian-dbw added the Resolution-Fixed The issue is fixed. label Aug 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Resolution-Fixed The issue is fixed. WG-Engine-Providers built-in PowerShell providers such as FileSystem, Certificates, Registry, etc.
Projects
None yet
Milestone
6.0.0-GA
Development

No branches or pull requests
4 participants
@daxian-dbw @jeffbi @SteveL-MSFT @iSazonov
0