Cleanup NativeItemExists() #6929

iSazonov · 2018-05-24T07:56:06Z

PR Summary

Related #6874. After the PR we can remove the files - isfile.cpp, isfile.h, isdirectory.cpp, isdirectory.h - from psl.

Use File.GetAttributes() instead of p/invokes. The method throws access exceptions (unlike File.Exists and Directory.Exists) which we use for better error messages.
Remove 'Native' from method names.
As stated in Cleanup FileSystem provider #6909 we get a regression in rare case (for files like pagefile.sys). Fix is ready in CoreFX, we get it in 2.1.1. I suggest ignore the regression because this is a very-very rare situation (Get-ChildItem c:\pagefile.sys -Hidden). The .Net Core team was not even able to create an artificial test for such files and uses a real pagefile.sys file for the test. Also the enumeration is still working (dir c:\ -hidden).

PR Checklist

iSazonov · 2018-05-25T05:44:56Z

Reopen to restart CIs.

iSazonov · 2018-05-25T15:21:16Z

@TravisEz13 Could you please help - I can not find why CIs failed?

iSazonov · 2018-05-28T12:18:37Z

Feature tests failed with Pester not found. 😕

SteveL-MSFT · 2018-05-30T04:42:30Z

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

+            catch
+            {
+            }
+            return false;


should be

return result;

SteveL-MSFT · 2018-05-30T04:44:55Z

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

+            bool result = false;
+            try
+            {
+                result =  ItemExists(path, out bool unused);


extra whitespace after equals sign

perhaps use named parameter to make it more clear what unused is?

unused here is self explaination - we don't use the parameter at all.

Extra space fixed.

SteveL-MSFT · 2018-05-30T04:49:41Z

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

        }

-        // This is done through P/Invoke since File.Exists and Directory.Exists pay 13% performance degradation


Are we sure we no longer have a perf penalty with this change?

We have still a perf penalty (on windows because normalization(!) - one extra kernel call in .Net code) but it is tracked in CoreFX https://github.com/dotnet/corefx/issues/19717
We have already mentioned that we do up to 7 the normalizations of the same path per cmdlet call. So perf penalty is more in PowerShell code. I'm trying to find how we can reduce the path normalization perf penalty.

Also please see https://github.com/dotnet/corefx/issues/27049#issuecomment-392826352 - .Net team is ready to help.

TravisEz13 · 2018-06-04T18:53:09Z

The Linux failure does not seem related to your change
https://travis-ci.org/PowerShell/PowerShell/jobs/385527642#L903

I'm restarting the CI run

NativeItemExists -> ItemExists NativeFileExists -> FileExists NativeDirectoryExists -> DirectoryExists

iSazonov · 2018-06-05T04:49:50Z

@TravisEz13 Thanks! But CIs failed again. I rebased - perhaps it resolve..

No need in throw here if file not found. (Previously we could throw in rare cases.)

iSazonov · 2018-06-05T11:23:39Z

@SteveL-MSFT @TravisEz13 Please review last two commits. I found my mistake. I added new comment and TODO to continue the cleanup process later.

daxian-dbw · 2018-06-06T22:00:58Z

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

-                    exception = new UnauthorizedAccessException(win32Exception.Message, win32Exception);
-                }
-                else if (errorCode == 32)
-                {


File.GetAttributes doesn't spend this extra effort in case the errorCode is 32 (ERROR_SHARING_VIOLATION). So, this is potentially a breaking change.

The same happens to WinSafeGetFileAttributes which is removed by #6909.

Please see the PR description (Fix is ready in CoreFX, we get it in 2.1.1.)

Please see the PR description - we'll get the fix in .Net Core 2.1.1.

OK, so this will only affect c:\pagefile.sys? If so, then it's fine to leave it as is.

Three files - pagefile.sys, hiberfil.sys, swapfile.sys. In the CoreFX PR a comment says that they can not emulate the behavior and so use the real file c:\pagefile.sys in test. I think users are unlikely to find something like this

daxian-dbw · 2018-06-06T23:40:43Z

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

+                // Use 'File.GetAttributes()' because we want to get access exceptions.
+                // TODO: we should review the tricky logic
+                // (historically we throw 'UnauthorizedAccessException' here) and migrate
+                // to standard methods 'File.Exist()' and 'Directory.Exists()' where possible.


Why having this todo comment? File.GetAttributes will also throw UnauthorizedAccessException when the error is ERROR_ACCESS_DENIED.

And can you please point me to the places where it turns to File.Exists() and Directory.Exits() when NativeItemExists fails?

Seems my comment is not clear.
I meant that we should migrate from the ItemExists method to File.Exists() and Directory.Exits() which don't throw UnauthorizedAccessException.

Yeah, I agree. I also pointed that out in my comments. Open an issue to track that migration instead of this comment. This comment is not useful in the context of this method, which is designed to throw on error.

TODO comment was removed.

daxian-dbw · 2018-06-06T23:59:02Z

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

-            return Platform.NonWindowsIsFile(path);
-#else
-
+#if !UNIX
            if (IsReservedDeviceName(path))


You don't need to enclose this in #if !Unix. The method body already separate the Unix/Windows code path.

Seems method in line 898 (internal static bool ItemExists(string path)) is used on Unix.

Take a look at the method IsReservedDeviceName, it's body already takes care of the #if/def.

:-) I looked the other way.
Fixed.

daxian-dbw · 2018-06-07T00:00:36Z

src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs

@@ -4111,7 +4111,7 @@ private ExternalScriptInfo FindLocalizedModuleManifest(string path)

                String filePath = stringBuilder.ToString();

-                if (Utils.NativeFileExists(filePath))
+                if (File.Exists(filePath))


Why not use Utils.FileExists ?

Utils.FileExists behavior is the same (no exception is thrown). So it is better to use the standard method.

It's better to make all the Utils.NativeFileExists -> File/Directory.Exists changes in a separate PR. So for now, just do the simple replacement of Utils.NativeFileExists -> Utils.FileExists.

daxian-dbw · 2018-06-07T00:10:18Z

src/System.Management.Automation/engine/Modules/ModuleUtils.cs

@@ -233,7 +233,7 @@ internal static IEnumerable<string> GetDefaultAvailableModuleFiles(string topDir
                foreach (string ext in ModuleIntrinsics.PSModuleExtensions)
                {
                    string moduleFile = Path.Combine(directoryToCheck, proposedModuleName) + ext;
-                    if (!Utils.NativeFileExists(moduleFile))
+                    if (!File.Exists(moduleFile))


Same here.
Actually, I see Utils.NativeFileExists and File.Exists mixed in this file (possibly in that files too). Do we really care about the exception on failure in those cases?

Utils.FileExists behavior is the same (no exception is thrown). So it is better to use the standard method.

Yes, we should review all these cases but there are dozens of them to do in this PR or in a new?

Yeah. let's do it in a separate PR. For this PR, just do the simple rename replacement: Utils.NativeFileExists -> Utils.FileExists.

daxian-dbw · 2018-06-07T00:50:50Z

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

                }
-
-                if (accessException != null)
+                catch (Exception accessException)
                {
                    ErrorRecord errorRecord = new ErrorRecord(accessException, "RemoveFileSystemItemUnAuthorizedAccess", ErrorCategory.PermissionDenied, directory);


Looks to me it's expecting an UnauthorizedAccessException, shall we narrow the catch to only catch this type of exception?

Here I only preserve the logic.
Really we should clean up the code too because .Net Core support reparse points and recursion https://github.com/dotnet/corefx/blob/f25eb288a449010574a6e95fe298f3ad880ada1e/src/System.IO.FileSystem/src/System/IO/FileSystem.Windows.cs#L359

Seems we should clean up all code in FileSystemProvider - perhaps we don't need FileSystemProviderV2 :-)

@daxian-dbw I opened #7017 to clean up the code.

I'm fine leaving this catch as is.

daxian-dbw · 2018-06-07T00:52:11Z

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

-                {
-                    throw accessException;
-                }
+                result = Utils.ItemExists(path, out bool unused);


"discard" variable.

daxian-dbw · 2018-06-07T00:54:17Z

src/System.Management.Automation/utils/ClrFacade.cs

            string sysRoot = System.Environment.GetEnvironmentVariable("SystemRoot");
            string urlmonPath = Path.Combine(sysRoot, @"System32\urlmon.dll");
-            if (Utils.NativeFileExists(urlmonPath))
+            if (Utils.FileExists(urlmonPath))


At least for these two uses, they do not expect exception to be thrown. So change them to File.Exists should be fine.

Maybe revert these two changes too, since we agreed to use a separate PR for the migration changes.

daxian-dbw · 2018-06-07T00:57:29Z

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

-        // This is done through P/Invoke since we pay 13% performance degradation
-        // through the CAS checks required by File.Exists and Directory.Exists
-        internal static bool NativeDirectoryExists(string path)
+        internal static bool DirectoryExists(string path)


I think we should change the name of this one and FileExists to something that indicate it might throw exception, so it's easy to tell whether you should use them instead of File.Exists and Directory.Exists when writing code.

I suggest rename them to CheckFileExistsAndThrowOnError and CheckDirectoryExistsAndThrowOnError.

I'll rename if we will not migrate to File.Exists/Directory.Exists in the PR.

No, let's use a separate PR to do the migration changes. Go ahead rename this method.

daxian-dbw · 2018-06-07T00:58:07Z

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

-        // This is done through P/Invoke since File.Exists and Directory.Exists pay 13% performance degradation
-        // through the CAS checks, and are terribly slow for network paths.
-        internal static bool NativeItemExists(string path, out bool isDirectory, out Exception exception)
+        internal static bool ItemExists(string path, out bool isDirectory)


Similarly, maybe changing this to CheckItemExistsAndThrowOnError.

I'll rename if we will not migrate to File.Exists/Directory.Exists in the PR.

iSazonov · 2018-06-07T03:25:44Z

@daxian-dbw I am trying to keep the PR as small as possible. There are dozens of places where we should consider replacing old code (ItemExists) with File.Exists (Directory.Exists). Do you want I do it in this PR?

daxian-dbw · 2018-06-08T18:47:33Z

@iSazonov No, you don't need to do it in this PR. Open an issue to track the analysis and migration work (Utils.FileExists/DirectoryExists => File.Exists/Directory.Exists).

iSazonov · 2018-06-09T08:14:49Z

Tracking Issue #7031

This reverts commit db25895.

iSazonov · 2018-06-09T14:52:24Z

@daxian-dbw File.Exists() and Directory.Exists() return false for device names so we can remove IsReservedDeviceName call from ItemExists() method at all. What do you think?

daxian-dbw · 2018-06-12T19:12:33Z

@iSazonov ItemExists doesn't call FileExists or Directory.Exists. Do you mean File.GetAttributes returns -1 for reserved device names?

daxian-dbw · 2018-06-12T19:20:40Z

The changes LGTM.
@iSazonov Are you going to rename FileExists, DirectoryExists and ItemExists to
CheckFileExistsAndThrowOnError, CheckDirectoryExistsAndThrowOnError and CheckItemExistsAndThrowOnError in this PR?

iSazonov · 2018-06-13T05:20:33Z

Are you going to rename FileExists, DirectoryExists and ItemExists to
CheckFileExistsAndThrowOnError, CheckDirectoryExistsAndThrowOnError and CheckItemExistsAndThrowOnError in this PR?

No, I'll remove the methods in follow PR.

ItemExists doesn't call FileExists or Directory.Exists. Do you mean File.GetAttributes returns -1 for reserved device names?

Never mind - I hope to remove ItemExists() altogether in follow PR.

iSazonov · 2018-06-14T03:47:44Z

@daxian-dbw Should I fix something else?

daxian-dbw

@iSazonov If you plan to remove those methods in your next PR, then it's fine to leave their names unchanged.

daxian-dbw · 2018-06-14T18:31:35Z

@iSazonov It's preferred that you remove the native code from psl-native along with your managed code changes in your following PRs. For the PRs you already finished, I listed the tasks to track removing the corresponding native code in #6874.

iSazonov · 2018-06-15T18:31:44Z

@daxian-dbw My thoughts was that if we have to re-publish psl-native after every change
it is better to finish first the cleanup in the repo and only then cleanup in psl-native repo.

iSazonov requested review from daxian-dbw, SteveL-MSFT and adityapatwardhan May 24, 2018 07:56

iSazonov requested review from anmenaga, BrucePay and Francisco-Gamino as code owners May 24, 2018 07:56

iSazonov force-pushed the psl-cleanup-isfile branch from 123c38a to 0b84586 Compare May 24, 2018 08:43

iSazonov closed this May 25, 2018

iSazonov reopened this May 25, 2018

iSazonov closed this May 25, 2018

iSazonov reopened this May 25, 2018

SteveL-MSFT requested changes May 30, 2018

View reviewed changes

iSazonov added 13 commits June 5, 2018 09:48

Remove unneeded p/invokes

d364c67

Create NativeItemExists based on File.GetAttributes(path)

01d2685

Update NativeItemExists w/o throw

1637ad1

Replace NativeItemExists in GetPathItems

058ac67

Replace NativeItemExists in RemoveDirectoryInfoItem

286d714

Replace NativeItemExists in ItemExists

4e941d6

Replace NativeItemExists in GetProperty

b515681

Replace NativeItemExists in SetProperty

5d85a4d

Replace CheckItemExists with NativeItemExists

23e44ac

Update NativeFileExists and NativeDirectoryExists

c6573fb

Remove old NativeItemExists

fdb1667

[Feature] Remove 'Native' from method names

cf419bf

NativeItemExists -> ItemExists NativeFileExists -> FileExists NativeDirectoryExists -> DirectoryExists

Fix return in ItemExists

67c5d33

iSazonov force-pushed the psl-cleanup-isfile branch from 8e8ac45 to 67c5d33 Compare June 5, 2018 04:48

SteveL-MSFT approved these changes Jun 5, 2018

View reviewed changes

iSazonov added 2 commits June 5, 2018 13:07

Fix module loading

c37761a

No need in throw here if file not found. (Previously we could throw in rare cases.)

Recover ItemExists() behavior for File not found exception

0f25099

daxian-dbw self-assigned this Jun 6, 2018

daxian-dbw requested changes Jun 7, 2018

View reviewed changes

iSazonov added 2 commits June 7, 2018 10:34

Use "discard" variable

757d545

Use File.Exists in GetFileSecurityZone

db25895

iSazonov mentioned this pull request Jun 9, 2018

Replace Utils.FileExists/DirectoryExists => File.Exists/Directory.Exists #7031

Closed

iSazonov added 3 commits June 9, 2018 13:58

Remove unneeded #if !UNIX

2c0fa8f

Revert "Use File.Exists in GetFileSecurityZone"

9098964

This reverts commit db25895.

Remove unneeded comment

3963c46

daxian-dbw approved these changes Jun 14, 2018

View reviewed changes

iSazonov mentioned this pull request Jun 14, 2018

Need psl-native library code review #6874

Closed

3 tasks

daxian-dbw merged commit 81244fb into PowerShell:master Jun 14, 2018

iSazonov deleted the psl-cleanup-isfile branch June 15, 2018 18:31

This was referenced Jun 18, 2018

Remove Utils.FileExists(), Utils.DirectoryExists() and Utils.ItemExists() methods #7100

Closed

Cleanup Utils.FileExists(), Utils.DirectoryExists() and Utils.ItemExists() methods #7129

Merged

daxian-dbw mentioned this pull request Jun 26, 2018

Move to dotnet 2.1.301 #7161

Merged

11 tasks

		}

		// This is done through P/Invoke since File.Exists and Directory.Exists pay 13% performance degradation

Cleanup NativeItemExists() #6929

Cleanup NativeItemExists() #6929

Conversation

iSazonov commented May 24, 2018

PR Summary

PR Checklist

iSazonov commented May 25, 2018

iSazonov commented May 25, 2018

iSazonov commented May 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iSazonov May 30, 2018 • edited Loading

Choose a reason for hiding this comment

TravisEz13 commented Jun 4, 2018

iSazonov commented Jun 5, 2018

iSazonov commented Jun 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

iSazonov Jun 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iSazonov commented Jun 7, 2018

daxian-dbw commented Jun 8, 2018

iSazonov commented Jun 9, 2018

iSazonov commented Jun 9, 2018

daxian-dbw commented Jun 12, 2018

daxian-dbw commented Jun 12, 2018

iSazonov commented Jun 13, 2018

iSazonov commented Jun 14, 2018

daxian-dbw left a comment

Choose a reason for hiding this comment

daxian-dbw commented Jun 14, 2018

iSazonov commented Jun 15, 2018

iSazonov May 30, 2018 •

edited

Loading

daxian-dbw Jun 8, 2018 •

edited

Loading

iSazonov Jun 9, 2018 •

edited

Loading