Unix: Move 'FileNotFound to DirectoryNotFound' handling to GetExceptionForIoErrno. by tmds · Pull Request #70661 · dotnet/runtime

tmds · 2022-06-13T12:07:05Z

On Windows, APIs throw DirectoryNotFoundException instead of
FileNotFoundException when the base components of a path don't exist.

SafeFileHandle, and FileSystemInfo have some specific code to detect
that case and do the same on Unix.

This change moves that mapping to Interop.IOErrors so it applies
to all users of GetExceptionForIoErrno.

…onForIoErrno. On Windows, APIs throw DirectoryNotFoundException instead of FileNotFoundException when the base components of a path don't exist. SafeFileHandle, and FileSystemInfo have some specific code to detect that case and to the same on Unix. This change moves that mapping to Interop.IOErrors so it applies to all users of GetExceptionForIoErrno.

ghost · 2022-06-13T12:07:19Z

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

On Windows, APIs throw DirectoryNotFoundException instead of
FileNotFoundException when the base components of a path don't exist.

SafeFileHandle, and FileSystemInfo have some specific code to detect
that case and to the same on Unix.

This change moves that mapping to Interop.IOErrors so it applies
to all users of GetExceptionForIoErrno.

Author:	tmds
Assignees:	-
Labels:	`area-System.IO`
Milestone:	-

tmds · 2022-06-13T17:54:05Z

/__w/1/s/src/libraries/Common/src/Interop/Unix/System.Native/Interop.Stat.cs(65,37): error SYSLIB1053: 'LibraryImportAttribute.StringMarshalling=StringMarshalling.Utf8' has no equivalent in 'DllImportAtttribute' and will not be forwarded [/__w/1/s/src/libraries/System.IO.Ports/src/System.IO.Ports.csproj]

@danmoseley @stephentoub I don't understand the error. What does this mean?

tmds · 2022-06-24T11:26:29Z

On Windows, APIs return different errors for the parent directory not being there (ERROR_PATH_NOT_FOUND), or the file itself (ERROR_FILE_NOT_FOUND). .NET throws DirectoryNotFoundException vs FileNotFoundException for these.

On Unix, ENOENT is used for both.
For Windows compatibility, on ENOENT .NET does a directory check for the parent to throw the same Exception type as Windows.

Currently those checks are implemented at different places.
This PR centralizes them so they apply to users of GetExceptionForIoErrno.
That will improve Windows compatibility because more locations will throw the same type as Windows.

To implement the exist check in Interop.GetExceptionForIoErrno I'm using Interop.Stat directly because I want to be clear that it doesn't end up calling GetExceptionForIoErrno in its implementation.

CI gives me an error I don't understand for using Interop.Stat in System.IO.Ports.csproj:

/__w/1/s/src/libraries/Common/src/Interop/Unix/System.Native/Interop.Stat.cs(65,37): error SYSLIB1053: 'LibraryImportAttribute.StringMarshalling=StringMarshalling.Utf8' has no equivalent in 'DllImportAtttribute' and will not be forwarded [/__w/1/s/src/libraries/System.IO.Ports/src/System.IO.Ports.csproj]

@dotnet/area-system-io @stephentoub ptal.

tmds · 2022-06-24T11:28:07Z

 if (isDirectory && errorCode == Interop.Errors.ERROR_FILE_NOT_FOUND)
 errorCode = Interop.Errors.ERROR_PATH_NOT_FOUND;
- if (isDirectory && errorCode == Interop.Errors.ERROR_ACCESS_DENIED && ignoreAccessDenied)
+ if (ignoreAccessDenied && errorCode == Interop.Errors.ERROR_ACCESS_DENIED)


This change is unrelated. I can move it to a separate PR.

tmds · 2022-07-05T13:48:20Z

@dotnet/area-system-io ptal.

Example where this change changes applies:

DirectoryInfodi=Directory.CreateDirectory(Path.Combine(Path.GetTempPath(),Path.GetRandomFileName()));FileInfofi=newFileInfo(Path.Combine(di.FullName,"file"));fi.OpenWrite().Dispose();fi.LastWriteTimeUtc=DateTime.UtcNow;Directory.Delete(di.FullName,recursive:true);fi.LastWriteTimeUtc=DateTime.UtcNow;

On Linux, the last statement currently throws FileNotFoundException. And with the change, it throws DirectoryNotFoundException as on Windows.

tmds · 2022-07-08T08:52:51Z

/__w/1/s/src/libraries/Common/src/Interop/Unix/System.Native/Interop.Stat.cs(65,37): error SYSLIB1053: 'LibraryImportAttribute.StringMarshalling=StringMarshalling.Utf8' has no equivalent in 'DllImportAtttribute' and will not be forwarded [/__w/1/s/src/libraries/System.IO.Ports/src/System.IO.Ports.csproj]

@dotnet/interop-contrib what does this error mean? And what can I do to fix it?

jkoritzinsky · 2022-07-08T16:05:05Z

System.IO.Ports supports downlevel TFMs. On those TFMs, we try to have the generator generate an old-style DllImport. Some functionality cannot be translated automatically to the old system. I’d recommend that you use an #if NET7_0_OR_GREATER condition around the LibraryImport method and have an old DllImport style P/Invoke for downlevel TFMs.

tmds · 2022-07-29T10:22:45Z

CI fail is unrelated.

@dotnet/area-system-io ptal.

tmds · 2022-08-05T05:13:38Z

@adamsitnik @jeffhandley ptal.

jozkee · 2022-08-15T16:49:55Z

- // Windows distinguishes between whether the directory or the file isn't found,
- // and throws a different exception in these cases. We attempt to approximate that
- // here; there is a race condition here, where something could change between
- // when the error occurs and our checks, but it's the best we can do, and the
- // worst case in such a race condition (which could occur if the file system is
- // being manipulated concurrently with these checks) is that we throw a
- // FileNotFoundException instead of DirectoryNotFoundException.


Is it worth keeping this comment inside GetExceptionForIoErrno?

jozkee · 2022-08-15T16:53:58Z

+ // macOS returns ENOTDIR when the path refers to a file and ends with '/'.
+ // Trim the separator so we get EEXIST instead.
+ ReadOnlySpan<char> path = PathInternal.TrimEndingDirectorySeparator(fullPath.AsSpan());
+ int result = Interop.Sys.MkDir(path, (int)unixCreateMode);


I assume there is no test that ensures we don't get DirectoryNotFoundException when the path ends with '/'? Could you please add it?

jozkee · 2022-08-15T17:08:47Z


 if (attributes == (FileAttributes)(-1))
-FileSystemInfo.ThrowNotFound(fullPath);
+throw Interop.GetExceptionForIoErrno(new Interop.ErrorInfo(Interop.Error.ENOENT), fullPath);


Suggested change
throwInterop.GetExceptionForIoErrno(newInterop.ErrorInfo(Interop.Error.ENOENT),fullPath);
throwInterop.GetExceptionForIoErrno(Interop.Error.ENOENT.Info(),fullPath);

jozkee · 2022-08-15T17:09:59Z


 if (mode == (UnixFileMode)(-1))
-FileSystemInfo.ThrowNotFound(fullPath);
+throw Interop.GetExceptionForIoErrno(new Interop.ErrorInfo(Interop.Error.ENOENT), fullPath);


Suggested change
throwInterop.GetExceptionForIoErrno(newInterop.ErrorInfo(Interop.Error.ENOENT),fullPath);
throwInterop.GetExceptionForIoErrno(Interop.Error.ENOENT.Info(),fullPath);

directhex · 2022-08-15T18:14:31Z

This appears to have broken main, at least Android AOT builds:

/home/directhex/Projects/runtime/src/libraries/Common/src/Interop/Unix/Interop.IOErrors.cs(53,70): error CS1739: The best overload for 'ThrowExceptionForIoErrno' does not have a parameter named 'isDirectory' [/home/directhex/Projects/runtime/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]

BruceForstall · 2022-08-15T18:44:44Z

@jozkee This seems to have broken many, many builds. E.g., https://dev.azure.com/dnceng/public/_build/results?buildId=1944354&view=results

radical · 2022-08-15T18:52:00Z

Almost everything - https://dev.azure.com/dnceng/public/_build/results?buildId=1944413&view=results

jozkee · 2022-08-15T18:58:46Z

Sorry for that, I don't know why that was not caught by this PR's build.

radical · 2022-08-15T19:01:27Z

Sorry for that, I don't know why that was not caught by this PR's build.

The breaking call site was added a few days ago. And the CI build here was 18 days ago. that's why it didn't get caught.

danmoseley · 2022-08-15T19:03:18Z

It is because the validation ran 18 days ago. In the meantime this change went in, and collided. It happens, but for something touching common code like this it might be good to ensure validation is fresher.

commit 9fd407a
Author: Eric Erhardt eric.erhardt@microsoft.com
Date: Wed Aug 10 07:43:04 2022 -0500
Add Directory.CreateTempSubdirectory (#73408)

ghost added area-System.IO community-contribution Indicates that the PR has been added by a community member labels Jun 13, 2022

tmds mentioned this pull request Jun 13, 2022
Implement UnixFileMode APIs #69980
Merged

tmds added 2 commits June 13, 2022 16:36

Fix OSX test failure.
08eeb30

Fix wasm build.
a0a0913

tmds added 2 commits June 24, 2022 10:05

Merge remote-tracking branch 'origin/main' into dir_not_found
eb4e464

Review usage of CheckIo/GetExceptionForIoErrno.
5cef5a6

tmds commented Jun 24, 2022
View reviewed changes

tmds added 2 commits July 29, 2022 06:22

Call Directory.Exists instead of using Stat.
e68737e

Merge branch 'main' into dir_not_found
3f447b3

tmds requested a review from adamsitnik August 10, 2022 13:40

jozkee approved these changes Aug 15, 2022
View reviewed changes

jozkee merged commit 68139cd into dotnet:mainAug 15, 2022

directhex added a commit to directhex/runtime that referenced this pull request Aug 15, 2022
Fix breakage fromdotnet#70661
ba55e24

akoeplinger pushed a commit that referenced this pull request Aug 15, 2022
Fix breakage from#70661(#73962)
63e7cba

carlossanlop mentioned this pull request Aug 15, 2022
Product dependencies in Versions.props must be on the latest patch version for libraries dependencies #73871
Merged

ghost locked as resolved and limited conversation to collaborators Sep 15, 2022

	throwInterop.GetExceptionForIoErrno(newInterop.ErrorInfo(Interop.Error.ENOENT),fullPath);
	throwInterop.GetExceptionForIoErrno(Interop.Error.ENOENT.Info(),fullPath);

Uh oh!

Conversation

tmds commented Jun 13, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jun 13, 2022

Uh oh!

tmds commented Jun 13, 2022

Uh oh!

tmds commented Jun 24, 2022

Uh oh!

tmdsJun 24, 2022

Choose a reason for hiding this comment

Uh oh!

tmds commented Jul 5, 2022

Uh oh!

tmds commented Jul 8, 2022

Uh oh!

jkoritzinsky commented Jul 8, 2022

Uh oh!

tmds commented Jul 29, 2022

Uh oh!

tmds commented Aug 5, 2022

Uh oh!

jozkeeAug 15, 2022

Choose a reason for hiding this comment

Uh oh!

jozkeeAug 15, 2022

Choose a reason for hiding this comment

Uh oh!

jozkeeAug 15, 2022

Choose a reason for hiding this comment

Uh oh!

jozkeeAug 15, 2022

Choose a reason for hiding this comment

Uh oh!

directhex commented Aug 15, 2022

Uh oh!

BruceForstall commented Aug 15, 2022

Uh oh!

radical commented Aug 15, 2022

Uh oh!

jozkee commented Aug 15, 2022

Uh oh!

radical commented Aug 15, 2022

Uh oh!

danmoseley commented Aug 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

tmds commented Jun 13, 2022•
edited
Loading