From 3f6b541a874746070fd3f6739c3eb08fd2fd7d4c Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Wed, 10 Mar 2021 13:10:50 +0100 Subject: [PATCH] address code review feedback --- .../tests/FileStream/SafeFileHandle.cs | 13 +++++ .../tests/FileStream/WriteAsync.cs | 12 ++++- .../IO/AsyncWindowsFileStreamStrategy.cs | 12 +++-- .../System/IO/BufferedFileStreamStrategy.cs | 5 +- .../src/System/IO/BufferedStream.cs | 53 ++++++++----------- .../src/System/IO/FileStream.cs | 2 + .../IO/LegacyFileStreamStrategy.Windows.cs | 6 +-- .../System/IO/WindowsFileStreamStrategy.cs | 18 ++----- .../src/System/ThrowHelper.cs | 30 +++++++++++ 9 files changed, 96 insertions(+), 55 deletions(-) diff --git a/src/libraries/System.IO.FileSystem/tests/FileStream/SafeFileHandle.cs b/src/libraries/System.IO.FileSystem/tests/FileStream/SafeFileHandle.cs index 020a25e48871e..92ad0ff503bdc 100644 --- a/src/libraries/System.IO.FileSystem/tests/FileStream/SafeFileHandle.cs +++ b/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() { diff --git a/src/libraries/System.IO.FileSystem/tests/FileStream/WriteAsync.cs b/src/libraries/System.IO.FileSystem/tests/FileStream/WriteAsync.cs index 358cf11310838..fb4c02f0a064a 100644 --- a/src/libraries/System.IO.FileSystem/tests/FileStream/WriteAsync.cs +++ b/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); + } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/AsyncWindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/AsyncWindowsFileStreamStrategy.cs index 8f8f012d1efff..305544d061ef0 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/AsyncWindowsFileStreamStrategy.cs +++ b/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 { diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/BufferedFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/BufferedFileStreamStrategy.cs index bf5c50e2cfbfb..dc04c4bb1724b 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/BufferedFileStreamStrategy.cs +++ b/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(); diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/BufferedStream.cs b/src/libraries/System.Private.CoreLib/src/System/IO/BufferedStream.cs index 66302d9cd18f4..195b3002d2b2c 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/BufferedStream.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/BufferedStream.cs @@ -97,9 +97,7 @@ 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() @@ -107,9 +105,7 @@ 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() @@ -117,9 +113,7 @@ 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() @@ -127,9 +121,7 @@ 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) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs index 6109cf014298c..2af6d8125e20e 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs +++ b/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); diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/LegacyFileStreamStrategy.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/LegacyFileStreamStrategy.Windows.cs index 0e236cfa5097f..ed8f5151992cd 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/LegacyFileStreamStrategy.Windows.cs +++ b/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 { diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/WindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/WindowsFileStreamStrategy.cs index e6f8116ddddb2..89654bfa90551 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/WindowsFileStreamStrategy.cs +++ b/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 - - /// Whether the file is opened for reading, writing, or both. - private readonly FileAccess _access; - - /// The path to the opened file. - 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. - - /// Whether the file stream's handle has been exposed. - 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) diff --git a/src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs b/src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs index 8667c198913bc..ad9f0e563271d 100644 --- a/src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs +++ b/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) {