Fix File.Exists for pipe paths on Windows
#117333
Conversation
dotnet-policy-service
bot
added
the
community-contribution
|
@dotnet/area-system-io Can someone look at the PR? |
|
Me waiting for a review on a commutiny PR: |
|
I was full of expectations, but I had been waiting for a long time. LoL |
|
@lindexi Yeah, same for me. It seems that |
|
@jozkee PTAL |
| /// </item> | ||
| /// </list> | ||
| /// </summary> | ||
| private static bool IsPipePath([NotNullWhen(true)] string? path) |
There was a problem hiding this comment.
Consider moving this method to PathInternal.Windows.cs
| } | ||
|
|
||
| [Fact] | ||
| [PlatformSpecific(TestPlatforms.Windows)] // Windows-specific pipes behavior |
There was a problem hiding this comment.
| [PlatformSpecific(TestPlatforms.Windows)] // Windows-specific pipes behavior | |
| [PlatformSpecific(TestPlatforms.Windows)] |
I think the test name + this attribute is self-explanatory, consider removing these comments.
| [PlatformSpecific(TestPlatforms.Windows)] // Windows-specific pipes behavior | ||
| public void CheckPipeExistsViaFileApi() | ||
| { | ||
| var pipeName = GetNamedPipeServerStreamName(); |
There was a problem hiding this comment.
Just by looking at GetNamedPipeServerStreamName
The IsInAppContainer scenarios will be adding another segment to pipe names meaning IsPipePath as is won't work. It may be tricky to figure out how to test that one.
I suspect is related to UWP cc @adamsitnik @danmoseley.
| var pipeName = GetNamedPipeServerStreamName(); | ||
| new NamedPipeServerStream(pipeName).Dispose(); | ||
| Assert.False(Exists(@$"\\.\pipe\{pipeName}")); |
There was a problem hiding this comment.
| var pipeName = GetNamedPipeServerStreamName(); | |
| new NamedPipeServerStream(pipeName).Dispose(); | |
| Assert.False(Exists(@$"\\.\pipe\{pipeName}")); | |
| Assert.False(Exists(@$"\\.\pipe\{pipeName}")); |
No need to create/delete the pipe?
| } | ||
|
|
||
| ReadOnlySpan<char> pathSpan = path.AsSpan(); | ||
| if (!pathSpan.StartsWith(@"\\")) |
There was a problem hiding this comment.
I think the method should also handle forward slashes.
There was a problem hiding this comment.
FillAttributeInfo also impacts FIle.GetAttributes, can you please add tests for that?
| // 1) '.' or 'serverName' | ||
| // 2) Constant 'pipe' segment | ||
| // 3) Pipe name | ||
| return written == 3 && pathSpan[segments[1]].SequenceEqual("pipe"); |
There was a problem hiding this comment.
I think this should be a case insensitive comparison.
jozkee
added
the
needs-author-action
5 participants
Fixes: #69604
Also while working on a core fix (commit 1) I noticed that
dwFileAttributesis often compared to-1. Windows documentation reffers to it asINVALID_FILE_ATTRIBUTES, so I decided to name this constant for clarity