Address PR feedback

src/libraries/System.IO.FileSystem/tests/Net5CompatTests/System.IO.FileSystem.Net5Compat.Tests.csproj

@@ -1,9 +1,8 @@
-<Project Sdk="Microsoft.NET.Sdk">
+<Project Sdk="Microsoft.NET.Sdk">
   <PropertyGroup>
     <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
     <IncludeRemoteExecutor>true</IncludeRemoteExecutor>
-    <!-- Windows is currently the only OS for which we provide a new strategy, so we test the Net5Compat only for Windows -->
-    <TargetFrameworks>$(NetCoreAppCurrent)-windows</TargetFrameworks>
+    <TargetFrameworks>$(NetCoreAppCurrent)-windows;$(NetCoreAppCurrent)-Unix</TargetFrameworks>
 
     <WasmXHarnessMonoArgs>--working-dir=/test-dir</WasmXHarnessMonoArgs>
   </PropertyGroup>

src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs

@@ -29,8 +29,6 @@ private SafeFileHandle(bool ownsHandle)
 
         internal bool CanSeek => !IsClosed && GetCanSeek();
 
-        internal bool IsPipe => false;
-
         internal ThreadPoolBoundHandle? ThreadPoolBinding => null;
 
         internal void EnsureThreadPoolBindingInitialized() { /* nop */ }

src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs

@@ -24,8 +24,6 @@ public SafeFileHandle() : base(true)
 
         internal bool CanSeek => !IsClosed && GetFileType() == Interop.Kernel32.FileTypes.FILE_TYPE_DISK;
 
-        internal bool IsPipe => GetFileType() == Interop.Kernel32.FileTypes.FILE_TYPE_PIPE;
-
         internal ThreadPoolBoundHandle? ThreadPoolBinding { get; set; }
 
         internal static unsafe SafeFileHandle Open(string fullPath, FileMode mode, FileAccess access, FileShare share, FileOptions options, long preallocationSize)

src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Unix.cs

@@ -28,7 +28,10 @@ internal static unsafe int ReadAtOffset(SafeFileHandle handle, Span<byte> buffer
         {
             fixed (byte* bufPtr = &MemoryMarshal.GetReference(buffer))
             {
-                int result = fileOffset >= 0 ?
+                // The Windows implementation uses ReadFile, which ignores the offset if the handle
+                // isn't seekable.  We do the same manually with PRead vs Read, in order to enable
+                // the function to be used by FileStream for all the same situations.
+                int result = handle.CanSeek ?
                     Interop.Sys.PRead(handle, bufPtr, buffer.Length, fileOffset) :
                     Interop.Sys.Read(handle, bufPtr, buffer.Length);
                 FileStreamHelpers.CheckFileCall(result, handle.Path);
@@ -80,7 +83,10 @@ internal static unsafe int WriteAtOffset(SafeFileHandle handle, ReadOnlySpan<byt
         {
             fixed (byte* bufPtr = &MemoryMarshal.GetReference(buffer))
             {
-                int result = fileOffset >= 0 ?
+                // The Windows implementation uses WriteFile, which ignores the offset if the handle
+                // isn't seekable.  We do the same manually with PWrite vs Write, in order to enable
+                // the function to be used by FileStream for all the same situations.
+                int result = handle.CanSeek ?
                     Interop.Sys.PWrite(handle, bufPtr, buffer.Length, fileOffset) :
                     Interop.Sys.Write(handle, bufPtr, buffer.Length);
                 FileStreamHelpers.CheckFileCall(result, handle.Path);

src/libraries/System.Private.CoreLib/src/System/IO/Strategies/BufferedFileStreamStrategy.cs

@@ -247,7 +247,7 @@ private int ReadSpan(Span<byte> destination, ArraySegment<byte> arraySegment)
 
             // If we are reading from a device with no clear EOF like a
             // serial port or a pipe, this will cause us to block incorrectly.
-            if (!_strategy.IsPipe)
+            if (_strategy.CanSeek)
             {
                 // If we hit the end of the buffer and didn't have enough bytes, we must
                 // read some more from the underlying stream.  However, if we got
@@ -340,9 +340,9 @@ public override ValueTask<int> ReadAsync(Memory<byte> buffer, CancellationToken
             Debug.Assert((_readPos == 0 && _readLen == 0 && _writePos >= 0) || (_writePos == 0 && _readPos <= _readLen),
                 "We're either reading or writing, but not both.");
 
-            if (_strategy.IsPipe) // pipes have a very limited support for buffering
+            if (!_strategy.CanSeek)
             {
-                return ReadFromPipeAsync(buffer, cancellationToken);
+                return ReadFromNonSeekableAsync(buffer, cancellationToken);
             }
 
             SemaphoreSlim semaphore = EnsureAsyncActiveSemaphoreInitialized();
@@ -383,9 +383,9 @@ public override ValueTask<int> ReadAsync(Memory<byte> buffer, CancellationToken
             return ReadAsyncSlowPath(semaphoreLockTask, buffer, cancellationToken);
         }
 
-        private async ValueTask<int> ReadFromPipeAsync(Memory<byte> destination, CancellationToken cancellationToken)
+        private async ValueTask<int> ReadFromNonSeekableAsync(Memory<byte> destination, CancellationToken cancellationToken)
         {
-            Debug.Assert(_strategy.IsPipe);
+            Debug.Assert(!_strategy.CanSeek);
 
             // Employ async waiting based on the same synchronization used in BeginRead of the abstract Stream.
             await EnsureAsyncActiveSemaphoreInitialized().WaitAsync(cancellationToken).ConfigureAwait(false);
@@ -426,7 +426,7 @@ private async ValueTask<int> ReadFromPipeAsync(Memory<byte> destination, Cancell
         private async ValueTask<int> ReadAsyncSlowPath(Task semaphoreLockTask, Memory<byte> buffer, CancellationToken cancellationToken)
         {
             Debug.Assert(_asyncActiveSemaphore != null);
-            Debug.Assert(!_strategy.IsPipe);
+            Debug.Assert(_strategy.CanSeek);
 
             // Employ async waiting based on the same synchronization used in BeginRead of the abstract Stream.
             await semaphoreLockTask.ConfigureAwait(false);
@@ -633,13 +633,13 @@ public override ValueTask WriteAsync(ReadOnlyMemory<byte> buffer, CancellationTo
             Debug.Assert(!_strategy.IsClosed, "FileStream ensures that strategy is not closed");
             Debug.Assert((_readPos == 0 && _readLen == 0 && _writePos >= 0) || (_writePos == 0 && _readPos <= _readLen),
                 "We're either reading or writing, but not both.");
-            Debug.Assert(!_strategy.IsPipe || (_readPos == 0 && _readLen == 0),
+            Debug.Assert(_strategy.CanSeek || (_readPos == 0 && _readLen == 0),
                 "Win32FileStream must not have buffered data here!  Pipes should be unidirectional.");
 
-            if (_strategy.IsPipe)
+            if (!_strategy.CanSeek)
             {
                 // avoid async buffering with pipes, as doing so can lead to deadlocks (see comments in ReadFromPipeAsync)
-                return WriteToPipeAsync(buffer, cancellationToken);
+                return WriteToNonSeekableAsync(buffer, cancellationToken);
             }
 
             SemaphoreSlim semaphore = EnsureAsyncActiveSemaphoreInitialized();
@@ -690,9 +690,9 @@ public override ValueTask WriteAsync(ReadOnlyMemory<byte> buffer, CancellationTo
             return WriteAsyncSlowPath(semaphoreLockTask, buffer, cancellationToken);
         }
 
-        private async ValueTask WriteToPipeAsync(ReadOnlyMemory<byte> source, CancellationToken cancellationToken)
+        private async ValueTask WriteToNonSeekableAsync(ReadOnlyMemory<byte> source, CancellationToken cancellationToken)
         {
-            Debug.Assert(_strategy.IsPipe);
+            Debug.Assert(!_strategy.CanSeek);
 
             await EnsureAsyncActiveSemaphoreInitialized().WaitAsync(cancellationToken).ConfigureAwait(false);
             try
@@ -708,7 +708,7 @@ private async ValueTask WriteToPipeAsync(ReadOnlyMemory<byte> source, Cancellati
         private async ValueTask WriteAsyncSlowPath(Task semaphoreLockTask, ReadOnlyMemory<byte> source, CancellationToken cancellationToken)
         {
             Debug.Assert(_asyncActiveSemaphore != null);
-            Debug.Assert(!_strategy.IsPipe);
+            Debug.Assert(_strategy.CanSeek);
 
             await semaphoreLockTask.ConfigureAwait(false);
             try

src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamStrategy.cs

@@ -17,8 +17,6 @@ internal abstract class FileStreamStrategy : Stream
 
         internal abstract bool IsClosed { get; }
 
-        internal virtual bool IsPipe => false;
-
         internal abstract void Lock(long position, long length);
 
         internal abstract void Unlock(long position, long length);

src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.Windows.cs

@@ -316,7 +316,7 @@ private int ReadSpan(Span<byte> destination)
 
             // If we are reading from a device with no clear EOF like a
             // serial port or a pipe, this will cause us to block incorrectly.
-            if (!_fileHandle.IsPipe)
+            if (_fileHandle.CanSeek)
             {
                 // If we hit the end of the buffer and didn't have enough bytes, we must
                 // read some more from the underlying stream.  However, if we got
@@ -581,7 +581,7 @@ private unsafe void WriteCore(ReadOnlySpan<byte> source)
 
             Debug.Assert((_readPos == 0 && _readLength == 0 && _writePos >= 0) || (_writePos == 0 && _readPos <= _readLength), "We're either reading or writing, but not both.");
 
-            if (_fileHandle.IsPipe)
+            if (!_fileHandle.CanSeek)
             {
                 // Pipes are tricky, at least when you have 2 different pipes
                 // that you want to use simultaneously.  When redirecting stdout
@@ -612,7 +612,7 @@ private unsafe void WriteCore(ReadOnlySpan<byte> source)
                 }
             }
 
-            Debug.Assert(!_fileHandle.IsPipe, "Should not be a pipe.");
+            Debug.Assert(_fileHandle.CanSeek, "Should be seekable");
 
             // Handle buffering.
             if (_writePos > 0) FlushWriteBuffer();
@@ -791,12 +791,12 @@ private ValueTask WriteAsyncInternal(ReadOnlyMemory<byte> source, CancellationTo
         {
             Debug.Assert(_useAsyncIO);
             Debug.Assert((_readPos == 0 && _readLength == 0 && _writePos >= 0) || (_writePos == 0 && _readPos <= _readLength), "We're either reading or writing, but not both.");
-            Debug.Assert(!_fileHandle.IsPipe || (_readPos == 0 && _readLength == 0), "Win32FileStream must not have buffered data here!  Pipes should be unidirectional.");
+            Debug.Assert(_fileHandle.CanSeek || (_readPos == 0 && _readLength == 0), "Win32FileStream must not have buffered data here!  Pipes should be unidirectional.");
 
             if (!CanWrite) ThrowHelper.ThrowNotSupportedException_UnwritableStream();
 
             bool writeDataStoredInBuffer = false;
-            if (!_fileHandle.IsPipe) // avoid async buffering with pipes, as doing so can lead to deadlocks (see comments in ReadInternalAsyncCore)
+            if (_fileHandle.CanSeek) // avoid async buffering with non-seekable files (e.g. pipes), as doing so can lead to deadlocks (see comments in ReadInternalAsyncCore)
             {
                 // Ensure the buffer is clear for writing
                 if (_writePos == 0)

src/libraries/System.Private.CoreLib/src/System/IO/Strategies/OSFileStreamStrategy.cs

@@ -128,8 +128,6 @@ public sealed override long Position
 
         internal sealed override bool IsClosed => _fileHandle.IsClosed;
 
-        internal sealed override bool IsPipe => _fileHandle.IsPipe;
-
         // Flushing is the responsibility of BufferedFileStreamStrategy
         internal sealed override SafeFileHandle SafeFileHandle
         {
@@ -265,7 +263,7 @@ public sealed override int Read(Span<byte> buffer)
                 ThrowHelper.ThrowNotSupportedException_UnreadableStream();
             }
 
-            int r = RandomAccess.ReadAtOffset(_fileHandle, buffer, CanSeek ? _filePosition : -1);
+            int r = RandomAccess.ReadAtOffset(_fileHandle, buffer, _filePosition);
             Debug.Assert(r >= 0, $"RandomAccess.ReadAtOffset returned {r}.");
             _filePosition += r;
 
@@ -289,7 +287,7 @@ public sealed override void Write(ReadOnlySpan<byte> buffer)
                 ThrowHelper.ThrowNotSupportedException_UnwritableStream();
             }
 
-            int r = RandomAccess.WriteAtOffset(_fileHandle, buffer, CanSeek ? _filePosition : -1);
+            int r = RandomAccess.WriteAtOffset(_fileHandle, buffer, _filePosition);
             Debug.Assert(r >= 0, $"RandomAccess.WriteAtOffset returned {r}.");
             _filePosition += r;
 

src/libraries/System.Private.CoreLib/src/System/IO/Strategies/UnixFileStreamStrategy.cs

@@ -58,9 +58,14 @@ public override void EndWrite(IAsyncResult asyncResult) =>
             TaskToApm.End(asyncResult);
 
         public override Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) =>
-            WriteAsync(new ReadOnlyMemory<byte>(buffer, offset, count), cancellationToken).AsTask();
+            WriteAsyncCore(new ReadOnlyMemory<byte>(buffer, offset, count), cancellationToken).AsTask();
 
-        public override ValueTask WriteAsync(ReadOnlyMemory<byte> source, CancellationToken cancellationToken)
+        public override ValueTask WriteAsync(ReadOnlyMemory<byte> source, CancellationToken cancellationToken) =>
+#pragma warning disable CA2012 // The analyzer doesn't know the internal AsValueTask is safe.
+            WriteAsyncCore(source, cancellationToken).AsValueTask();
+#pragma warning restore CA2012
+
+        private ValueTask<int> WriteAsyncCore(ReadOnlyMemory<byte> source, CancellationToken cancellationToken)
         {
             if (!CanWrite)
             {
@@ -74,9 +79,7 @@ public override ValueTask WriteAsync(ReadOnlyMemory<byte> source, CancellationTo
                 _filePosition += source.Length;
             }
 
-#pragma warning disable CA2012 // The analyzer doesn't know the internal AsValueTask is safe.
-            return RandomAccess.WriteAtOffsetAsync(_fileHandle, source, filePositionBefore, cancellationToken).AsValueTask();
-#pragma warning restore CA2012
+            return RandomAccess.WriteAtOffsetAsync(_fileHandle, source, filePositionBefore, cancellationToken);
         }
 
         /// <summary>Provides a reusable ValueTask-backing object for implementing ReadAsync.</summary>