address code review feedback

src/libraries/System.IO.FileSystem/tests/FileStream/SafeFileHandle.cs

@@ -37,6 +37,19 @@ public void DisposeClosesHandle()
             }
         }
 
+        [Fact]
+        public void DisposingBufferedStreamThatWrapsAFileStreamWhichHasBennClosedViaSafeFileHandleCloseDoesNotThrow()
+        {
+            using (FileStream fs = new FileStream(GetTestFilePath(), FileMode.Create))
+            {
+                var bufferedStream = new BufferedStream(fs, 100);
+
+                fs.SafeFileHandle.Dispose();
+
+                bufferedStream.Dispose(); // must not throw
+            }
+        }
+
         [Fact]
         public void AccessFlushesFileClosesHandle()
         {

src/libraries/System.IO.FileSystem/tests/FileStream/WriteAsync.cs

@@ -220,9 +220,17 @@ public async Task ManyConcurrentWriteAsyncs_OuterLoop(
                 {
                     writes[i] = WriteAsync(fs, expectedData, i * writeSize, writeSize, cancellationToken);
                     Assert.Null(writes[i].Exception);
-                    if (useAsync && PlatformDetection.IsLegacyFileStreamEnabled)
+                    if (useAsync)
                     {
-                        Assert.Equal((i + 1) * writeSize, fs.Position);
+                        // To ensure that the buffer of a FileStream opened for async IO is flushed
+                        // by FlushAsync in asynchronous way, we aquire a lock for every buffered WriteAsync.
+                        // The side effect of this is that the Position of FileStream is not updated until
+                        // the lock is released by a previous operation.
+                        // So now all WriteAsync calls should be awaited before starting another async file operation.
+                        if (PlatformDetection.IsLegacyFileStreamEnabled)
+                        {
+                            Assert.Equal((i + 1) * writeSize, fs.Position);
+                        }
                     }
                 }
 

src/libraries/System.Private.CoreLib/src/System/IO/AsyncWindowsFileStreamStrategy.cs

@@ -27,7 +27,8 @@ internal AsyncWindowsFileStreamStrategy(string path, FileMode mode, FileAccess a
 
         public override ValueTask DisposeAsync()
         {
-            // the order matters, let the base class Dispose handle first
+            // the base class must dispose ThreadPoolBinding and FileHandle
+            // before _preallocatedOverlapped is disposed
             ValueTask result = base.DisposeAsync();
             Debug.Assert(result.IsCompleted, "the method must be sync, as it performs no flushing");
 
@@ -38,7 +39,8 @@ public override ValueTask DisposeAsync()
 
         protected override void Dispose(bool disposing)
         {
-            // the order matters, let the base class Dispose handle first
+            // the base class must dispose ThreadPoolBinding and FileHandle
+            // before _preallocatedOverlapped is disposed
             base.Dispose(disposing);
 
             _preallocatedOverlapped?.Dispose();
@@ -389,9 +391,9 @@ private async Task AsyncModeCopyToAsync(Stream destination, int bufferSize, Canc
 
             try
             {
-#pragma warning disable CA2007 // Consider calling ConfigureAwait on the awaited task
-                await FileStreamHelpers.AsyncModeCopyToAsync(_fileHandle, _path, canSeek, _filePosition, destination, bufferSize, cancellationToken);
-#pragma warning restore CA2007 // Consider calling ConfigureAwait on the awaited task
+                await FileStreamHelpers
+                    .AsyncModeCopyToAsync(_fileHandle, _path, canSeek, _filePosition, destination, bufferSize, cancellationToken)
+                    .ConfigureAwait(false);
             }
             finally
             {

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

@@ -33,7 +33,7 @@ internal BufferedFileStreamStrategy(FileStreamStrategy strategy, int bufferSize)
 
         public override long Position
         {
-            get => _bufferedStream.GetPositionWithoutFlushing();
+            get => _bufferedStream.Position;
             set => _bufferedStream.Position = value;
         }
 
@@ -106,6 +106,9 @@ public override Task FlushAsync(CancellationToken cancellationToken)
         public override Task CopyToAsync(Stream destination, int bufferSize, CancellationToken cancellationToken)
             => _bufferedStream.CopyToAsync(destination, bufferSize, cancellationToken);
 
+        public override void CopyTo(Stream destination, int bufferSize)
+            => _bufferedStream.CopyTo(destination, bufferSize);
+
         public override ValueTask DisposeAsync()
             => _bufferedStream.DisposeAsync();
 

src/libraries/System.Private.CoreLib/src/System/IO/BufferedStream.cs

@@ -97,39 +97,31 @@ internal BufferedStream(Stream stream, int bufferSize, bool actLikeFileStream) :
         private void EnsureNotClosed()
         {
             if (_stream == null)
-                Throw();
-
-            static void Throw() => throw new ObjectDisposedException(null, SR.ObjectDisposed_StreamClosed);
+                ThrowHelper.ThrowObjectDisposedException_StreamClosed(null);
         }
 
         private void EnsureCanSeek()
         {
             Debug.Assert(_stream != null);
 
             if (!_stream.CanSeek)
-                Throw();
-
-            static void Throw() => throw new NotSupportedException(SR.NotSupported_UnseekableStream);
+                ThrowHelper.ThrowNotSupportedException_UnseekableStream();
         }
 
         private void EnsureCanRead()
         {
             Debug.Assert(_stream != null);
 
             if (!_stream.CanRead)
-                Throw();
-
-            static void Throw() => throw new NotSupportedException(SR.NotSupported_UnreadableStream);
+                ThrowHelper.ThrowNotSupportedException_UnreadableStream();
         }
 
         private void EnsureCanWrite()
         {
             Debug.Assert(_stream != null);
 
             if (!_stream.CanWrite)
-                Throw();
-
-            static void Throw() => throw new NotSupportedException(SR.NotSupported_UnwritableStream);
+                ThrowHelper.ThrowNotSupportedException_UnwritableStream();
         }
 
         private void EnsureShadowBufferAllocated()
@@ -224,7 +216,6 @@ public override long Length
             }
         }
 
-        // this method exists to keep old FileStream behaviour and don't perform a Flush when getting Length
         internal long GetLengthWithoutFlushing()
         {
             Debug.Assert(_actLikeFileStream);
@@ -233,6 +224,9 @@ internal long GetLengthWithoutFlushing()
 
             long len = _stream!.Length;
 
+            // If we're writing near the end of the file, we must include our
+            // internal buffer in our Length calculation.  Don't flush because
+            // we use the length of the file in our async write method.
             if (_writePos > 0 && _stream!.Position + _writePos > len)
                 len = _writePos + _stream!.Position;
 
@@ -252,7 +246,7 @@ public override long Position
             set
             {
                 if (value < 0)
-                    throw new ArgumentOutOfRangeException(nameof(value), SR.ArgumentOutOfRange_NeedNonNegNum);
+                    ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.value, ExceptionResource.ArgumentOutOfRange_NeedNonNegNum);
 
                 EnsureNotClosed();
                 EnsureCanSeek();
@@ -266,17 +260,6 @@ public override long Position
             }
         }
 
-        // this method exists to keep old FileStream behaviour and don't perform a Flush when getting Position
-        internal long GetPositionWithoutFlushing()
-        {
-            Debug.Assert(_actLikeFileStream);
-
-            EnsureNotClosed();
-            EnsureCanSeek();
-
-            return (_stream!.Position - _readLen) + _readPos + _writePos;
-        }
-
         internal void DisposeInternal(bool disposing) => Dispose(disposing);
 
         protected override void Dispose(bool disposing)
@@ -309,6 +292,9 @@ protected override void Dispose(bool disposing)
 
                 // Call base.Dispose(bool) to cleanup async IO resources
                 base.Dispose(disposing);
+
+                // ensure that all positions are set to 0
+                Debug.Assert(_writePos == 0 && _readPos == 0 && _readLen == 0);
             }
         }
 
@@ -339,12 +325,16 @@ public override async ValueTask DisposeAsync()
                 {
                     _buffer = null;
                 }
+
+                // ensure that all positions are set to 0
+                Debug.Assert(_writePos == 0 && _readPos == 0 && _readLen == 0);
             }
         }
 
         public override void Flush() => Flush(true);
 
-        internal void Flush(bool performActualFlush)
+        // flushUnderlyingStream can be set to false by BufferedFileStreamStrategy.Flush(bool flushToDisk)
+        internal void Flush(bool flushUnderlyingStream)
         {
             EnsureNotClosed();
 
@@ -356,7 +346,7 @@ internal void Flush(bool performActualFlush)
                 // so to avoid getting exception here, we just ensure that we can Write before doing it
                 if (_stream!.CanWrite)
                 {
-                    FlushWrite(performActualFlush);
+                    FlushWrite(flushUnderlyingStream);
                     Debug.Assert(_writePos == 0 && _readPos == 0 && _readLen == 0);
                     return;
                 }
@@ -376,7 +366,7 @@ internal void Flush(bool performActualFlush)
                 // User streams may have opted to throw from Flush if CanWrite is false (although the abstract Stream does not do so).
                 // However, if we do not forward the Flush to the underlying stream, we may have problems when chaining several streams.
                 // Let us make a best effort attempt:
-                if (performActualFlush && _stream.CanWrite)
+                if (flushUnderlyingStream && _stream.CanWrite)
                     _stream.Flush();
 
                 // If the Stream was seekable, then we should have called FlushRead which resets _readPos & _readLen.
@@ -385,7 +375,7 @@ internal void Flush(bool performActualFlush)
             }
 
             // We had no data in the buffer, but we still need to tell the underlying stream to flush.
-            if (performActualFlush && _stream!.CanWrite)
+            if (flushUnderlyingStream && _stream!.CanWrite)
                 _stream.Flush();
 
             _writePos = _readPos = _readLen = 0;
@@ -487,11 +477,12 @@ private void ClearReadBufferBeforeWrite()
             // However, since the user did not call a method that is intuitively expected to seek, a better message is in order.
             // Ideally, we would throw an InvalidOperation here, but for backward compat we have to stick with NotSupported.
             if (!_stream.CanSeek)
-                Throw();
+                ThrowNotSupported_CannotWriteToBufferedStreamIfReadBufferCannotBeFlushed();
 
             FlushRead();
 
-            static void Throw() => throw new NotSupportedException(SR.NotSupported_CannotWriteToBufferedStreamIfReadBufferCannotBeFlushed);
+            static void ThrowNotSupported_CannotWriteToBufferedStreamIfReadBufferCannotBeFlushed()
+                => throw new NotSupportedException(SR.NotSupported_CannotWriteToBufferedStreamIfReadBufferCannotBeFlushed);
         }
 
         private void FlushWrite(bool performActualFlush)

src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs

@@ -403,6 +403,8 @@ public override long Position
 
         public override ValueTask DisposeAsync() => _strategy.DisposeAsync();
 
+        public override void CopyTo(Stream destination, int bufferSize) => _strategy.CopyTo(destination, bufferSize);
+
         public override Task CopyToAsync(Stream destination, int bufferSize, CancellationToken cancellationToken)
             => _strategy.CopyToAsync(destination, bufferSize, cancellationToken);
 

src/libraries/System.Private.CoreLib/src/System/IO/LegacyFileStreamStrategy.Windows.cs

@@ -1153,9 +1153,9 @@ private async Task AsyncModeCopyToAsync(Stream destination, int bufferSize, Canc
 
             try
             {
-#pragma warning disable CA2007 // Consider calling ConfigureAwait on the awaited task
-                await FileStreamHelpers.AsyncModeCopyToAsync(_fileHandle, _path, canSeek, _filePosition, destination, bufferSize, cancellationToken);
-#pragma warning restore CA2007 // Consider calling ConfigureAwait on the awaited task
+                await FileStreamHelpers
+                    .AsyncModeCopyToAsync(_fileHandle, _path, canSeek, _filePosition, destination, bufferSize, cancellationToken)
+                    .ConfigureAwait(false);
             }
             finally
             {

src/libraries/System.Private.CoreLib/src/System/IO/WindowsFileStreamStrategy.cs

@@ -19,21 +19,13 @@ internal abstract class WindowsFileStreamStrategy : FileStreamStrategy
         protected const int ERROR_IO_PENDING = 997;
 
         protected readonly SafeFileHandle _fileHandle; // only ever null if ctor throws
-
-        /// <summary>Whether the file is opened for reading, writing, or both.</summary>
-        private readonly FileAccess _access;
-
-        /// <summary>The path to the opened file.</summary>
-        protected readonly string? _path;
+        protected readonly string? _path; // The path to the opened file.
+        private readonly FileAccess _access; // What file was opened for.
+        private readonly bool _canSeek; // Whether can seek (file) or not (pipe).
+        private readonly bool _isPipe; // Whether to disable async buffering code.
 
         protected long _filePosition;
-
-        private readonly bool _canSeek;
-        private readonly bool _isPipe;      // Whether to disable async buffering code.
-
-        /// <summary>Whether the file stream's handle has been exposed.</summary>
-        protected bool _exposedHandle;
-
+        protected bool _exposedHandle; // Whether the file stream's handle has been exposed.
         private long _appendStart; // When appending, prevent overwriting file.
 
         internal WindowsFileStreamStrategy(SafeFileHandle handle, FileAccess access)

src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs

@@ -307,6 +307,24 @@ internal static void ThrowNotSupportedException(ExceptionResource resource)
             throw new NotSupportedException(GetResourceString(resource));
         }
 
+        [DoesNotReturn]
+        internal static void ThrowNotSupportedException_UnseekableStream()
+        {
+            throw new NotSupportedException(SR.NotSupported_UnseekableStream);
+        }
+
+        [DoesNotReturn]
+        internal static void ThrowNotSupportedException_UnreadableStream()
+        {
+            throw new NotSupportedException(SR.NotSupported_UnreadableStream);
+        }
+
+        [DoesNotReturn]
+        internal static void ThrowNotSupportedException_UnwritableStream()
+        {
+            throw new NotSupportedException(SR.NotSupported_UnwritableStream);
+        }
+
         [DoesNotReturn]
         internal static void ThrowUnauthorizedAccessException(ExceptionResource resource)
         {
@@ -319,6 +337,18 @@ internal static void ThrowObjectDisposedException(string objectName, ExceptionRe
             throw new ObjectDisposedException(objectName, GetResourceString(resource));
         }
 
+        [DoesNotReturn]
+        internal static void ThrowObjectDisposedException_StreamClosed(string? objectName)
+        {
+            throw new ObjectDisposedException(objectName, SR.ObjectDisposed_StreamClosed);
+        }
+
+        [DoesNotReturn]
+        internal static void ThrowObjectDisposedException_FileClosed()
+        {
+            throw new ObjectDisposedException(null, SR.ObjectDisposed_FileClosed);
+        }
+
         [DoesNotReturn]
         internal static void ThrowObjectDisposedException(ExceptionResource resource)
         {