From 30d8ef7a2ac9d8faa328f5322dd12898cc85869a Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 25 Feb 2021 15:47:15 +0100 Subject: [PATCH 01/20] introduce WindowsFileStreamStrategy --- .../System.Private.CoreLib.Shared.projitems | 3 +- .../System/IO/FileStreamHelpers.Windows.cs | 206 ++++++++++++ .../IO/LegacyFileStreamStrategy.Windows.cs | 176 +---------- .../System/IO/WindowsFileStreamStrategy.cs | 298 ++++++++++++++++++ 4 files changed, 516 insertions(+), 167 deletions(-) create mode 100644 src/libraries/System.Private.CoreLib/src/System/IO/WindowsFileStreamStrategy.cs diff --git a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems index d9900d80412cd2..8e99e53d99b09b 100644 --- a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems +++ b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems @@ -1636,10 +1636,11 @@ + - + diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs index 66dd3cb259e4fc..02e8aa29326836 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs @@ -3,6 +3,7 @@ using System.Diagnostics; using System.Runtime.InteropServices; +using System.Threading; using Microsoft.Win32.SafeHandles; namespace System.IO @@ -144,5 +145,210 @@ private static SafeFileHandle ValidateFileHandle(SafeFileHandle fileHandle, stri fileHandle.IsAsync = useAsyncIO; return fileHandle; } + + internal static unsafe long GetFileLength(SafeFileHandle handle, string? path) + { + Interop.Kernel32.FILE_STANDARD_INFO info; + + if (!Interop.Kernel32.GetFileInformationByHandleEx(handle, Interop.Kernel32.FileStandardInfo, &info, (uint)sizeof(Interop.Kernel32.FILE_STANDARD_INFO))) + { + throw Win32Marshal.GetExceptionForLastWin32Error(path); + } + + return info.EndOfFile; + } + + internal static void FlushToDisk(SafeFileHandle handle, string? path) + { + if (!Interop.Kernel32.FlushFileBuffers(handle)) + { + throw Win32Marshal.GetExceptionForLastWin32Error(path); + } + } + + internal static long Seek(SafeFileHandle handle, string? path, long offset, SeekOrigin origin, bool closeInvalidHandle = false) + { + Debug.Assert(origin >= SeekOrigin.Begin && origin <= SeekOrigin.End, "origin >= SeekOrigin.Begin && origin <= SeekOrigin.End"); + + if (!Interop.Kernel32.SetFilePointerEx(handle, offset, out long ret, (uint)origin)) + { + if (closeInvalidHandle) + { + throw Win32Marshal.GetExceptionForWin32Error(GetLastWin32ErrorAndDisposeHandleIfInvalid(handle), path); + } + else + { + throw Win32Marshal.GetExceptionForLastWin32Error(path); + } + } + + return ret; + } + + private static int GetLastWin32ErrorAndDisposeHandleIfInvalid(SafeFileHandle handle) + { + int errorCode = Marshal.GetLastWin32Error(); + + // If ERROR_INVALID_HANDLE is returned, it doesn't suffice to set + // the handle as invalid; the handle must also be closed. + // + // Marking the handle as invalid but not closing the handle + // resulted in exceptions during finalization and locked column + // values (due to invalid but unclosed handle) in SQL Win32FileStream + // scenarios. + // + // A more mainstream scenario involves accessing a file on a + // network share. ERROR_INVALID_HANDLE may occur because the network + // connection was dropped and the server closed the handle. However, + // the client side handle is still open and even valid for certain + // operations. + // + // Note that _parent.Dispose doesn't throw so we don't need to special case. + // SetHandleAsInvalid only sets _closed field to true (without + // actually closing handle) so we don't need to call that as well. + if (errorCode == Interop.Errors.ERROR_INVALID_HANDLE) + { + handle.Dispose(); + } + + return errorCode; + } + + internal static void Lock(SafeFileHandle handle, string? path, long position, long length) + { + int positionLow = unchecked((int)(position)); + int positionHigh = unchecked((int)(position >> 32)); + int lengthLow = unchecked((int)(length)); + int lengthHigh = unchecked((int)(length >> 32)); + + if (!Interop.Kernel32.LockFile(handle, positionLow, positionHigh, lengthLow, lengthHigh)) + { + throw Win32Marshal.GetExceptionForLastWin32Error(path); + } + } + + internal static void Unlock(SafeFileHandle handle, string? path, long position, long length) + { + int positionLow = unchecked((int)(position)); + int positionHigh = unchecked((int)(position >> 32)); + int lengthLow = unchecked((int)(length)); + int lengthHigh = unchecked((int)(length >> 32)); + + if (!Interop.Kernel32.UnlockFile(handle, positionLow, positionHigh, lengthLow, lengthHigh)) + { + throw Win32Marshal.GetExceptionForLastWin32Error(path); + } + } + + internal static void ValidateFileTypeForNonExtendedPaths(SafeFileHandle handle, string originalPath) + { + if (!PathInternal.IsExtended(originalPath)) + { + // To help avoid stumbling into opening COM/LPT ports by accident, we will block on non file handles unless + // we were explicitly passed a path that has \\?\. GetFullPath() will turn paths like C:\foo\con.txt into + // \\.\CON, so we'll only allow the \\?\ syntax. + + int fileType = Interop.Kernel32.GetFileType(handle); + if (fileType != Interop.Kernel32.FileTypes.FILE_TYPE_DISK) + { + int errorCode = fileType == Interop.Kernel32.FileTypes.FILE_TYPE_UNKNOWN + ? Marshal.GetLastWin32Error() + : Interop.Errors.ERROR_SUCCESS; + + handle.Dispose(); + + if (errorCode != Interop.Errors.ERROR_SUCCESS) + { + throw Win32Marshal.GetExceptionForWin32Error(errorCode); + } + throw new NotSupportedException(SR.NotSupported_FileStreamOnNonFiles); + } + } + } + + internal static void GetFileTypeSpecificInformation(SafeFileHandle handle, out bool canSeek, out bool isPipe) + { + int handleType = Interop.Kernel32.GetFileType(handle); + Debug.Assert(handleType == Interop.Kernel32.FileTypes.FILE_TYPE_DISK + || handleType == Interop.Kernel32.FileTypes.FILE_TYPE_PIPE + || handleType == Interop.Kernel32.FileTypes.FILE_TYPE_CHAR, + "FileStream was passed an unknown file type!"); + + canSeek = handleType == Interop.Kernel32.FileTypes.FILE_TYPE_DISK; + isPipe = handleType == Interop.Kernel32.FileTypes.FILE_TYPE_PIPE; + } + + internal static unsafe void SetLength(SafeFileHandle handle, string? path, long length) + { + var eofInfo = new Interop.Kernel32.FILE_END_OF_FILE_INFO + { + EndOfFile = length + }; + + if (!Interop.Kernel32.SetFileInformationByHandle( + handle, + Interop.Kernel32.FileEndOfFileInfo, + &eofInfo, + (uint)sizeof(Interop.Kernel32.FILE_END_OF_FILE_INFO))) + { + int errorCode = Marshal.GetLastWin32Error(); + if (errorCode == Interop.Errors.ERROR_INVALID_PARAMETER) + throw new ArgumentOutOfRangeException(nameof(length), SR.ArgumentOutOfRange_FileLengthTooBig); + throw Win32Marshal.GetExceptionForWin32Error(errorCode, path); + } + } + + // __ConsoleStream also uses this code. + internal static unsafe int ReadFileNative(SafeFileHandle handle, Span bytes, NativeOverlapped* overlapped, out int errorCode) + { + Debug.Assert(handle != null, "handle != null"); + + int r; + int numBytesRead = 0; + + fixed (byte* p = &MemoryMarshal.GetReference(bytes)) + { + r = overlapped != null ? + Interop.Kernel32.ReadFile(handle, p, bytes.Length, IntPtr.Zero, overlapped) : + Interop.Kernel32.ReadFile(handle, p, bytes.Length, out numBytesRead, IntPtr.Zero); + } + + if (r == 0) + { + errorCode = GetLastWin32ErrorAndDisposeHandleIfInvalid(handle); + return -1; + } + else + { + errorCode = 0; + return numBytesRead; + } + } + + internal static unsafe int WriteFileNative(SafeFileHandle handle, ReadOnlySpan buffer, NativeOverlapped* overlapped, out int errorCode) + { + Debug.Assert(handle != null, "handle != null"); + + int numBytesWritten = 0; + int r; + + fixed (byte* p = &MemoryMarshal.GetReference(buffer)) + { + r = overlapped != null ? + Interop.Kernel32.WriteFile(handle, p, buffer.Length, IntPtr.Zero, overlapped) : + Interop.Kernel32.WriteFile(handle, p, buffer.Length, out numBytesWritten, IntPtr.Zero); + } + + if (r == 0) + { + errorCode = GetLastWin32ErrorAndDisposeHandleIfInvalid(handle); + return -1; + } + else + { + errorCode = 0; + return numBytesWritten; + } + } } } 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 4eaf3d69d0e2fe..b0c3bfb5353ca8 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 @@ -3,7 +3,6 @@ using System.Buffers; using System.Diagnostics; -using System.Runtime.InteropServices; using System.Threading; using System.Threading.Tasks; using Microsoft.Win32.SafeHandles; @@ -53,28 +52,7 @@ internal sealed partial class LegacyFileStreamStrategy : FileStreamStrategy private void Init(FileMode mode, FileShare share, string originalPath, FileOptions options) { - if (!PathInternal.IsExtended(originalPath)) - { - // To help avoid stumbling into opening COM/LPT ports by accident, we will block on non file handles unless - // we were explicitly passed a path that has \\?\. GetFullPath() will turn paths like C:\foo\con.txt into - // \\.\CON, so we'll only allow the \\?\ syntax. - - int fileType = Interop.Kernel32.GetFileType(_fileHandle); - if (fileType != Interop.Kernel32.FileTypes.FILE_TYPE_DISK) - { - int errorCode = fileType == Interop.Kernel32.FileTypes.FILE_TYPE_UNKNOWN - ? Marshal.GetLastWin32Error() - : Interop.Errors.ERROR_SUCCESS; - - _fileHandle.Dispose(); - - if (errorCode != Interop.Errors.ERROR_SUCCESS) - { - throw Win32Marshal.GetExceptionForWin32Error(errorCode); - } - throw new NotSupportedException(SR.NotSupported_FileStreamOnNonFiles); - } - } + FileStreamHelpers.ValidateFileTypeForNonExtendedPaths(_fileHandle, originalPath); // This is necessary for async IO using IO Completion ports via our // managed Threadpool API's. This (theoretically) calls the OS's @@ -139,11 +117,7 @@ private void InitFromHandle(SafeFileHandle handle, FileAccess access, bool useAs private void InitFromHandleImpl(SafeFileHandle handle, bool useAsyncIO) { - int handleType = Interop.Kernel32.GetFileType(handle); - Debug.Assert(handleType == Interop.Kernel32.FileTypes.FILE_TYPE_DISK || handleType == Interop.Kernel32.FileTypes.FILE_TYPE_PIPE || handleType == Interop.Kernel32.FileTypes.FILE_TYPE_CHAR, "FileStream was passed an unknown file type!"); - - _canSeek = handleType == Interop.Kernel32.FileTypes.FILE_TYPE_DISK; - _isPipe = handleType == Interop.Kernel32.FileTypes.FILE_TYPE_PIPE; + FileStreamHelpers.GetFileTypeSpecificInformation(handle, out _canSeek, out _isPipe); // This is necessary for async IO using IO Completion ports via our // managed Threadpool API's. This calls the OS's @@ -189,11 +163,7 @@ public unsafe override long Length { get { - Interop.Kernel32.FILE_STANDARD_INFO info; - - if (!Interop.Kernel32.GetFileInformationByHandleEx(_fileHandle, Interop.Kernel32.FileStandardInfo, &info, (uint)sizeof(Interop.Kernel32.FILE_STANDARD_INFO))) - throw Win32Marshal.GetExceptionForLastWin32Error(_path); - long len = info.EndOfFile; + long len = FileStreamHelpers.GetFileLength(_fileHandle, _path); // If we're writing near the end of the file, we must include our // internal buffer in our Length calculation. Don't flush because @@ -275,13 +245,7 @@ public override async ValueTask DisposeAsync() } } - private void FlushOSBuffer() - { - if (!Interop.Kernel32.FlushFileBuffers(_fileHandle)) - { - throw Win32Marshal.GetExceptionForLastWin32Error(_path); - } - } + private void FlushOSBuffer() => FileStreamHelpers.FlushToDisk(_fileHandle, _path); // Returns a task that flushes the internal write buffer private Task FlushWriteAsync(CancellationToken cancellationToken) @@ -368,22 +332,7 @@ private unsafe void SetLengthCore(long value) Debug.Assert(value >= 0, "value >= 0"); VerifyOSHandlePosition(); - var eofInfo = new Interop.Kernel32.FILE_END_OF_FILE_INFO - { - EndOfFile = value - }; - - if (!Interop.Kernel32.SetFileInformationByHandle( - _fileHandle, - Interop.Kernel32.FileEndOfFileInfo, - &eofInfo, - (uint)sizeof(Interop.Kernel32.FILE_END_OF_FILE_INFO))) - { - int errorCode = Marshal.GetLastWin32Error(); - if (errorCode == Interop.Errors.ERROR_INVALID_PARAMETER) - throw new ArgumentOutOfRangeException(nameof(value), SR.ArgumentOutOfRange_FileLengthTooBig); - throw Win32Marshal.GetExceptionForWin32Error(errorCode, _path); - } + FileStreamHelpers.SetLength(_fileHandle, _path, value); if (_filePosition > value) { @@ -594,22 +543,8 @@ public override long Seek(long offset, SeekOrigin origin) private long SeekCore(SafeFileHandle fileHandle, long offset, SeekOrigin origin, bool closeInvalidHandle = false) { Debug.Assert(!fileHandle.IsClosed && _canSeek, "!fileHandle.IsClosed && _canSeek"); - Debug.Assert(origin >= SeekOrigin.Begin && origin <= SeekOrigin.End, "origin >= SeekOrigin.Begin && origin <= SeekOrigin.End"); - - if (!Interop.Kernel32.SetFilePointerEx(fileHandle, offset, out long ret, (uint)origin)) - { - if (closeInvalidHandle) - { - throw Win32Marshal.GetExceptionForWin32Error(GetLastWin32ErrorAndDisposeHandleIfInvalid(), _path); - } - else - { - throw Win32Marshal.GetExceptionForLastWin32Error(_path); - } - } - _filePosition = ret; - return ret; + return _filePosition = FileStreamHelpers.Seek(fileHandle, _path, offset, origin, closeInvalidHandle); } partial void OnBufferAllocated() @@ -1144,85 +1079,16 @@ private unsafe Task WriteAsyncInternalCore(ReadOnlyMemory source, Cancella // __ConsoleStream also uses this code. private unsafe int ReadFileNative(SafeFileHandle handle, Span bytes, NativeOverlapped* overlapped, out int errorCode) { - Debug.Assert(handle != null, "handle != null"); Debug.Assert((_useAsyncIO && overlapped != null) || (!_useAsyncIO && overlapped == null), "Async IO and overlapped parameters inconsistent in call to ReadFileNative."); - int r; - int numBytesRead = 0; - - fixed (byte* p = &MemoryMarshal.GetReference(bytes)) - { - r = _useAsyncIO ? - Interop.Kernel32.ReadFile(handle, p, bytes.Length, IntPtr.Zero, overlapped) : - Interop.Kernel32.ReadFile(handle, p, bytes.Length, out numBytesRead, IntPtr.Zero); - } - - if (r == 0) - { - errorCode = GetLastWin32ErrorAndDisposeHandleIfInvalid(); - return -1; - } - else - { - errorCode = 0; - return numBytesRead; - } + return FileStreamHelpers.ReadFileNative(handle, bytes, overlapped, out errorCode); } private unsafe int WriteFileNative(SafeFileHandle handle, ReadOnlySpan buffer, NativeOverlapped* overlapped, out int errorCode) { - Debug.Assert(handle != null, "handle != null"); Debug.Assert((_useAsyncIO && overlapped != null) || (!_useAsyncIO && overlapped == null), "Async IO and overlapped parameters inconsistent in call to WriteFileNative."); - int numBytesWritten = 0; - int r; - - fixed (byte* p = &MemoryMarshal.GetReference(buffer)) - { - r = _useAsyncIO ? - Interop.Kernel32.WriteFile(handle, p, buffer.Length, IntPtr.Zero, overlapped) : - Interop.Kernel32.WriteFile(handle, p, buffer.Length, out numBytesWritten, IntPtr.Zero); - } - - if (r == 0) - { - errorCode = GetLastWin32ErrorAndDisposeHandleIfInvalid(); - return -1; - } - else - { - errorCode = 0; - return numBytesWritten; - } - } - - private int GetLastWin32ErrorAndDisposeHandleIfInvalid() - { - int errorCode = Marshal.GetLastWin32Error(); - - // If ERROR_INVALID_HANDLE is returned, it doesn't suffice to set - // the handle as invalid; the handle must also be closed. - // - // Marking the handle as invalid but not closing the handle - // resulted in exceptions during finalization and locked column - // values (due to invalid but unclosed handle) in SQL Win32FileStream - // scenarios. - // - // A more mainstream scenario involves accessing a file on a - // network share. ERROR_INVALID_HANDLE may occur because the network - // connection was dropped and the server closed the handle. However, - // the client side handle is still open and even valid for certain - // operations. - // - // Note that _parent.Dispose doesn't throw so we don't need to special case. - // SetHandleAsInvalid only sets _closed field to true (without - // actually closing handle) so we don't need to call that as well. - if (errorCode == Interop.Errors.ERROR_INVALID_HANDLE) - { - _fileHandle.Dispose(); - } - - return errorCode; + return FileStreamHelpers.WriteFileNative(handle, buffer, overlapped, out errorCode); } public override Task CopyToAsync(Stream destination, int bufferSize, CancellationToken cancellationToken) @@ -1529,30 +1395,8 @@ public void UnsafeOnCompleted(Action continuation) } } - internal override void Lock(long position, long length) - { - int positionLow = unchecked((int)(position)); - int positionHigh = unchecked((int)(position >> 32)); - int lengthLow = unchecked((int)(length)); - int lengthHigh = unchecked((int)(length >> 32)); - - if (!Interop.Kernel32.LockFile(_fileHandle, positionLow, positionHigh, lengthLow, lengthHigh)) - { - throw Win32Marshal.GetExceptionForLastWin32Error(_path); - } - } - - internal override void Unlock(long position, long length) - { - int positionLow = unchecked((int)(position)); - int positionHigh = unchecked((int)(position >> 32)); - int lengthLow = unchecked((int)(length)); - int lengthHigh = unchecked((int)(length >> 32)); + internal override void Lock(long position, long length) => FileStreamHelpers.Lock(_fileHandle, _path, position, length); - if (!Interop.Kernel32.UnlockFile(_fileHandle, positionLow, positionHigh, lengthLow, lengthHigh)) - { - throw Win32Marshal.GetExceptionForLastWin32Error(_path); - } - } + internal override void Unlock(long position, long length) => FileStreamHelpers.Unlock(_fileHandle, _path, position, length); } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/WindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/WindowsFileStreamStrategy.cs new file mode 100644 index 00000000000000..a5655cc3ac1e2c --- /dev/null +++ b/src/libraries/System.Private.CoreLib/src/System/IO/WindowsFileStreamStrategy.cs @@ -0,0 +1,298 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Diagnostics; +using System.Threading.Tasks; +using Microsoft.Win32.SafeHandles; +using System.Runtime.CompilerServices; + +namespace System.IO +{ + // this type serves some basic functionality that is common for Async and Sync Windows File Stream Strategies + internal abstract class WindowsFileStreamStrategy : FileStreamStrategy + { + // Error codes (not HRESULTS), from winerror.h + internal const int ERROR_BROKEN_PIPE = 109; + internal const int ERROR_NO_DATA = 232; + protected const int ERROR_HANDLE_EOF = 38; + protected const int ERROR_INVALID_PARAMETER = 87; + 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 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; + + private long _appendStart; // When appending, prevent overwriting file. + + internal WindowsFileStreamStrategy(SafeFileHandle handle, FileAccess access) + { + _exposedHandle = true; + + InitFromHandle(handle, access, out _canSeek, out _isPipe); + + // Note: Cleaner to set the following fields in ValidateAndInitFromHandle, + // but we can't as they're readonly. + _access = access; + + // As the handle was passed in, we must set the handle field at the very end to + // avoid the finalizer closing the handle when we throw errors. + _fileHandle = handle; + } + + internal WindowsFileStreamStrategy(string path, FileMode mode, FileAccess access, FileShare share, FileOptions options) + { + string fullPath = Path.GetFullPath(path); + + _path = fullPath; + _access = access; + + _fileHandle = FileStreamHelpers.OpenHandle(fullPath, mode, access, share, options); + + try + { + _canSeek = true; + + Init(mode, path); + } + catch + { + // If anything goes wrong while setting up the stream, make sure we deterministically dispose + // of the opened handle. + _fileHandle.Dispose(); + _fileHandle = null!; + throw; + } + } + + public sealed override bool CanSeek => _canSeek; + + public sealed override bool CanRead => !_fileHandle.IsClosed && (_access & FileAccess.Read) != 0; + + public sealed override bool CanWrite => !_fileHandle.IsClosed && (_access & FileAccess.Write) != 0; + + public unsafe sealed override long Length => FileStreamHelpers.GetFileLength(_fileHandle, _path); + + /// Gets or sets the position within the current stream + public override long Position + { + get + { + VerifyOSHandlePosition(); + + return _filePosition; + } + set + { + Seek(value, SeekOrigin.Begin); + } + } + + internal sealed override string Name => _path ?? SR.IO_UnknownFileName; + + internal sealed override bool IsClosed => _fileHandle.IsClosed; + + internal sealed override SafeFileHandle SafeFileHandle + { + get + { + // Flushing is the responsibility of BufferedFileStreamStrategy + _exposedHandle = true; + return _fileHandle; + } + } + + // this method just disposes everything as there is no buffer here + // and we don't really need to Flush anything here + public override ValueTask DisposeAsync() + { + if (_fileHandle != null && !_fileHandle.IsClosed) + { + _fileHandle.ThreadPoolBinding?.Dispose(); + _fileHandle.Dispose(); + } + + GC.SuppressFinalize(this); // the handle is closed; nothing further for the finalizer to do + + return ValueTask.CompletedTask; + } + + // this method in the future will be called in no-buffering scenarios + internal sealed override void DisposeInternal(bool disposing) => Dispose(disposing); + + // this method is called from BufferedStream.Dispose so the content is already flushed + protected override void Dispose(bool disposing) + { + if (_fileHandle != null && !_fileHandle.IsClosed) + { + _fileHandle.ThreadPoolBinding?.Dispose(); + _fileHandle.Dispose(); + } + + // Don't set the buffer to null, to avoid a NullReferenceException + // when users have a race condition in their code (i.e. they call + // Close when calling another method on Stream like Read). + } + + public sealed override void Flush() => Flush(flushToDisk: false); // we have nothing to flush as there is no buffer here + + internal sealed override void Flush(bool flushToDisk) + { + if (flushToDisk && CanWrite) + { + FileStreamHelpers.FlushToDisk(_fileHandle, _path); + } + } + + public sealed override long Seek(long offset, SeekOrigin origin) + { + if (origin < SeekOrigin.Begin || origin > SeekOrigin.End) + throw new ArgumentException(SR.Argument_InvalidSeekOrigin, nameof(origin)); + if (_fileHandle.IsClosed) throw Error.GetFileNotOpen(); + if (!CanSeek) throw Error.GetSeekNotSupported(); + + // Verify that internal position is in sync with the handle + VerifyOSHandlePosition(); + + long oldPos = _filePosition; + long pos = SeekCore(_fileHandle, offset, origin); + + // Prevent users from overwriting data in a file that was opened in + // append mode. + if (_appendStart != -1 && pos < _appendStart) + { + SeekCore(_fileHandle, oldPos, SeekOrigin.Begin); + throw new IOException(SR.IO_SeekAppendOverwrite); + } + + return pos; + } + + // This doesn't do argument checking. Necessary for SetLength, which must + // set the file pointer beyond the end of the file. This will update the + // internal position + protected long SeekCore(SafeFileHandle fileHandle, long offset, SeekOrigin origin, bool closeInvalidHandle = false) + { + Debug.Assert(!fileHandle.IsClosed && _canSeek, "!fileHandle.IsClosed && _canSeek"); + + return _filePosition = FileStreamHelpers.Seek(fileHandle, _path, offset, origin, closeInvalidHandle); + } + + internal sealed override void Lock(long position, long length) => FileStreamHelpers.Lock(_fileHandle, _path, position, length); + + internal sealed override void Unlock(long position, long length) => FileStreamHelpers.Unlock(_fileHandle, _path, position, length); + + protected abstract void OnInitFromHandle(SafeFileHandle handle); + + protected virtual void OnInit() { } + + private void Init(FileMode mode, string originalPath) + { + FileStreamHelpers.ValidateFileTypeForNonExtendedPaths(_fileHandle, originalPath); + + OnInit(); + + // For Append mode... + if (mode == FileMode.Append) + { + _appendStart = SeekCore(_fileHandle, 0, SeekOrigin.End); + } + else + { + _appendStart = -1; + } + } + + private void InitFromHandle(SafeFileHandle handle, FileAccess access, out bool canSeek, out bool isPipe) + { +#if DEBUG + bool hadBinding = handle.ThreadPoolBinding != null; + + try + { +#endif + InitFromHandleImpl(handle, out canSeek, out isPipe); +#if DEBUG + } + catch + { + Debug.Assert(hadBinding || handle.ThreadPoolBinding == null, "We should never error out with a ThreadPoolBinding we've added"); + throw; + } +#endif + } + + private void InitFromHandleImpl(SafeFileHandle handle, out bool canSeek, out bool isPipe) + { + FileStreamHelpers.GetFileTypeSpecificInformation(handle, out canSeek, out isPipe); + + OnInitFromHandle(handle); + + if (_canSeek) + SeekCore(handle, 0, SeekOrigin.Current); + else + _filePosition = 0; + } + + public sealed override void SetLength(long value) + { + if (_appendStart != -1 && value < _appendStart) + throw new IOException(SR.IO_SetLengthAppendTruncate); + + SetLengthCore(value); + } + + // We absolutely need this method broken out so that WriteInternalCoreAsync can call + // a method without having to go through buffering code that might call FlushWrite. + protected unsafe void SetLengthCore(long value) + { + Debug.Assert(value >= 0, "value >= 0"); + VerifyOSHandlePosition(); + + FileStreamHelpers.SetLength(_fileHandle, _path, value); + + if (_filePosition > value) + { + SeekCore(_fileHandle, 0, SeekOrigin.End); + } + } + + /// + /// Verify that the actual position of the OS's handle equals what we expect it to. + /// This will fail if someone else moved the UnixFileStream's handle or if + /// our position updating code is incorrect. + /// + [MethodImpl(MethodImplOptions.AggressiveInlining)] + protected void VerifyOSHandlePosition() + { + bool verifyPosition = _exposedHandle; // in release, only verify if we've given out the handle such that someone else could be manipulating it +#if DEBUG + verifyPosition = true; // in debug, always make sure our position matches what the OS says it should be +#endif + if (verifyPosition && CanSeek) + { + long oldPos = _filePosition; // SeekCore will override the current _position, so save it now + long curPos = SeekCore(_fileHandle, 0, SeekOrigin.Current); + if (oldPos != curPos) + { + // For reads, this is non-fatal but we still could have returned corrupted + // data in some cases, so discard the internal buffer. For writes, + // this is a problem; discard the buffer and error out. + + throw new IOException(SR.IO_FileStreamHandlePosition); + } + } + } + } +} From 5c8018d4e207cacbe08f4a6fcc3546773dfe5ac8 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 25 Feb 2021 16:55:39 +0100 Subject: [PATCH 02/20] introduce SyncWindowsFileStreamStrategy --- .../System.Private.CoreLib.Shared.projitems | 1 + .../System/IO/FileStreamHelpers.Windows.cs | 2 +- .../IO/SyncWindowsFileStreamStrategy.cs | 159 ++++++++++++++++++ 3 files changed, 161 insertions(+), 1 deletion(-) create mode 100644 src/libraries/System.Private.CoreLib/src/System/IO/SyncWindowsFileStreamStrategy.cs diff --git a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems index 8e99e53d99b09b..0cd764a72b495a 100644 --- a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems +++ b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems @@ -1640,6 +1640,7 @@ + diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs index 02e8aa29326836..cb1380f69bd77b 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs @@ -59,7 +59,7 @@ internal static bool GetDefaultIsAsync(SafeFileHandle handle, bool defaultIsAsyn return handle.IsAsync ?? !IsHandleSynchronous(handle, ignoreInvalid: true) ?? defaultIsAsync; } - private static unsafe bool? IsHandleSynchronous(SafeFileHandle fileHandle, bool ignoreInvalid) + internal static unsafe bool? IsHandleSynchronous(SafeFileHandle fileHandle, bool ignoreInvalid) { if (fileHandle.IsInvalid) return null; diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/SyncWindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/SyncWindowsFileStreamStrategy.cs new file mode 100644 index 00000000000000..99a64e961cc123 --- /dev/null +++ b/src/libraries/System.Private.CoreLib/src/System/IO/SyncWindowsFileStreamStrategy.cs @@ -0,0 +1,159 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Diagnostics; +using System.Runtime.InteropServices; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Win32.SafeHandles; + +namespace System.IO +{ + internal sealed class SyncWindowsFileStreamStrategy : WindowsFileStreamStrategy + { + internal SyncWindowsFileStreamStrategy(SafeFileHandle handle, FileAccess access) : base(handle, access) + { + } + + internal SyncWindowsFileStreamStrategy(string path, FileMode mode, FileAccess access, FileShare share, FileOptions options) + : base(path, mode, access, share, options) + { + } + + internal override bool IsAsync => false; + + protected override void OnInitFromHandle(SafeFileHandle handle) + { + // As we can accurately check the handle type when we have access to NtQueryInformationFile we don't need to skip for + // any particular file handle type. + + // If the handle was passed in without an explicit async setting, we already looked it up in GetDefaultIsAsync + if (!handle.IsAsync.HasValue) + return; + + // If we can't check the handle, just assume it is ok. + if (!(FileStreamHelpers.IsHandleSynchronous(handle, ignoreInvalid: false) ?? true)) + throw new ArgumentException(SR.Arg_HandleNotSync, nameof(handle)); + } + + public override int Read(byte[] buffer, int offset, int count) => ReadSpan(new Span(buffer, offset, count)); + + public override int Read(Span buffer) => ReadSpan(buffer); + + public override Task ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) + { + // If we weren't opened for asynchronous I/O, we still call to the base implementation so that + // Read is invoked asynchronously. But we can do so using the base Stream's internal helper + // that bypasses delegating to BeginRead, since we already know this is FileStream rather + // than something derived from it and what our BeginRead implementation is going to do. + return (Task)BeginReadInternal(buffer, offset, count, null, null, serializeAsynchronously: true, apm: false); + } + + public override ValueTask ReadAsync(Memory buffer, CancellationToken cancellationToken = default) + { + // If we weren't opened for asynchronous I/O, we still call to the base implementation so that + // Read is invoked asynchronously. But if we have a byte[], we can do so using the base Stream's + // internal helper that bypasses delegating to BeginRead, since we already know this is FileStream + // rather than something derived from it and what our BeginRead implementation is going to do. + return MemoryMarshal.TryGetArray(buffer, out ArraySegment segment) ? + new ValueTask((Task)BeginReadInternal(segment.Array!, segment.Offset, segment.Count, null, null, serializeAsynchronously: true, apm: false)) : + base.ReadAsync(buffer, cancellationToken); + } + + public override void Write(byte[] buffer, int offset, int count) + => WriteSpan(new ReadOnlySpan(buffer, offset, count)); + + public override void Write(ReadOnlySpan buffer) + { + if (_fileHandle.IsClosed) + { + throw Error.GetFileNotOpen(); + } + + WriteSpan(buffer); + } + + public override Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) + { + // If we weren't opened for asynchronous I/O, we still call to the base implementation so that + // Write is invoked asynchronously. But we can do so using the base Stream's internal helper + // that bypasses delegating to BeginWrite, since we already know this is FileStream rather + // than something derived from it and what our BeginWrite implementation is going to do. + return (Task)BeginWriteInternal(buffer, offset, count, null, null, serializeAsynchronously: true, apm: false); + } + + public override ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationToken cancellationToken = default) + { + // If we weren't opened for asynchronous I/O, we still call to the base implementation so that + // Write is invoked asynchronously. But if we have a byte[], we can do so using the base Stream's + // internal helper that bypasses delegating to BeginWrite, since we already know this is FileStream + // rather than something derived from it and what our BeginWrite implementation is going to do. + return MemoryMarshal.TryGetArray(buffer, out ArraySegment segment) ? + new ValueTask((Task)BeginWriteInternal(segment.Array!, segment.Offset, segment.Count, null, null, serializeAsynchronously: true, apm: false)) : + base.WriteAsync(buffer, cancellationToken); + } + + private unsafe int ReadSpan(Span destination) + { + Debug.Assert(CanRead, "BufferedStream has already verified that"); + Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed"); + + // Make sure we are reading from the right spot + VerifyOSHandlePosition(); + + int r = FileStreamHelpers.ReadFileNative(_fileHandle, destination, null, out int errorCode); + + if (r == -1) + { + // For pipes, ERROR_BROKEN_PIPE is the normal end of the pipe. + if (errorCode == ERROR_BROKEN_PIPE) + { + r = 0; + } + else + { + if (errorCode == ERROR_INVALID_PARAMETER) + throw new ArgumentException(SR.Arg_HandleNotSync, "_fileHandle"); + + throw Win32Marshal.GetExceptionForWin32Error(errorCode, _path); + } + } + Debug.Assert(r >= 0, "FileStream's ReadNative is likely broken."); + _filePosition += r; + + return r; + } + + private unsafe void WriteSpan(ReadOnlySpan source) + { + Debug.Assert(CanWrite, "BufferedStream has already verified that"); + Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed"); + + // Make sure we are writing to the position that we think we are + VerifyOSHandlePosition(); + + int r = FileStreamHelpers.WriteFileNative(_fileHandle, source, null, out int errorCode); + + if (r == -1) + { + // For pipes, ERROR_NO_DATA is not an error, but the pipe is closing. + if (errorCode == ERROR_NO_DATA) + { + r = 0; + } + else + { + // ERROR_INVALID_PARAMETER may be returned for writes + // where the position is too large or for synchronous writes + // to a handle opened asynchronously. + if (errorCode == ERROR_INVALID_PARAMETER) + throw new IOException(SR.IO_FileTooLongOrHandleNotSync); + throw Win32Marshal.GetExceptionForWin32Error(errorCode, _path); + } + } + Debug.Assert(r >= 0, "FileStream's WriteCore is likely broken."); + _filePosition += r; + return; + } + } +} From 2e159c4536e4e2e0556777eaef7013722d61e665 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 25 Feb 2021 17:00:23 +0100 Subject: [PATCH 03/20] introduce AsyncWindowsFileStreamStrategy --- .../src/System/Threading/Overlapped.cs | 4 + .../System.Private.CoreLib.Shared.projitems | 3 +- .../IO/AsyncWindowsFileStreamStrategy.cs | 396 ++++++++++++++++++ .../IO/FileStreamCompletionSource.Win32.cs | 257 ------------ .../IO/FileStreamCompletionSource.Windows.cs | 267 ++++++++++++ .../System/IO/FileStreamHelpers.Windows.cs | 246 +++++++++++ .../IO/LegacyFileStreamStrategy.Windows.cs | 252 +---------- 7 files changed, 928 insertions(+), 497 deletions(-) create mode 100644 src/libraries/System.Private.CoreLib/src/System/IO/AsyncWindowsFileStreamStrategy.cs delete mode 100644 src/libraries/System.Private.CoreLib/src/System/IO/FileStreamCompletionSource.Win32.cs create mode 100644 src/libraries/System.Private.CoreLib/src/System/IO/FileStreamCompletionSource.Windows.cs diff --git a/src/coreclr/System.Private.CoreLib/src/System/Threading/Overlapped.cs b/src/coreclr/System.Private.CoreLib/src/System/Threading/Overlapped.cs index 728954377dff84..53abea5f0d16d3 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Threading/Overlapped.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Threading/Overlapped.cs @@ -132,6 +132,8 @@ internal sealed unsafe class OverlappedData return AllocateNativeOverlapped(); } + internal bool IsUserObject(byte[]? buffer) => ReferenceEquals(_userObject, buffer); + [MethodImpl(MethodImplOptions.InternalCall)] private extern NativeOverlapped* AllocateNativeOverlapped(); @@ -258,6 +260,8 @@ public static unsafe void Free(NativeOverlapped* nativeOverlappedPtr) OverlappedData.GetOverlappedFromNative(nativeOverlappedPtr)._overlapped._overlappedData = null; OverlappedData.FreeNativeOverlapped(nativeOverlappedPtr); } + + internal bool IsUserObject(byte[]? buffer) => _overlappedData!.IsUserObject(buffer); } #endregion class Overlapped diff --git a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems index 0cd764a72b495a..2b7a12a56e52d2 100644 --- a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems +++ b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems @@ -1632,10 +1632,11 @@ + - + diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/AsyncWindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/AsyncWindowsFileStreamStrategy.cs new file mode 100644 index 00000000000000..9d21309fd21075 --- /dev/null +++ b/src/libraries/System.Private.CoreLib/src/System/IO/AsyncWindowsFileStreamStrategy.cs @@ -0,0 +1,396 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Diagnostics; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Win32.SafeHandles; + +namespace System.IO +{ + internal sealed partial class AsyncWindowsFileStreamStrategy : WindowsFileStreamStrategy, IFileStreamCompletionSourceStrategy + { + private PreAllocatedOverlapped? _preallocatedOverlapped; // optimization for async ops to avoid per-op allocations + private FileStreamCompletionSource? _currentOverlappedOwner; // async op currently using the preallocated overlapped + + internal AsyncWindowsFileStreamStrategy(SafeFileHandle handle, FileAccess access) + : base(handle, access) + { + } + + internal AsyncWindowsFileStreamStrategy(string path, FileMode mode, FileAccess access, FileShare share, FileOptions options) + : base(path, mode, access, share, options) + { + } + + internal override bool IsAsync => true; + + public override ValueTask DisposeAsync() + { + // the order matters, let the base class Dispose handle first + ValueTask result = base.DisposeAsync(); + Debug.Assert(result.IsCompleted, "the method must be sync, as it performs no flushing"); + + _preallocatedOverlapped?.Dispose(); + + return result; + } + + protected override void Dispose(bool disposing) + { + // the order matters, let the base class Dispose handle first + base.Dispose(disposing); + + _preallocatedOverlapped?.Dispose(); + } + + protected override void OnInitFromHandle(SafeFileHandle handle) + { + // This is necessary for async IO using IO Completion ports via our + // managed Threadpool API's. This calls the OS's + // BindIoCompletionCallback method, and passes in a stub for the + // LPOVERLAPPED_COMPLETION_ROUTINE. This stub looks at the Overlapped + // struct for this request and gets a delegate to a managed callback + // from there, which it then calls on a threadpool thread. (We allocate + // our native OVERLAPPED structs 2 pointers too large and store EE + // state & a handle to a delegate there.) + // + // If, however, we've already bound this file handle to our completion port, + // don't try to bind it again because it will fail. A handle can only be + // bound to a single completion port at a time. + if (!(handle.IsAsync ?? false)) + { + try + { + handle.ThreadPoolBinding = ThreadPoolBoundHandle.BindHandle(handle); + } + catch (Exception ex) + { + // If you passed in a synchronous handle and told us to use + // it asynchronously, throw here. + throw new ArgumentException(SR.Arg_HandleNotAsync, nameof(handle), ex); + } + } + } + + protected override void OnInit() + { + // This is necessary for async IO using IO Completion ports via our + // managed Threadpool API's. This (theoretically) calls the OS's + // BindIoCompletionCallback method, and passes in a stub for the + // LPOVERLAPPED_COMPLETION_ROUTINE. This stub looks at the Overlapped + // struct for this request and gets a delegate to a managed callback + // from there, which it then calls on a threadpool thread. (We allocate + // our native OVERLAPPED structs 2 pointers too large and store EE state + // & GC handles there, one to an IAsyncResult, the other to a delegate.) + try + { + _fileHandle.ThreadPoolBinding = ThreadPoolBoundHandle.BindHandle(_fileHandle); + } + catch (ArgumentException ex) + { + throw new IOException(SR.IO_BindHandleFailed, ex); + } + finally + { + if (_fileHandle.ThreadPoolBinding == null) + { + // We should close the handle so that the handle is not open until SafeFileHandle GC + Debug.Assert(!_exposedHandle, "Are we closing handle that we exposed/not own, how?"); + _fileHandle.Dispose(); + } + } + } + + // called by BufferedStream. TODO: find a cleaner solution + internal void OnBufferAllocated(byte[] buffer) + { + Debug.Assert(buffer != null); + Debug.Assert(_preallocatedOverlapped == null); + + _preallocatedOverlapped = new PreAllocatedOverlapped(FileStreamCompletionSource.s_ioCallback, this, buffer); + } + + SafeFileHandle IFileStreamCompletionSourceStrategy.FileHandle => _fileHandle; + + FileStreamCompletionSource? IFileStreamCompletionSourceStrategy.CurrentOverlappedOwner => _currentOverlappedOwner; + + FileStreamCompletionSource? IFileStreamCompletionSourceStrategy.CompareExchangeCurrentOverlappedOwner(FileStreamCompletionSource? newSource, FileStreamCompletionSource? existingSource) + => Interlocked.CompareExchange(ref _currentOverlappedOwner, newSource, existingSource); + + public override int Read(byte[] buffer, int offset, int count) + => ReadAsyncInternal(new Memory(buffer, offset, count)).GetAwaiter().GetResult(); + + public override Task ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) + => ReadAsyncInternal(new Memory(buffer, offset, count), cancellationToken); + + public override ValueTask ReadAsync(Memory destination, CancellationToken cancellationToken = default) + => new ValueTask(ReadAsyncInternal(destination, cancellationToken)); + + private unsafe Task ReadAsyncInternal(Memory destination, CancellationToken cancellationToken = default) + { + Debug.Assert(CanRead, "BufferedStream has already verified that"); + Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed"); + + // Create and store async stream class library specific data in the async result + FileStreamCompletionSource completionSource = FileStreamCompletionSource.Create(this, _preallocatedOverlapped, 0, destination); + NativeOverlapped* intOverlapped = completionSource.Overlapped; + + // Calculate position in the file we should be at after the read is done + if (CanSeek) + { + long len = Length; + + // Make sure we are reading from the position that we think we are + VerifyOSHandlePosition(); + + if (_filePosition + destination.Length > len) + { + if (_filePosition <= len) + { + destination = destination.Slice(0, (int)(len - _filePosition)); + } + else + { + destination = default; + } + } + + // Now set the position to read from in the NativeOverlapped struct + // For pipes, we should leave the offset fields set to 0. + intOverlapped->OffsetLow = unchecked((int)_filePosition); + intOverlapped->OffsetHigh = (int)(_filePosition >> 32); + + // When using overlapped IO, the OS is not supposed to + // touch the file pointer location at all. We will adjust it + // ourselves. This isn't threadsafe. + + // WriteFile should not update the file pointer when writing + // in overlapped mode, according to MSDN. But it does update + // the file pointer when writing to a UNC path! + // So changed the code below to seek to an absolute + // location, not a relative one. ReadFile seems consistent though. + SeekCore(_fileHandle, destination.Length, SeekOrigin.Current); + } + + // queue an async ReadFile operation and pass in a packed overlapped + int r = FileStreamHelpers.ReadFileNative(_fileHandle, destination.Span, intOverlapped, out int errorCode); + + // ReadFile, the OS version, will return 0 on failure. But + // my ReadFileNative wrapper returns -1. My wrapper will return + // the following: + // On error, r==-1. + // On async requests that are still pending, r==-1 w/ errorCode==ERROR_IO_PENDING + // on async requests that completed sequentially, r==0 + // You will NEVER RELIABLY be able to get the number of bytes + // read back from this call when using overlapped structures! You must + // not pass in a non-null lpNumBytesRead to ReadFile when using + // overlapped structures! This is by design NT behavior. + if (r == -1) + { + // For pipes, when they hit EOF, they will come here. + if (errorCode == ERROR_BROKEN_PIPE) + { + // Not an error, but EOF. AsyncFSCallback will NOT be + // called. Call the user callback here. + + // We clear the overlapped status bit for this special case. + // Failure to do so looks like we are freeing a pending overlapped later. + intOverlapped->InternalLow = IntPtr.Zero; + completionSource.SetCompletedSynchronously(0); + } + else if (errorCode != ERROR_IO_PENDING) + { + if (!_fileHandle.IsClosed && CanSeek) // Update Position - It could be anywhere. + { + SeekCore(_fileHandle, 0, SeekOrigin.Current); + } + + completionSource.ReleaseNativeResource(); + + if (errorCode == ERROR_HANDLE_EOF) + { + throw Error.GetEndOfFile(); + } + else + { + throw Win32Marshal.GetExceptionForWin32Error(errorCode, _path); + } + } + else if (cancellationToken.CanBeCanceled) // ERROR_IO_PENDING + { + // Only once the IO is pending do we register for cancellation + completionSource.RegisterForCancellation(cancellationToken); + } + } + else + { + // Due to a workaround for a race condition in NT's ReadFile & + // WriteFile routines, we will always be returning 0 from ReadFileNative + // when we do async IO instead of the number of bytes read, + // irregardless of whether the operation completed + // synchronously or asynchronously. We absolutely must not + // set asyncResult._numBytes here, since will never have correct + // results. + } + + return completionSource.Task; + } + + public override void Write(byte[] buffer, int offset, int count) + => WriteAsyncInternal(new ReadOnlyMemory(buffer, offset, count), CancellationToken.None).AsTask().GetAwaiter().GetResult(); + + public override Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) + => WriteAsyncInternal(new ReadOnlyMemory(buffer, offset, count), cancellationToken).AsTask(); + + public override ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationToken cancellationToken = default) + => WriteAsyncInternal(buffer, cancellationToken); + + private ValueTask WriteAsyncInternal(ReadOnlyMemory source, CancellationToken cancellationToken) + => new ValueTask(WriteAsyncInternalCore(source, cancellationToken)); + + private unsafe Task WriteAsyncInternalCore(ReadOnlyMemory source, CancellationToken cancellationToken) + { + Debug.Assert(CanWrite, "BufferedStream has already verified that"); + Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed"); + + // Create and store async stream class library specific data in the async result + FileStreamCompletionSource completionSource = FileStreamCompletionSource.Create(this, _preallocatedOverlapped, 0, source); + NativeOverlapped* intOverlapped = completionSource.Overlapped; + + if (CanSeek) + { + // Make sure we set the length of the file appropriately. + long len = Length; + + // Make sure we are writing to the position that we think we are + VerifyOSHandlePosition(); + + if (_filePosition + source.Length > len) + { + SetLengthCore(_filePosition + source.Length); + } + + // Now set the position to read from in the NativeOverlapped struct + // For pipes, we should leave the offset fields set to 0. + intOverlapped->OffsetLow = (int)_filePosition; + intOverlapped->OffsetHigh = (int)(_filePosition >> 32); + + // When using overlapped IO, the OS is not supposed to + // touch the file pointer location at all. We will adjust it + // ourselves. This isn't threadsafe. + SeekCore(_fileHandle, source.Length, SeekOrigin.Current); + } + + // queue an async WriteFile operation and pass in a packed overlapped + int r = FileStreamHelpers.WriteFileNative(_fileHandle, source.Span, intOverlapped, out int errorCode); + + // WriteFile, the OS version, will return 0 on failure. But + // my WriteFileNative wrapper returns -1. My wrapper will return + // the following: + // On error, r==-1. + // On async requests that are still pending, r==-1 w/ errorCode==ERROR_IO_PENDING + // On async requests that completed sequentially, r==0 + // You will NEVER RELIABLY be able to get the number of bytes + // written back from this call when using overlapped IO! You must + // not pass in a non-null lpNumBytesWritten to WriteFile when using + // overlapped structures! This is ByDesign NT behavior. + if (r == -1) + { + // For pipes, when they are closed on the other side, they will come here. + if (errorCode == ERROR_NO_DATA) + { + // Not an error, but EOF. AsyncFSCallback will NOT be called. + // Completing TCS and return cached task allowing the GC to collect TCS. + completionSource.SetCompletedSynchronously(0); + return Task.CompletedTask; + } + else if (errorCode != ERROR_IO_PENDING) + { + if (!_fileHandle.IsClosed && CanSeek) // Update Position - It could be anywhere. + { + SeekCore(_fileHandle, 0, SeekOrigin.Current); + } + + completionSource.ReleaseNativeResource(); + + if (errorCode == ERROR_HANDLE_EOF) + { + throw Error.GetEndOfFile(); + } + else + { + throw Win32Marshal.GetExceptionForWin32Error(errorCode, _path); + } + } + else if (cancellationToken.CanBeCanceled) // ERROR_IO_PENDING + { + // Only once the IO is pending do we register for cancellation + completionSource.RegisterForCancellation(cancellationToken); + } + } + else + { + // Due to a workaround for a race condition in NT's ReadFile & + // WriteFile routines, we will always be returning 0 from WriteFileNative + // when we do async IO instead of the number of bytes written, + // irregardless of whether the operation completed + // synchronously or asynchronously. We absolutely must not + // set asyncResult._numBytes here, since will never have correct + // results. + } + + return completionSource.Task; + } + + public override Task CopyToAsync(Stream destination, int bufferSize, CancellationToken cancellationToken) + { + ValidateCopyToArguments(destination, bufferSize); + + // Fail if the file was closed + if (_fileHandle.IsClosed) + { + throw Error.GetFileNotOpen(); + } + if (!CanRead) + { + throw Error.GetReadNotSupported(); + } + + // Bail early for cancellation if cancellation has been requested + if (cancellationToken.IsCancellationRequested) + { + return Task.FromCanceled(cancellationToken); + } + + return AsyncModeCopyToAsync(destination, bufferSize, cancellationToken); + } + + private async Task AsyncModeCopyToAsync(Stream destination, int bufferSize, CancellationToken cancellationToken) + { + Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed"); + Debug.Assert(CanRead, "_parent.CanRead"); + + bool canSeek = CanSeek; + if (canSeek) + { + VerifyOSHandlePosition(); + } + + 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 + } + finally + { + // Make sure the stream's current position reflects where we ended up + if (!_fileHandle.IsClosed && CanSeek) + { + SeekCore(_fileHandle, 0, SeekOrigin.End); + } + } + } + } +} diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamCompletionSource.Win32.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamCompletionSource.Win32.cs deleted file mode 100644 index c7b56290ca7eee..00000000000000 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamCompletionSource.Win32.cs +++ /dev/null @@ -1,257 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using System.Buffers; -using System.Diagnostics; -using System.Runtime.InteropServices; -using System.Threading; -using System.Threading.Tasks; - -namespace System.IO -{ - internal sealed partial class LegacyFileStreamStrategy : FileStreamStrategy - { - // This is an internal object extending TaskCompletionSource with fields - // for all of the relevant data necessary to complete the IO operation. - // This is used by IOCallback and all of the async methods. - private unsafe class FileStreamCompletionSource : TaskCompletionSource - { - private const long NoResult = 0; - private const long ResultSuccess = (long)1 << 32; - private const long ResultError = (long)2 << 32; - private const long RegisteringCancellation = (long)4 << 32; - private const long CompletedCallback = (long)8 << 32; - private const ulong ResultMask = ((ulong)uint.MaxValue) << 32; - - private static Action? s_cancelCallback; - - private readonly LegacyFileStreamStrategy _stream; - private readonly int _numBufferedBytes; - private CancellationTokenRegistration _cancellationRegistration; -#if DEBUG - private bool _cancellationHasBeenRegistered; -#endif - private NativeOverlapped* _overlapped; // Overlapped class responsible for operations in progress when an appdomain unload occurs - private long _result; // Using long since this needs to be used in Interlocked APIs - - // Using RunContinuationsAsynchronously for compat reasons (old API used Task.Factory.StartNew for continuations) - protected FileStreamCompletionSource(LegacyFileStreamStrategy stream, int numBufferedBytes, byte[]? bytes) - : base(TaskCreationOptions.RunContinuationsAsynchronously) - { - _numBufferedBytes = numBufferedBytes; - _stream = stream; - _result = NoResult; - - // Create the native overlapped. We try to use the preallocated overlapped if possible: it's possible if the byte - // buffer is null (there's nothing to pin) or the same one that's associated with the preallocated overlapped (and - // thus is already pinned) and if no one else is currently using the preallocated overlapped. This is the fast-path - // for cases where the user-provided buffer is smaller than the FileStream's buffer (such that the FileStream's - // buffer is used) and where operations on the FileStream are not being performed concurrently. - Debug.Assert(bytes == null || ReferenceEquals(bytes, _stream._buffer)); - - // The _preallocatedOverlapped is null if the internal buffer was never created, so we check for - // a non-null bytes before using the stream's _preallocatedOverlapped - _overlapped = bytes != null && _stream.CompareExchangeCurrentOverlappedOwner(this, null) == null ? - _stream._fileHandle.ThreadPoolBinding!.AllocateNativeOverlapped(_stream._preallocatedOverlapped!) : // allocated when buffer was created, and buffer is non-null - _stream._fileHandle.ThreadPoolBinding!.AllocateNativeOverlapped(s_ioCallback, this, bytes); - Debug.Assert(_overlapped != null, "AllocateNativeOverlapped returned null"); - } - - internal NativeOverlapped* Overlapped => _overlapped; - - public void SetCompletedSynchronously(int numBytes) - { - ReleaseNativeResource(); - TrySetResult(numBytes + _numBufferedBytes); - } - - public void RegisterForCancellation(CancellationToken cancellationToken) - { -#if DEBUG - Debug.Assert(cancellationToken.CanBeCanceled); - Debug.Assert(!_cancellationHasBeenRegistered, "Cannot register for cancellation twice"); - _cancellationHasBeenRegistered = true; -#endif - - // Quick check to make sure the IO hasn't completed - if (_overlapped != null) - { - Action? cancelCallback = s_cancelCallback ??= Cancel; - - // Register the cancellation only if the IO hasn't completed - long packedResult = Interlocked.CompareExchange(ref _result, RegisteringCancellation, NoResult); - if (packedResult == NoResult) - { - _cancellationRegistration = cancellationToken.UnsafeRegister(cancelCallback, this); - - // Switch the result, just in case IO completed while we were setting the registration - packedResult = Interlocked.Exchange(ref _result, NoResult); - } - else if (packedResult != CompletedCallback) - { - // Failed to set the result, IO is in the process of completing - // Attempt to take the packed result - packedResult = Interlocked.Exchange(ref _result, NoResult); - } - - // If we have a callback that needs to be completed - if ((packedResult != NoResult) && (packedResult != CompletedCallback) && (packedResult != RegisteringCancellation)) - { - CompleteCallback((ulong)packedResult); - } - } - } - - internal virtual void ReleaseNativeResource() - { - // Ensure that cancellation has been completed and cleaned up. - _cancellationRegistration.Dispose(); - - // Free the overlapped. - // NOTE: The cancellation must *NOT* be running at this point, or it may observe freed memory - // (this is why we disposed the registration above). - if (_overlapped != null) - { - _stream._fileHandle.ThreadPoolBinding!.FreeNativeOverlapped(_overlapped); - _overlapped = null; - } - - // Ensure we're no longer set as the current completion source (we may not have been to begin with). - // Only one operation at a time is eligible to use the preallocated overlapped, - _stream.CompareExchangeCurrentOverlappedOwner(null, this); - } - - // When doing IO asynchronously (i.e. _isAsync==true), this callback is - // called by a free thread in the threadpool when the IO operation - // completes. - internal static void IOCallback(uint errorCode, uint numBytes, NativeOverlapped* pOverlapped) - { - // Extract the completion source from the overlapped. The state in the overlapped - // will either be a FileStream (in the case where the preallocated overlapped was used), - // in which case the operation being completed is its _currentOverlappedOwner, or it'll - // be directly the FileStreamCompletionSource that's completing (in the case where the preallocated - // overlapped was already in use by another operation). - object? state = ThreadPoolBoundHandle.GetNativeOverlappedState(pOverlapped); - Debug.Assert(state is LegacyFileStreamStrategy || state is FileStreamCompletionSource); - FileStreamCompletionSource completionSource = state is LegacyFileStreamStrategy fs ? - fs._currentOverlappedOwner! : // must be owned - (FileStreamCompletionSource)state!; - Debug.Assert(completionSource != null); - Debug.Assert(completionSource._overlapped == pOverlapped, "Overlaps don't match"); - - // Handle reading from & writing to closed pipes. While I'm not sure - // this is entirely necessary anymore, maybe it's possible for - // an async read on a pipe to be issued and then the pipe is closed, - // returning this error. This may very well be necessary. - ulong packedResult; - if (errorCode != 0 && errorCode != ERROR_BROKEN_PIPE && errorCode != ERROR_NO_DATA) - { - packedResult = ((ulong)ResultError | errorCode); - } - else - { - packedResult = ((ulong)ResultSuccess | numBytes); - } - - // Stow the result so that other threads can observe it - // And, if no other thread is registering cancellation, continue - if (NoResult == Interlocked.Exchange(ref completionSource._result, (long)packedResult)) - { - // Successfully set the state, attempt to take back the callback - if (Interlocked.Exchange(ref completionSource._result, CompletedCallback) != NoResult) - { - // Successfully got the callback, finish the callback - completionSource.CompleteCallback(packedResult); - } - // else: Some other thread stole the result, so now it is responsible to finish the callback - } - // else: Some other thread is registering a cancellation, so it *must* finish the callback - } - - private void CompleteCallback(ulong packedResult) - { - // Free up the native resource and cancellation registration - CancellationToken cancellationToken = _cancellationRegistration.Token; // access before disposing registration - ReleaseNativeResource(); - - // Unpack the result and send it to the user - long result = (long)(packedResult & ResultMask); - if (result == ResultError) - { - int errorCode = unchecked((int)(packedResult & uint.MaxValue)); - if (errorCode == Interop.Errors.ERROR_OPERATION_ABORTED) - { - TrySetCanceled(cancellationToken.IsCancellationRequested ? cancellationToken : new CancellationToken(true)); - } - else - { - Exception e = Win32Marshal.GetExceptionForWin32Error(errorCode); - e.SetCurrentStackTrace(); - TrySetException(e); - } - } - else - { - Debug.Assert(result == ResultSuccess, "Unknown result"); - TrySetResult((int)(packedResult & uint.MaxValue) + _numBufferedBytes); - } - } - - private static void Cancel(object? state) - { - // WARNING: This may potentially be called under a lock (during cancellation registration) - - Debug.Assert(state is FileStreamCompletionSource, "Unknown state passed to cancellation"); - FileStreamCompletionSource completionSource = (FileStreamCompletionSource)state; - Debug.Assert(completionSource._overlapped != null && !completionSource.Task.IsCompleted, "IO should not have completed yet"); - - // If the handle is still valid, attempt to cancel the IO - if (!completionSource._stream._fileHandle.IsInvalid && - !Interop.Kernel32.CancelIoEx(completionSource._stream._fileHandle, completionSource._overlapped)) - { - int errorCode = Marshal.GetLastWin32Error(); - - // ERROR_NOT_FOUND is returned if CancelIoEx cannot find the request to cancel. - // This probably means that the IO operation has completed. - if (errorCode != Interop.Errors.ERROR_NOT_FOUND) - { - throw Win32Marshal.GetExceptionForWin32Error(errorCode); - } - } - } - - public static FileStreamCompletionSource Create(LegacyFileStreamStrategy stream, int numBufferedBytesRead, ReadOnlyMemory memory) - { - // If the memory passed in is the stream's internal buffer, we can use the base FileStreamCompletionSource, - // which has a PreAllocatedOverlapped with the memory already pinned. Otherwise, we use the derived - // MemoryFileStreamCompletionSource, which Retains the memory, which will result in less pinning in the case - // where the underlying memory is backed by pre-pinned buffers. - return MemoryMarshal.TryGetArray(memory, out ArraySegment buffer) && ReferenceEquals(buffer.Array, stream._buffer) ? - new FileStreamCompletionSource(stream, numBufferedBytesRead, buffer.Array) : - new MemoryFileStreamCompletionSource(stream, numBufferedBytesRead, memory); - } - } - - /// - /// Extends with to support disposing of a - /// when the operation has completed. This should only be used - /// when memory doesn't wrap a byte[]. - /// - private sealed class MemoryFileStreamCompletionSource : FileStreamCompletionSource - { - private MemoryHandle _handle; // mutable struct; do not make this readonly - - internal MemoryFileStreamCompletionSource(LegacyFileStreamStrategy stream, int numBufferedBytes, ReadOnlyMemory memory) : - base(stream, numBufferedBytes, bytes: null) // this type handles the pinning, so null is passed for bytes - { - _handle = memory.Pin(); - } - - internal override void ReleaseNativeResource() - { - _handle.Dispose(); - base.ReleaseNativeResource(); - } - } - } -} diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamCompletionSource.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamCompletionSource.Windows.cs new file mode 100644 index 00000000000000..eeea50a56f5a60 --- /dev/null +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamCompletionSource.Windows.cs @@ -0,0 +1,267 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Buffers; +using System.Diagnostics; +using System.Runtime.InteropServices; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Win32.SafeHandles; + +namespace System.IO +{ + // to avoid code duplicaiton of FileStreamCompletionSource for LegacyFileStreamStrategy and AsyncWindowsFileStreamStrategy + // we have created the following interface that is a common contract for both of them + internal interface IFileStreamCompletionSourceStrategy + { + SafeFileHandle FileHandle { get; } + + FileStreamCompletionSource? CurrentOverlappedOwner { get; } + + FileStreamCompletionSource? CompareExchangeCurrentOverlappedOwner(FileStreamCompletionSource? newSource, FileStreamCompletionSource? existingSource); + } + + // This is an internal object extending TaskCompletionSource with fields + // for all of the relevant data necessary to complete the IO operation. + // This is used by IOCallback and all of the async methods. + internal unsafe class FileStreamCompletionSource : TaskCompletionSource + { + private const long NoResult = 0; + private const long ResultSuccess = (long)1 << 32; + private const long ResultError = (long)2 << 32; + private const long RegisteringCancellation = (long)4 << 32; + private const long CompletedCallback = (long)8 << 32; + private const ulong ResultMask = ((ulong)uint.MaxValue) << 32; + private const int ERROR_BROKEN_PIPE = 109; + private const int ERROR_NO_DATA = 232; + + internal static readonly unsafe IOCompletionCallback s_ioCallback = IOCallback; + + private static Action? s_cancelCallback; + + private readonly IFileStreamCompletionSourceStrategy _strategy; + private readonly int _numBufferedBytes; + private CancellationTokenRegistration _cancellationRegistration; +#if DEBUG + private bool _cancellationHasBeenRegistered; +#endif + internal NativeOverlapped* _overlapped; // Overlapped class responsible for operations in progress when an appdomain unload occurs + private long _result; // Using long since this needs to be used in Interlocked APIs + + // Using RunContinuationsAsynchronously for compat reasons (old API used Task.Factory.StartNew for continuations) + internal FileStreamCompletionSource(IFileStreamCompletionSourceStrategy strategy, PreAllocatedOverlapped? preallocatedOverlapped, + int numBufferedBytes, byte[]? bytes) : base(TaskCreationOptions.RunContinuationsAsynchronously) + { + _numBufferedBytes = numBufferedBytes; + _strategy = strategy; + _result = NoResult; + + // The _preallocatedOverlapped is null if the internal buffer was never created, so we check for + // a non-null bytes before using the stream's _preallocatedOverlapped + _overlapped = bytes != null && strategy.CompareExchangeCurrentOverlappedOwner(this, null) == null ? + strategy.FileHandle.ThreadPoolBinding!.AllocateNativeOverlapped(preallocatedOverlapped!) : // allocated when buffer was created, and buffer is non-null + strategy.FileHandle.ThreadPoolBinding!.AllocateNativeOverlapped(s_ioCallback, this, bytes); + Debug.Assert(_overlapped != null, "AllocateNativeOverlapped returned null"); + } + + internal NativeOverlapped* Overlapped => _overlapped; + + public void SetCompletedSynchronously(int numBytes) + { + ReleaseNativeResource(); + TrySetResult(numBytes + _numBufferedBytes); + } + + public void RegisterForCancellation(CancellationToken cancellationToken) + { +#if DEBUG + Debug.Assert(cancellationToken.CanBeCanceled); + Debug.Assert(!_cancellationHasBeenRegistered, "Cannot register for cancellation twice"); + _cancellationHasBeenRegistered = true; +#endif + + // Quick check to make sure the IO hasn't completed + if (_overlapped != null) + { + Action? cancelCallback = s_cancelCallback ??= Cancel; + + // Register the cancellation only if the IO hasn't completed + long packedResult = Interlocked.CompareExchange(ref _result, RegisteringCancellation, NoResult); + if (packedResult == NoResult) + { + _cancellationRegistration = cancellationToken.UnsafeRegister(cancelCallback, this); + + // Switch the result, just in case IO completed while we were setting the registration + packedResult = Interlocked.Exchange(ref _result, NoResult); + } + else if (packedResult != CompletedCallback) + { + // Failed to set the result, IO is in the process of completing + // Attempt to take the packed result + packedResult = Interlocked.Exchange(ref _result, NoResult); + } + + // If we have a callback that needs to be completed + if ((packedResult != NoResult) && (packedResult != CompletedCallback) && (packedResult != RegisteringCancellation)) + { + CompleteCallback((ulong)packedResult); + } + } + } + + internal virtual void ReleaseNativeResource() + { + // Ensure that cancellation has been completed and cleaned up. + _cancellationRegistration.Dispose(); + + // Free the overlapped. + // NOTE: The cancellation must *NOT* be running at this point, or it may observe freed memory + // (this is why we disposed the registration above). + if (_overlapped != null) + { + _strategy.FileHandle.ThreadPoolBinding!.FreeNativeOverlapped(_overlapped); + _overlapped = null; + } + + // Ensure we're no longer set as the current completion source (we may not have been to begin with). + // Only one operation at a time is eligible to use the preallocated overlapped, + _strategy.CompareExchangeCurrentOverlappedOwner(null, this); + } + + // When doing IO asynchronously (i.e. _isAsync==true), this callback is + // called by a free thread in the threadpool when the IO operation + // completes. + internal static void IOCallback(uint errorCode, uint numBytes, NativeOverlapped* pOverlapped) + { + // Extract the completion source from the overlapped. The state in the overlapped + // will either be a FileStreamStrategy (in the case where the preallocated overlapped was used), + // in which case the operation being completed is its _currentOverlappedOwner, or it'll + // be directly the FileStreamCompletionSource that's completing (in the case where the preallocated + // overlapped was already in use by another operation). + object? state = ThreadPoolBoundHandle.GetNativeOverlappedState(pOverlapped); + Debug.Assert(state is IFileStreamCompletionSourceStrategy || state is FileStreamCompletionSource); + FileStreamCompletionSource completionSource = state switch + { + IFileStreamCompletionSourceStrategy strategy => strategy.CurrentOverlappedOwner!, // must be owned + _ => (FileStreamCompletionSource)state + }; + Debug.Assert(completionSource != null); + Debug.Assert(completionSource._overlapped == pOverlapped, "Overlaps don't match"); + + // Handle reading from & writing to closed pipes. While I'm not sure + // this is entirely necessary anymore, maybe it's possible for + // an async read on a pipe to be issued and then the pipe is closed, + // returning this error. This may very well be necessary. + ulong packedResult; + if (errorCode != 0 && errorCode != ERROR_BROKEN_PIPE && errorCode != ERROR_NO_DATA) + { + packedResult = ((ulong)ResultError | errorCode); + } + else + { + packedResult = ((ulong)ResultSuccess | numBytes); + } + + // Stow the result so that other threads can observe it + // And, if no other thread is registering cancellation, continue + if (NoResult == Interlocked.Exchange(ref completionSource._result, (long)packedResult)) + { + // Successfully set the state, attempt to take back the callback + if (Interlocked.Exchange(ref completionSource._result, CompletedCallback) != NoResult) + { + // Successfully got the callback, finish the callback + completionSource.CompleteCallback(packedResult); + } + // else: Some other thread stole the result, so now it is responsible to finish the callback + } + // else: Some other thread is registering a cancellation, so it *must* finish the callback + } + + private void CompleteCallback(ulong packedResult) + { + // Free up the native resource and cancellation registration + CancellationToken cancellationToken = _cancellationRegistration.Token; // access before disposing registration + ReleaseNativeResource(); + + // Unpack the result and send it to the user + long result = (long)(packedResult & ResultMask); + if (result == ResultError) + { + int errorCode = unchecked((int)(packedResult & uint.MaxValue)); + if (errorCode == Interop.Errors.ERROR_OPERATION_ABORTED) + { + TrySetCanceled(cancellationToken.IsCancellationRequested ? cancellationToken : new CancellationToken(true)); + } + else + { + Exception e = Win32Marshal.GetExceptionForWin32Error(errorCode); + e.SetCurrentStackTrace(); + TrySetException(e); + } + } + else + { + Debug.Assert(result == ResultSuccess, "Unknown result"); + TrySetResult((int)(packedResult & uint.MaxValue) + _numBufferedBytes); + } + } + + private static void Cancel(object? state) + { + // WARNING: This may potentially be called under a lock (during cancellation registration) + + Debug.Assert(state is FileStreamCompletionSource, "Unknown state passed to cancellation"); + FileStreamCompletionSource completionSource = (FileStreamCompletionSource)state; + Debug.Assert(completionSource._overlapped != null && !completionSource.Task.IsCompleted, "IO should not have completed yet"); + + // If the handle is still valid, attempt to cancel the IO + if (!completionSource._strategy.FileHandle.IsInvalid && + !Interop.Kernel32.CancelIoEx(completionSource._strategy.FileHandle, completionSource._overlapped)) + { + int errorCode = Marshal.GetLastWin32Error(); + + // ERROR_NOT_FOUND is returned if CancelIoEx cannot find the request to cancel. + // This probably means that the IO operation has completed. + if (errorCode != Interop.Errors.ERROR_NOT_FOUND) + { + throw Win32Marshal.GetExceptionForWin32Error(errorCode); + } + } + } + + public static FileStreamCompletionSource Create(IFileStreamCompletionSourceStrategy strategy, PreAllocatedOverlapped? preallocatedOverlapped, + int numBufferedBytesRead, ReadOnlyMemory memory) + { + // If the memory passed in is the strategy's internal buffer, we can use the base FileStreamCompletionSource, + // which has a PreAllocatedOverlapped with the memory already pinned. Otherwise, we use the derived + // MemoryFileStreamCompletionSource, which Retains the memory, which will result in less pinning in the case + // where the underlying memory is backed by pre-pinned buffers. + return preallocatedOverlapped != null && MemoryMarshal.TryGetArray(memory, out ArraySegment buffer) + && preallocatedOverlapped._overlapped.IsUserObject(buffer.Array) // preallocatedOverlapped is allocated when BufferedStream|LegacyFileStreamStrategy allocates the buffer + ? new FileStreamCompletionSource(strategy, preallocatedOverlapped, numBufferedBytesRead, buffer.Array) + : new MemoryFileStreamCompletionSource(strategy, numBufferedBytesRead, memory); + } + } + + /// + /// Extends with to support disposing of a + /// when the operation has completed. This should only be used + /// when memory doesn't wrap a byte[]. + /// + internal sealed class MemoryFileStreamCompletionSource : FileStreamCompletionSource + { + private MemoryHandle _handle; // mutable struct; do not make this readonly + + internal MemoryFileStreamCompletionSource(IFileStreamCompletionSourceStrategy strategy, int numBufferedBytes, ReadOnlyMemory memory) + : base(strategy, null, numBufferedBytes, null) // this type handles the pinning, so null is passed for bytes + { + _handle = memory.Pin(); + } + + internal override void ReleaseNativeResource() + { + _handle.Dispose(); + base.ReleaseNativeResource(); + } + } +} diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs index cb1380f69bd77b..dea977581e78eb 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs @@ -1,9 +1,12 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Buffers; using System.Diagnostics; +using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Threading; +using System.Threading.Tasks; using Microsoft.Win32.SafeHandles; namespace System.IO @@ -11,6 +14,11 @@ namespace System.IO // this type defines a set of stateless FileStream/FileStreamStrategy helper methods internal static class FileStreamHelpers { + internal const int ERROR_BROKEN_PIPE = 109; + internal const int ERROR_NO_DATA = 232; + private const int ERROR_HANDLE_EOF = 38; + private const int ERROR_IO_PENDING = 997; + internal static FileStreamStrategy ChooseStrategy(FileStream fileStream, SafeFileHandle handle, FileAccess access, int bufferSize, bool isAsync) => new LegacyFileStreamStrategy(fileStream, handle, access, bufferSize, isAsync); @@ -350,5 +358,243 @@ internal static unsafe int WriteFileNative(SafeFileHandle handle, ReadOnlySpan.Shared.Rent(bufferSize); + + // Allocate an Overlapped we can use repeatedly for all operations + var awaitableOverlapped = new PreAllocatedOverlapped(AsyncCopyToAwaitable.s_callback, readAwaitable, copyBuffer); + var cancellationReg = default(CancellationTokenRegistration); + try + { + // Register for cancellation. We do this once for the whole copy operation, and just try to cancel + // whatever read operation may currently be in progress, if there is one. It's possible the cancellation + // request could come in between operations, in which case we flag that with explicit calls to ThrowIfCancellationRequested + // in the read/write copy loop. + if (cancellationToken.CanBeCanceled) + { + cancellationReg = cancellationToken.UnsafeRegister(static s => + { + Debug.Assert(s is AsyncCopyToAwaitable); + var innerAwaitable = (AsyncCopyToAwaitable)s; + unsafe + { + lock (innerAwaitable.CancellationLock) // synchronize with cleanup of the overlapped + { + if (innerAwaitable._nativeOverlapped != null) + { + // Try to cancel the I/O. We ignore the return value, as cancellation is opportunistic and we + // don't want to fail the operation because we couldn't cancel it. + Interop.Kernel32.CancelIoEx(innerAwaitable._fileHandle, innerAwaitable._nativeOverlapped); + } + } + } + }, readAwaitable); + } + + // Repeatedly read from this FileStream and write the results to the destination stream. + while (true) + { + cancellationToken.ThrowIfCancellationRequested(); + readAwaitable.ResetForNextOperation(); + + try + { + bool synchronousSuccess; + int errorCode; + unsafe + { + // Allocate a native overlapped for our reusable overlapped, and set position to read based on the next + // desired address stored in the awaitable. (This position may be 0, if either we're at the beginning or + // if the stream isn't seekable.) + readAwaitable._nativeOverlapped = handle.ThreadPoolBinding!.AllocateNativeOverlapped(awaitableOverlapped); + if (canSeek) + { + readAwaitable._nativeOverlapped->OffsetLow = unchecked((int)readAwaitable._position); + readAwaitable._nativeOverlapped->OffsetHigh = (int)(readAwaitable._position >> 32); + } + + // Kick off the read. + synchronousSuccess = ReadFileNative(handle, copyBuffer, readAwaitable._nativeOverlapped, out errorCode) >= 0; + } + + // If the operation did not synchronously succeed, it either failed or initiated the asynchronous operation. + if (!synchronousSuccess) + { + switch (errorCode) + { + case ERROR_IO_PENDING: + // Async operation in progress. + break; + case ERROR_BROKEN_PIPE: + case ERROR_HANDLE_EOF: + // We're at or past the end of the file, and the overlapped callback + // won't be raised in these cases. Mark it as completed so that the await + // below will see it as such. + readAwaitable.MarkCompleted(); + break; + default: + // Everything else is an error (and there won't be a callback). + throw Win32Marshal.GetExceptionForWin32Error(errorCode, path); + } + } + + // Wait for the async operation (which may or may not have already completed), then throw if it failed. + await readAwaitable; + switch (readAwaitable._errorCode) + { + case 0: // success + break; + case ERROR_BROKEN_PIPE: // logically success with 0 bytes read (write end of pipe closed) + case ERROR_HANDLE_EOF: // logically success with 0 bytes read (read at end of file) + Debug.Assert(readAwaitable._numBytes == 0, $"Expected 0 bytes read, got {readAwaitable._numBytes}"); + break; + case Interop.Errors.ERROR_OPERATION_ABORTED: // canceled + throw new OperationCanceledException(cancellationToken.IsCancellationRequested ? cancellationToken : new CancellationToken(true)); + default: // error + throw Win32Marshal.GetExceptionForWin32Error((int)readAwaitable._errorCode, path); + } + + // Successful operation. If we got zero bytes, we're done: exit the read/write loop. + int numBytesRead = (int)readAwaitable._numBytes; + if (numBytesRead == 0) + { + break; + } + + // Otherwise, update the read position for next time accordingly. + if (canSeek) + { + readAwaitable._position += numBytesRead; + } + } + finally + { + // Free the resources for this read operation + unsafe + { + NativeOverlapped* overlapped; + lock (readAwaitable.CancellationLock) // just an Exchange, but we need this to be synchronized with cancellation, so using the same lock + { + overlapped = readAwaitable._nativeOverlapped; + readAwaitable._nativeOverlapped = null; + } + if (overlapped != null) + { + handle.ThreadPoolBinding!.FreeNativeOverlapped(overlapped); + } + } + } + + // Write out the read data. + await destination.WriteAsync(new ReadOnlyMemory(copyBuffer, 0, (int)readAwaitable._numBytes), cancellationToken).ConfigureAwait(false); + } + } + finally + { + // Cleanup from the whole copy operation + cancellationReg.Dispose(); + awaitableOverlapped.Dispose(); + + ArrayPool.Shared.Return(copyBuffer); + } + } + + /// Used by AsyncWindowsFileStreamStrategy and LegacyFileStreamStrategy CopyToAsync to enable awaiting the result of an overlapped I/O operation with minimal overhead. + private sealed unsafe class AsyncCopyToAwaitable : ICriticalNotifyCompletion + { + /// Sentinel object used to indicate that the I/O operation has completed before being awaited. + private static readonly Action s_sentinel = () => { }; + /// Cached delegate to IOCallback. + internal static readonly IOCompletionCallback s_callback = IOCallback; + + internal readonly SafeFileHandle _fileHandle; + + /// Tracked position representing the next location from which to read. + internal long _position; + /// The current native overlapped pointer. This changes for each operation. + internal NativeOverlapped* _nativeOverlapped; + /// + /// null if the operation is still in progress, + /// s_sentinel if the I/O operation completed before the await, + /// s_callback if it completed after the await yielded. + /// + internal Action? _continuation; + /// Last error code from completed operation. + internal uint _errorCode; + /// Last number of read bytes from completed operation. + internal uint _numBytes; + + /// Lock object used to protect cancellation-related access to _nativeOverlapped. + internal object CancellationLock => this; + + /// Initialize the awaitable. + internal AsyncCopyToAwaitable(SafeFileHandle fileHandle) => _fileHandle = fileHandle; + + /// Reset state to prepare for the next read operation. + internal void ResetForNextOperation() + { + Debug.Assert(_position >= 0, $"Expected non-negative position, got {_position}"); + _continuation = null; + _errorCode = 0; + _numBytes = 0; + } + + /// Overlapped callback: store the results, then invoke the continuation delegate. + internal static void IOCallback(uint errorCode, uint numBytes, NativeOverlapped* pOVERLAP) + { + var awaitable = (AsyncCopyToAwaitable?)ThreadPoolBoundHandle.GetNativeOverlappedState(pOVERLAP); + Debug.Assert(awaitable != null); + + Debug.Assert(!ReferenceEquals(awaitable._continuation, s_sentinel), "Sentinel must not have already been set as the continuation"); + awaitable._errorCode = errorCode; + awaitable._numBytes = numBytes; + + (awaitable._continuation ?? Interlocked.CompareExchange(ref awaitable._continuation, s_sentinel, null))?.Invoke(); + } + + /// + /// Called when it's known that the I/O callback for an operation will not be invoked but we'll + /// still be awaiting the awaitable. + /// + internal void MarkCompleted() + { + Debug.Assert(_continuation == null, "Expected null continuation"); + _continuation = s_sentinel; + } + + public AsyncCopyToAwaitable GetAwaiter() => this; + public bool IsCompleted => ReferenceEquals(_continuation, s_sentinel); + public void GetResult() { } + public void OnCompleted(Action continuation) => UnsafeOnCompleted(continuation); + public void UnsafeOnCompleted(Action continuation) + { + if (ReferenceEquals(_continuation, s_sentinel) || + Interlocked.CompareExchange(ref _continuation, continuation, null) != null) + { + Debug.Assert(ReferenceEquals(_continuation, s_sentinel), $"Expected continuation set to s_sentinel, got ${_continuation}"); + Task.Run(continuation); + } + } + } } } 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 b0c3bfb5353ca8..dd34b3b3d66485 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 @@ -1,12 +1,10 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.Buffers; using System.Diagnostics; using System.Threading; using System.Threading.Tasks; using Microsoft.Win32.SafeHandles; -using System.Runtime.CompilerServices; /* * Win32FileStream supports different modes of accessing the disk - async mode @@ -38,7 +36,7 @@ namespace System.IO { - internal sealed partial class LegacyFileStreamStrategy : FileStreamStrategy + internal sealed partial class LegacyFileStreamStrategy : FileStreamStrategy, IFileStreamCompletionSourceStrategy { private bool _canSeek; private bool _isPipe; // Whether to disable async buffering code. @@ -340,11 +338,6 @@ private unsafe void SetLengthCore(long value) } } - // Instance method to help code external to this MarshalByRefObject avoid - // accessing its fields by ref. This avoids a compiler warning. - private FileStreamCompletionSource? CompareExchangeCurrentOverlappedOwner(FileStreamCompletionSource? newSource, FileStreamCompletionSource? existingSource) => - Interlocked.CompareExchange(ref _currentOverlappedOwner, newSource, existingSource); - private int ReadSpan(Span destination) { Debug.Assert(!_useAsyncIO, "Must only be used when in synchronous mode"); @@ -556,6 +549,13 @@ partial void OnBufferAllocated() _preallocatedOverlapped = new PreAllocatedOverlapped(s_ioCallback, this, _buffer); } + SafeFileHandle IFileStreamCompletionSourceStrategy.FileHandle => _fileHandle; + + FileStreamCompletionSource? IFileStreamCompletionSourceStrategy.CurrentOverlappedOwner => _currentOverlappedOwner; + + FileStreamCompletionSource? IFileStreamCompletionSourceStrategy.CompareExchangeCurrentOverlappedOwner(FileStreamCompletionSource? newSource, FileStreamCompletionSource? existingSource) + => Interlocked.CompareExchange(ref _currentOverlappedOwner, newSource, existingSource); + private void WriteSpan(ReadOnlySpan source) { Debug.Assert(!_useAsyncIO, "Must only be used when in synchronous mode"); @@ -763,7 +763,7 @@ private unsafe Task ReadNativeAsync(Memory destination, int numBuffer Debug.Assert(_useAsyncIO, "ReadNativeAsync doesn't work on synchronous file streams!"); // Create and store async stream class library specific data in the async result - FileStreamCompletionSource completionSource = FileStreamCompletionSource.Create(this, numBufferedBytesRead, destination); + FileStreamCompletionSource completionSource = FileStreamCompletionSource.Create(this, _preallocatedOverlapped, numBufferedBytesRead, destination); NativeOverlapped* intOverlapped = completionSource.Overlapped; // Calculate position in the file we should be at after the read is done @@ -981,7 +981,7 @@ private unsafe Task WriteAsyncInternalCore(ReadOnlyMemory source, Cancella Debug.Assert(_useAsyncIO, "WriteInternalCoreAsync doesn't work on synchronous file streams!"); // Create and store async stream class library specific data in the async result - FileStreamCompletionSource completionSource = FileStreamCompletionSource.Create(this, 0, source); + FileStreamCompletionSource completionSource = FileStreamCompletionSource.Create(this, _preallocatedOverlapped, 0, source); NativeOverlapped* intOverlapped = completionSource.Overlapped; if (CanSeek) @@ -1147,164 +1147,20 @@ private async Task AsyncModeCopyToAsync(Stream destination, int bufferSize, Canc } } - // For efficiency, we avoid creating a new task and associated state for each asynchronous read. - // Instead, we create a single reusable awaitable object that will be triggered when an await completes - // and reset before going again. - var readAwaitable = new AsyncCopyToAwaitable(this); - - // Make sure we are reading from the position that we think we are. - // Only set the position in the awaitable if we can seek (e.g. not for pipes). bool canSeek = CanSeek; if (canSeek) { VerifyOSHandlePosition(); - readAwaitable._position = _filePosition; } - // Get the buffer to use for the copy operation, as the base CopyToAsync does. We don't try to use - // _buffer here, even if it's not null, as concurrent operations are allowed, and another operation may - // actually be using the buffer already. Plus, it'll be rare for _buffer to be non-null, as typically - // CopyToAsync is used as the only operation performed on the stream, and the buffer is lazily initialized. - // Further, typically the CopyToAsync buffer size will be larger than that used by the FileStream, such that - // we'd likely be unable to use it anyway. Instead, we rent the buffer from a pool. - byte[] copyBuffer = ArrayPool.Shared.Rent(bufferSize); - - // Allocate an Overlapped we can use repeatedly for all operations - var awaitableOverlapped = new PreAllocatedOverlapped(AsyncCopyToAwaitable.s_callback, readAwaitable, copyBuffer); - var cancellationReg = default(CancellationTokenRegistration); try { - // Register for cancellation. We do this once for the whole copy operation, and just try to cancel - // whatever read operation may currently be in progress, if there is one. It's possible the cancellation - // request could come in between operations, in which case we flag that with explicit calls to ThrowIfCancellationRequested - // in the read/write copy loop. - if (cancellationToken.CanBeCanceled) - { - cancellationReg = cancellationToken.UnsafeRegister(static s => - { - Debug.Assert(s is AsyncCopyToAwaitable); - var innerAwaitable = (AsyncCopyToAwaitable)s; - unsafe - { - lock (innerAwaitable.CancellationLock) // synchronize with cleanup of the overlapped - { - if (innerAwaitable._nativeOverlapped != null) - { - // Try to cancel the I/O. We ignore the return value, as cancellation is opportunistic and we - // don't want to fail the operation because we couldn't cancel it. - Interop.Kernel32.CancelIoEx(innerAwaitable._fileStream._fileHandle, innerAwaitable._nativeOverlapped); - } - } - } - }, readAwaitable); - } - - // Repeatedly read from this FileStream and write the results to the destination stream. - while (true) - { - cancellationToken.ThrowIfCancellationRequested(); - readAwaitable.ResetForNextOperation(); - - try - { - bool synchronousSuccess; - int errorCode; - unsafe - { - // Allocate a native overlapped for our reusable overlapped, and set position to read based on the next - // desired address stored in the awaitable. (This position may be 0, if either we're at the beginning or - // if the stream isn't seekable.) - readAwaitable._nativeOverlapped = _fileHandle.ThreadPoolBinding!.AllocateNativeOverlapped(awaitableOverlapped); - if (canSeek) - { - readAwaitable._nativeOverlapped->OffsetLow = unchecked((int)readAwaitable._position); - readAwaitable._nativeOverlapped->OffsetHigh = (int)(readAwaitable._position >> 32); - } - - // Kick off the read. - synchronousSuccess = ReadFileNative(_fileHandle, copyBuffer, readAwaitable._nativeOverlapped, out errorCode) >= 0; - } - - // If the operation did not synchronously succeed, it either failed or initiated the asynchronous operation. - if (!synchronousSuccess) - { - switch (errorCode) - { - case ERROR_IO_PENDING: - // Async operation in progress. - break; - case ERROR_BROKEN_PIPE: - case ERROR_HANDLE_EOF: - // We're at or past the end of the file, and the overlapped callback - // won't be raised in these cases. Mark it as completed so that the await - // below will see it as such. - readAwaitable.MarkCompleted(); - break; - default: - // Everything else is an error (and there won't be a callback). - throw Win32Marshal.GetExceptionForWin32Error(errorCode, _path); - } - } - - // Wait for the async operation (which may or may not have already completed), then throw if it failed. - await readAwaitable; - switch (readAwaitable._errorCode) - { - case 0: // success - break; - case ERROR_BROKEN_PIPE: // logically success with 0 bytes read (write end of pipe closed) - case ERROR_HANDLE_EOF: // logically success with 0 bytes read (read at end of file) - Debug.Assert(readAwaitable._numBytes == 0, $"Expected 0 bytes read, got {readAwaitable._numBytes}"); - break; - case Interop.Errors.ERROR_OPERATION_ABORTED: // canceled - throw new OperationCanceledException(cancellationToken.IsCancellationRequested ? cancellationToken : new CancellationToken(true)); - default: // error - throw Win32Marshal.GetExceptionForWin32Error((int)readAwaitable._errorCode, _path); - } - - // Successful operation. If we got zero bytes, we're done: exit the read/write loop. - int numBytesRead = (int)readAwaitable._numBytes; - if (numBytesRead == 0) - { - break; - } - - // Otherwise, update the read position for next time accordingly. - if (canSeek) - { - readAwaitable._position += numBytesRead; - } - } - finally - { - // Free the resources for this read operation - unsafe - { - NativeOverlapped* overlapped; - lock (readAwaitable.CancellationLock) // just an Exchange, but we need this to be synchronized with cancellation, so using the same lock - { - overlapped = readAwaitable._nativeOverlapped; - readAwaitable._nativeOverlapped = null; - } - if (overlapped != null) - { - _fileHandle.ThreadPoolBinding!.FreeNativeOverlapped(overlapped); - } - } - } - - // Write out the read data. - await destination.WriteAsync(new ReadOnlyMemory(copyBuffer, 0, (int)readAwaitable._numBytes), cancellationToken).ConfigureAwait(false); - } +#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 } finally { - // Cleanup from the whole copy operation - cancellationReg.Dispose(); - awaitableOverlapped.Dispose(); - - ArrayPool.Shared.Return(copyBuffer); - // Make sure the stream's current position reflects where we ended up if (!_fileHandle.IsClosed && CanSeek) { @@ -1313,88 +1169,6 @@ private async Task AsyncModeCopyToAsync(Stream destination, int bufferSize, Canc } } - /// Used by CopyToAsync to enable awaiting the result of an overlapped I/O operation with minimal overhead. - private sealed unsafe class AsyncCopyToAwaitable : ICriticalNotifyCompletion - { - /// Sentinel object used to indicate that the I/O operation has completed before being awaited. - private static readonly Action s_sentinel = () => { }; - /// Cached delegate to IOCallback. - internal static readonly IOCompletionCallback s_callback = IOCallback; - - /// The FileStream that owns this instance. - internal readonly LegacyFileStreamStrategy _fileStream; - - /// Tracked position representing the next location from which to read. - internal long _position; - /// The current native overlapped pointer. This changes for each operation. - internal NativeOverlapped* _nativeOverlapped; - /// - /// null if the operation is still in progress, - /// s_sentinel if the I/O operation completed before the await, - /// s_callback if it completed after the await yielded. - /// - internal Action? _continuation; - /// Last error code from completed operation. - internal uint _errorCode; - /// Last number of read bytes from completed operation. - internal uint _numBytes; - - /// Lock object used to protect cancellation-related access to _nativeOverlapped. - internal object CancellationLock => this; - - /// Initialize the awaitable. - internal AsyncCopyToAwaitable(LegacyFileStreamStrategy fileStream) - { - _fileStream = fileStream; - } - - /// Reset state to prepare for the next read operation. - internal void ResetForNextOperation() - { - Debug.Assert(_position >= 0, $"Expected non-negative position, got {_position}"); - _continuation = null; - _errorCode = 0; - _numBytes = 0; - } - - /// Overlapped callback: store the results, then invoke the continuation delegate. - internal static void IOCallback(uint errorCode, uint numBytes, NativeOverlapped* pOVERLAP) - { - var awaitable = (AsyncCopyToAwaitable?)ThreadPoolBoundHandle.GetNativeOverlappedState(pOVERLAP); - Debug.Assert(awaitable != null); - - Debug.Assert(!ReferenceEquals(awaitable._continuation, s_sentinel), "Sentinel must not have already been set as the continuation"); - awaitable._errorCode = errorCode; - awaitable._numBytes = numBytes; - - (awaitable._continuation ?? Interlocked.CompareExchange(ref awaitable._continuation, s_sentinel, null))?.Invoke(); - } - - /// - /// Called when it's known that the I/O callback for an operation will not be invoked but we'll - /// still be awaiting the awaitable. - /// - internal void MarkCompleted() - { - Debug.Assert(_continuation == null, "Expected null continuation"); - _continuation = s_sentinel; - } - - public AsyncCopyToAwaitable GetAwaiter() => this; - public bool IsCompleted => ReferenceEquals(_continuation, s_sentinel); - public void GetResult() { } - public void OnCompleted(Action continuation) => UnsafeOnCompleted(continuation); - public void UnsafeOnCompleted(Action continuation) - { - if (ReferenceEquals(_continuation, s_sentinel) || - Interlocked.CompareExchange(ref _continuation, continuation, null) != null) - { - Debug.Assert(ReferenceEquals(_continuation, s_sentinel), $"Expected continuation set to s_sentinel, got ${_continuation}"); - Task.Run(continuation); - } - } - } - internal override void Lock(long position, long length) => FileStreamHelpers.Lock(_fileHandle, _path, position, length); internal override void Unlock(long position, long length) => FileStreamHelpers.Unlock(_fileHandle, _path, position, length); From 5e87d72e53664ed361cce0f3557030cea895d3f6 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 26 Feb 2021 12:09:28 +0100 Subject: [PATCH 04/20] some minor improvements after reading the code again --- .../src/System/IO/FileStreamCompletionSource.Windows.cs | 2 +- .../src/System/IO/LegacyFileStreamStrategy.Windows.cs | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamCompletionSource.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamCompletionSource.Windows.cs index eeea50a56f5a60..da3075211567db 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamCompletionSource.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamCompletionSource.Windows.cs @@ -45,7 +45,7 @@ internal unsafe class FileStreamCompletionSource : TaskCompletionSource #if DEBUG private bool _cancellationHasBeenRegistered; #endif - internal NativeOverlapped* _overlapped; // Overlapped class responsible for operations in progress when an appdomain unload occurs + private NativeOverlapped* _overlapped; // Overlapped class responsible for operations in progress when an appdomain unload occurs private long _result; // Using long since this needs to be used in Interlocked APIs // Using RunContinuationsAsynchronously for compat reasons (old API used Task.Factory.StartNew for continuations) 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 dd34b3b3d66485..3849f620ebfc30 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 @@ -42,8 +42,6 @@ internal sealed partial class LegacyFileStreamStrategy : FileStreamStrategy, IFi private bool _isPipe; // Whether to disable async buffering code. private long _appendStart; // When appending, prevent overwriting file. - private static readonly unsafe IOCompletionCallback s_ioCallback = FileStreamCompletionSource.IOCallback; - private Task _activeBufferOperation = Task.CompletedTask; // tracks in-progress async ops using the buffer private PreAllocatedOverlapped? _preallocatedOverlapped; // optimization for async ops to avoid per-op allocations private FileStreamCompletionSource? _currentOverlappedOwner; // async op currently using the preallocated overlapped @@ -546,7 +544,7 @@ partial void OnBufferAllocated() Debug.Assert(_preallocatedOverlapped == null); if (_useAsyncIO) - _preallocatedOverlapped = new PreAllocatedOverlapped(s_ioCallback, this, _buffer); + _preallocatedOverlapped = new PreAllocatedOverlapped(FileStreamCompletionSource.s_ioCallback, this, _buffer); } SafeFileHandle IFileStreamCompletionSourceStrategy.FileHandle => _fileHandle; From f5c247000344f12c243ee0fb7cc33df10c7e62ed Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 26 Feb 2021 12:20:55 +0100 Subject: [PATCH 05/20] only DerivedFileStreamStrategy needs to have a reference to FileStream remove finalizer from FileStream, keep it only in DerivedFileStreamStrategy and LegacyFileStreamStrategy --- .../System/IO/DerivedFileStreamStrategy.cs | 34 +++++++++++++++++-- .../src/System/IO/FileStream.cs | 19 +++-------- .../src/System/IO/FileStreamHelpers.Unix.cs | 8 ++--- .../System/IO/FileStreamHelpers.Windows.cs | 8 ++--- .../src/System/IO/FileStreamStrategy.cs | 4 --- .../src/System/IO/LegacyFileStreamStrategy.cs | 14 +++----- .../System/IO/WindowsFileStreamStrategy.cs | 2 -- 7 files changed, 48 insertions(+), 41 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/DerivedFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/DerivedFileStreamStrategy.cs index 76d4441309f7b9..e7dd824305f495 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/DerivedFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/DerivedFileStreamStrategy.cs @@ -15,8 +15,21 @@ namespace System.IO internal sealed class DerivedFileStreamStrategy : FileStreamStrategy { private readonly FileStreamStrategy _strategy; + private readonly FileStream _fileStream; - internal DerivedFileStreamStrategy(FileStream fileStream, FileStreamStrategy strategy) : base(fileStream) => _strategy = strategy; + internal DerivedFileStreamStrategy(FileStream fileStream, FileStreamStrategy strategy) + { + _fileStream = fileStream; + _strategy = strategy; + } + + ~DerivedFileStreamStrategy() + { + // Preserved for compatibility since FileStream has defined a + // finalizer in past releases and derived classes may depend + // on Dispose(false) call. + _fileStream.DisposeInternal(false); + } public override bool CanRead => _strategy.CanRead; @@ -36,7 +49,14 @@ public override long Position internal override string Name => _strategy.Name; - internal override SafeFileHandle SafeFileHandle => _strategy.SafeFileHandle; + internal override SafeFileHandle SafeFileHandle + { + get + { + _fileStream.Flush(false); + return _strategy.SafeFileHandle; + } + } internal override bool IsClosed => _strategy.IsClosed; @@ -136,6 +156,14 @@ public override Task CopyToAsync(Stream destination, int bufferSize, Cancellatio public override ValueTask DisposeAsync() => _fileStream.BaseDisposeAsync(); - internal override void DisposeInternal(bool disposing) => _strategy.DisposeInternal(disposing); + internal override void DisposeInternal(bool disposing) + { + _strategy.DisposeInternal(disposing); + + if (disposing) + { + GC.SuppressFinalize(this); + } + } } } 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 078b0bd0a41860..03620f33bf5552 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs @@ -46,7 +46,7 @@ public FileStream(IntPtr handle, FileAccess access, bool ownsHandle, int bufferS { ValidateHandle(safeHandle, access, bufferSize, isAsync); - _strategy = WrapIfDerivedType(FileStreamHelpers.ChooseStrategy(this, safeHandle, access, bufferSize, isAsync)); + _strategy = WrapIfDerivedType(FileStreamHelpers.ChooseStrategy(safeHandle, access, bufferSize, isAsync)); } catch { @@ -95,7 +95,7 @@ public FileStream(SafeFileHandle handle, FileAccess access, int bufferSize, bool { ValidateHandle(handle, access, bufferSize, isAsync); - _strategy = WrapIfDerivedType(FileStreamHelpers.ChooseStrategy(this, handle, access, bufferSize, isAsync)); + _strategy = WrapIfDerivedType(FileStreamHelpers.ChooseStrategy(handle, access, bufferSize, isAsync)); } public FileStream(string path, FileMode mode) : @@ -164,7 +164,7 @@ public FileStream(string path, FileMode mode, FileAccess access, FileShare share SerializationInfo.ThrowIfDeserializationInProgress("AllowFileWrites", ref s_cachedSerializationSwitch); } - _strategy = WrapIfDerivedType(FileStreamHelpers.ChooseStrategy(this, path, mode, access, share, bufferSize, options)); + _strategy = WrapIfDerivedType(FileStreamHelpers.ChooseStrategy(path, mode, access, share, bufferSize, options)); } [Obsolete("This property has been deprecated. Please use FileStream's SafeFileHandle property instead. https://go.microsoft.com/fwlink/?linkid=14202")] @@ -397,18 +397,9 @@ public override long Position /// The byte to write to the stream. public override void WriteByte(byte value) => _strategy.WriteByte(value); - ~FileStream() - { - // Preserved for compatibility since FileStream has defined a - // finalizer in past releases and derived classes may depend - // on Dispose(false) call. - Dispose(false); - } + protected override void Dispose(bool disposing) => _strategy?.DisposeInternal(disposing); - protected override void Dispose(bool disposing) - { - _strategy?.DisposeInternal(disposing); // null _strategy possible in finalizer - } + internal void DisposeInternal(bool disposing) => Dispose(disposing); public override ValueTask DisposeAsync() => _strategy.DisposeAsync(); diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Unix.cs index 471005b833f746..25a09f1ab89403 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Unix.cs @@ -14,11 +14,11 @@ namespace System.IO internal static class FileStreamHelpers { // in the future we are most probably going to introduce more strategies (io_uring etc) - internal static FileStreamStrategy ChooseStrategy(FileStream fileStream, SafeFileHandle handle, FileAccess access, int bufferSize, bool isAsync) - => new LegacyFileStreamStrategy(fileStream, handle, access, bufferSize, isAsync); + internal static FileStreamStrategy ChooseStrategy(SafeFileHandle handle, FileAccess access, int bufferSize, bool isAsync) + => new LegacyFileStreamStrategy(handle, access, bufferSize, isAsync); - internal static FileStreamStrategy ChooseStrategy(FileStream fileStream, string path, FileMode mode, FileAccess access, FileShare share, int bufferSize, FileOptions options) - => new LegacyFileStreamStrategy(fileStream, path, mode, access, share, bufferSize, options); + internal static FileStreamStrategy ChooseStrategy(string path, FileMode mode, FileAccess access, FileShare share, int bufferSize, FileOptions options) + => new LegacyFileStreamStrategy(path, mode, access, share, bufferSize, options); internal static SafeFileHandle OpenHandle(string path, FileMode mode, FileAccess access, FileShare share, FileOptions options) { diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs index dea977581e78eb..c701b308e0071b 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs @@ -19,11 +19,11 @@ internal static class FileStreamHelpers private const int ERROR_HANDLE_EOF = 38; private const int ERROR_IO_PENDING = 997; - internal static FileStreamStrategy ChooseStrategy(FileStream fileStream, SafeFileHandle handle, FileAccess access, int bufferSize, bool isAsync) - => new LegacyFileStreamStrategy(fileStream, handle, access, bufferSize, isAsync); + internal static FileStreamStrategy ChooseStrategy(SafeFileHandle handle, FileAccess access, int bufferSize, bool isAsync) + => new LegacyFileStreamStrategy(handle, access, bufferSize, isAsync); - internal static FileStreamStrategy ChooseStrategy(FileStream fileStream, string path, FileMode mode, FileAccess access, FileShare share, int bufferSize, FileOptions options) - => new LegacyFileStreamStrategy(fileStream, path, mode, access, share, bufferSize, options); + internal static FileStreamStrategy ChooseStrategy(string path, FileMode mode, FileAccess access, FileShare share, int bufferSize, FileOptions options) + => new LegacyFileStreamStrategy(path, mode, access, share, bufferSize, options); internal static SafeFileHandle OpenHandle(string path, FileMode mode, FileAccess access, FileShare share, FileOptions options) => CreateFileOpenHandle(path, mode, access, share, options); diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamStrategy.cs index 97b5969355383c..54166cd68d82fa 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamStrategy.cs @@ -7,10 +7,6 @@ namespace System.IO { internal abstract class FileStreamStrategy : Stream { - protected readonly FileStream _fileStream; - - protected FileStreamStrategy(FileStream fileStream) => _fileStream = fileStream; - internal abstract bool IsAsync { get; } internal abstract string Name { get; } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/LegacyFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/LegacyFileStreamStrategy.cs index 01e5db4f0827c0..ea7b982da30b34 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/LegacyFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/LegacyFileStreamStrategy.cs @@ -61,7 +61,7 @@ internal sealed partial class LegacyFileStreamStrategy : FileStreamStrategy /// Whether the file stream's handle has been exposed. private bool _exposedHandle; - internal LegacyFileStreamStrategy(FileStream fileStream, SafeFileHandle handle, FileAccess access, int bufferSize, bool isAsync) : base(fileStream) + internal LegacyFileStreamStrategy(SafeFileHandle handle, FileAccess access, int bufferSize, bool isAsync) { _exposedHandle = true; _bufferLength = bufferSize; @@ -78,7 +78,7 @@ internal LegacyFileStreamStrategy(FileStream fileStream, SafeFileHandle handle, _fileHandle = handle; } - internal LegacyFileStreamStrategy(FileStream fileStream, string path, FileMode mode, FileAccess access, FileShare share, int bufferSize, FileOptions options) : base(fileStream) + internal LegacyFileStreamStrategy(string path, FileMode mode, FileAccess access, FileShare share, int bufferSize, FileOptions options) { string fullPath = Path.GetFullPath(path); @@ -105,12 +105,7 @@ internal LegacyFileStreamStrategy(FileStream fileStream, string path, FileMode m } } - ~LegacyFileStreamStrategy() - { - // it looks like having this finalizer is mandatory, - // as we can not guarantee that the Strategy won't be null in FileStream finalizer - Dispose(false); - } + ~LegacyFileStreamStrategy() => Dispose(false); // mandatory to Flush the write buffer internal override void DisposeInternal(bool disposing) => Dispose(disposing); @@ -272,8 +267,7 @@ public override ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationTo return WriteAsyncInternal(buffer, cancellationToken); } - // this method might call Derived type implenentation of Flush(flushToDisk) - public override void Flush() => _fileStream.Flush(); + public override void Flush() => Flush(flushToDisk: false); internal override void Flush(bool flushToDisk) { 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 a5655cc3ac1e2c..b04658c955f0be 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/WindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/WindowsFileStreamStrategy.cs @@ -123,8 +123,6 @@ public override ValueTask DisposeAsync() _fileHandle.Dispose(); } - GC.SuppressFinalize(this); // the handle is closed; nothing further for the finalizer to do - return ValueTask.CompletedTask; } From 71e7d4349785a99dd65e8482a52c3ea5ba497cdc Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 26 Feb 2021 16:01:36 +0100 Subject: [PATCH 06/20] implement ReadByte and WriteByte to make new windows strategies fully functional they can now be used directly without any buffering on top of them! --- .../System/IO/AsyncWindowsFileStreamStrategy.cs | 12 ++++++++++-- .../System/IO/SyncWindowsFileStreamStrategy.cs | 12 ++++++++++-- .../src/System/IO/WindowsFileStreamStrategy.cs | 17 +++++++++++++++++ 3 files changed, 37 insertions(+), 4 deletions(-) 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 9d21309fd21075..4db3309cc075a3 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/AsyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/AsyncWindowsFileStreamStrategy.cs @@ -129,7 +129,11 @@ public override ValueTask ReadAsync(Memory destination, CancellationT private unsafe Task ReadAsyncInternal(Memory destination, CancellationToken cancellationToken = default) { - Debug.Assert(CanRead, "BufferedStream has already verified that"); + if (!CanRead) + { + throw Error.GetReadNotSupported(); + } + Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed"); // Create and store async stream class library specific data in the async result @@ -251,7 +255,11 @@ private ValueTask WriteAsyncInternal(ReadOnlyMemory source, CancellationTo private unsafe Task WriteAsyncInternalCore(ReadOnlyMemory source, CancellationToken cancellationToken) { - Debug.Assert(CanWrite, "BufferedStream has already verified that"); + if (!CanWrite) + { + throw Error.GetWriteNotSupported(); + } + Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed"); // Create and store async stream class library specific data in the async result diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/SyncWindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/SyncWindowsFileStreamStrategy.cs index 99a64e961cc123..4bf7e208e56d73 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/SyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/SyncWindowsFileStreamStrategy.cs @@ -95,7 +95,11 @@ public override ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationTo private unsafe int ReadSpan(Span destination) { - Debug.Assert(CanRead, "BufferedStream has already verified that"); + if (!CanRead) + { + throw Error.GetReadNotSupported(); + } + Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed"); // Make sure we are reading from the right spot @@ -126,7 +130,11 @@ private unsafe int ReadSpan(Span destination) private unsafe void WriteSpan(ReadOnlySpan source) { - Debug.Assert(CanWrite, "BufferedStream has already verified that"); + if (!CanWrite) + { + throw Error.GetWriteNotSupported(); + } + Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed"); // Make sure we are writing to the position that we think we are 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 b04658c955f0be..a3100794ca3d8d 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/WindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/WindowsFileStreamStrategy.cs @@ -113,6 +113,23 @@ internal sealed override SafeFileHandle SafeFileHandle } } + // ReadByte and WriteByte methods are used only when the user has disabled buffering on purpose + // their performance is going to be horrible + // TODO: should we consider adding a new event provider and log an event so it can be detected? + public override int ReadByte() + { + Span buffer = stackalloc byte[1]; + int bytesRead = Read(buffer); + return bytesRead == 1 ? buffer[0] : -1; + } + + public override void WriteByte(byte value) + { + Span buffer = stackalloc byte[1]; + buffer[0] = value; + Write(buffer); + } + // this method just disposes everything as there is no buffer here // and we don't really need to Flush anything here public override ValueTask DisposeAsync() From 367ad84c9c256b6a4c34d124abb4cbde8e7ff614 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 26 Feb 2021 16:49:33 +0100 Subject: [PATCH 07/20] implement FlushAsync for no buffering strategies as nop to get Flush_NothingToFlush_CompletesSynchronously passing --- .../src/System/IO/AsyncWindowsFileStreamStrategy.cs | 2 ++ .../src/System/IO/SyncWindowsFileStreamStrategy.cs | 2 ++ 2 files changed, 4 insertions(+) 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 4db3309cc075a3..ef77a3c0a52d6d 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/AsyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/AsyncWindowsFileStreamStrategy.cs @@ -250,6 +250,8 @@ public override Task WriteAsync(byte[] buffer, int offset, int count, Cancellati public override ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationToken cancellationToken = default) => WriteAsyncInternal(buffer, cancellationToken); + public override Task FlushAsync(CancellationToken cancellationToken) => Task.CompletedTask; // no buffering = nothing to flush + private ValueTask WriteAsyncInternal(ReadOnlyMemory source, CancellationToken cancellationToken) => new ValueTask(WriteAsyncInternalCore(source, cancellationToken)); diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/SyncWindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/SyncWindowsFileStreamStrategy.cs index 4bf7e208e56d73..44563299c579f4 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/SyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/SyncWindowsFileStreamStrategy.cs @@ -93,6 +93,8 @@ public override ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationTo base.WriteAsync(buffer, cancellationToken); } + public override Task FlushAsync(CancellationToken cancellationToken) => Task.CompletedTask; // no buffering = nothing to flush + private unsafe int ReadSpan(Span destination) { if (!CanRead) From 5579e9431d12a236b0807bcdbaa7971253254d0e Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 26 Feb 2021 16:53:30 +0100 Subject: [PATCH 08/20] use the new strategies when buffering is not enabled --- .../System/IO/FileStreamHelpers.Windows.cs | 24 +++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs index c701b308e0071b..6eff2056257ed9 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs @@ -19,11 +19,31 @@ internal static class FileStreamHelpers private const int ERROR_HANDLE_EOF = 38; private const int ERROR_IO_PENDING = 997; + private static readonly bool UseLegacyStrategy = Environment.GetEnvironmentVariable("DOTNET_LEGACY_FILE_IO") == "1"; + internal static FileStreamStrategy ChooseStrategy(SafeFileHandle handle, FileAccess access, int bufferSize, bool isAsync) - => new LegacyFileStreamStrategy(handle, access, bufferSize, isAsync); + { + if (!UseLegacyStrategy && bufferSize == 1) + { + return isAsync + ? new AsyncWindowsFileStreamStrategy(handle, access) + : new SyncWindowsFileStreamStrategy(handle, access); + } + + return new LegacyFileStreamStrategy(handle, access, bufferSize, isAsync); + } internal static FileStreamStrategy ChooseStrategy(string path, FileMode mode, FileAccess access, FileShare share, int bufferSize, FileOptions options) - => new LegacyFileStreamStrategy(path, mode, access, share, bufferSize, options); + { + if (!UseLegacyStrategy && bufferSize == 1) + { + return (options & FileOptions.Asynchronous) != 0 + ? new AsyncWindowsFileStreamStrategy(path, mode, access, share, options) + : new SyncWindowsFileStreamStrategy(path, mode, access, share, options); + } + + return new LegacyFileStreamStrategy(path, mode, access, share, bufferSize, options); + } internal static SafeFileHandle OpenHandle(string path, FileMode mode, FileAccess access, FileShare share, FileOptions options) => CreateFileOpenHandle(path, mode, access, share, options); From 8bbbcee63abbcf2d946e2892b3d7266bb3be6faa Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 26 Feb 2021 17:05:18 +0100 Subject: [PATCH 09/20] introduce BufferedFileStreamStrategy --- .../tests/FileStream/SafeFileHandle.cs | 9 +- .../System.Private.CoreLib.Shared.projitems | 1 + .../IO/AsyncWindowsFileStreamStrategy.cs | 2 +- .../System/IO/BufferedFileStreamStrategy.cs | 132 ++++++++ .../src/System/IO/BufferedStream.cs | 308 +++++++++++++----- .../src/System/IO/FileStream.cs | 16 + .../System/IO/FileStreamHelpers.Windows.cs | 28 +- .../IO/LegacyFileStreamStrategy.Unix.cs | 2 +- .../IO/LegacyFileStreamStrategy.Windows.cs | 2 +- .../src/System/IO/LegacyFileStreamStrategy.cs | 16 - 10 files changed, 405 insertions(+), 111 deletions(-) create mode 100644 src/libraries/System.Private.CoreLib/src/System/IO/BufferedFileStreamStrategy.cs diff --git a/src/libraries/System.IO.FileSystem/tests/FileStream/SafeFileHandle.cs b/src/libraries/System.IO.FileSystem/tests/FileStream/SafeFileHandle.cs index 04773b5a8df72b..020a25e48871ed 100644 --- a/src/libraries/System.IO.FileSystem/tests/FileStream/SafeFileHandle.cs +++ b/src/libraries/System.IO.FileSystem/tests/FileStream/SafeFileHandle.cs @@ -104,14 +104,7 @@ private async Task ThrowWhenHandlePositionIsChanged(bool useAsync) fs.WriteByte(0); fsr.Position++; - if (useAsync && OperatingSystem.IsWindows()) // Async I/O behaviors differ due to kernel-based implementation on Windows - { - Assert.Throws(() => FSAssert.CompletesSynchronously(fs.ReadAsync(new byte[1], 0, 1))); - } - else - { - await Assert.ThrowsAsync(() => fs.ReadAsync(new byte[1], 0, 1)); - } + await Assert.ThrowsAsync(() => fs.ReadAsync(new byte[1], 0, 1)); fs.WriteByte(0); fsr.Position++; diff --git a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems index 2b7a12a56e52d2..635da72ed67a2b 100644 --- a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems +++ b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems @@ -391,6 +391,7 @@ + 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 ef77a3c0a52d6d..dadffb0164ffb6 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/AsyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/AsyncWindowsFileStreamStrategy.cs @@ -102,7 +102,7 @@ protected override void OnInit() } } - // called by BufferedStream. TODO: find a cleaner solution + // called by BufferedStream internal void OnBufferAllocated(byte[] buffer) { Debug.Assert(buffer != null); diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/BufferedFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/BufferedFileStreamStrategy.cs new file mode 100644 index 00000000000000..bf5c50e2cfbfb1 --- /dev/null +++ b/src/libraries/System.Private.CoreLib/src/System/IO/BufferedFileStreamStrategy.cs @@ -0,0 +1,132 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Win32.SafeHandles; + +namespace System.IO +{ + // this type exists so we can avoid duplicating the buffering logic in every FileStreamStrategy implementation + // for simple properties that would just call the wrapped stream properties, we call strategy directly + // for everything else, we are calling BufferedStream methods that take care of all the buffering work + internal sealed class BufferedFileStreamStrategy : FileStreamStrategy + { + private readonly FileStreamStrategy _strategy; + private readonly BufferedStream _bufferedStream; + + internal BufferedFileStreamStrategy(FileStreamStrategy strategy, int bufferSize) + { + _strategy = strategy; + _bufferedStream = new BufferedStream(strategy, bufferSize, actLikeFileStream: true); + } + + ~BufferedFileStreamStrategy() => DisposeInternal(false); + + public override bool CanRead => _strategy.CanRead; + + public override bool CanWrite => _strategy.CanWrite; + + public override bool CanSeek => _strategy.CanSeek; + + public override long Length => _bufferedStream.GetLengthWithoutFlushing(); + + public override long Position + { + get => _bufferedStream.GetPositionWithoutFlushing(); + set => _bufferedStream.Position = value; + } + + internal override bool IsAsync => _strategy.IsAsync; + + internal override string Name => _strategy.Name; + + internal override SafeFileHandle SafeFileHandle + { + get + { + _bufferedStream.Flush(); + return _strategy.SafeFileHandle; + } + } + + internal override bool IsClosed => _strategy.IsClosed; + + internal override void Lock(long position, long length) => _strategy.Lock(position, length); + + internal override void Unlock(long position, long length) => _strategy.Unlock(position, length); + + public override long Seek(long offset, SeekOrigin origin) => _bufferedStream.Seek(offset, origin); + + public override void SetLength(long value) => _bufferedStream.SetLength(value); + + public override int ReadByte() => _bufferedStream.ReadByte(); + + public override IAsyncResult BeginRead(byte[] buffer, int offset, int count, AsyncCallback? callback, object? state) + => _bufferedStream.BeginRead(buffer, offset, count, callback, state); + + public override int EndRead(IAsyncResult asyncResult) + => _bufferedStream.EndRead(asyncResult); + + public override int Read(byte[] buffer, int offset, int count) => _bufferedStream.Read(buffer, offset, count); + + public override int Read(Span buffer) => _bufferedStream.Read(buffer); + + public override Task ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) + => _bufferedStream.ReadAsync(buffer, offset, count, cancellationToken); + + public override ValueTask ReadAsync(Memory buffer, CancellationToken cancellationToken = default) + => _bufferedStream.ReadAsync(buffer, cancellationToken); + + public override IAsyncResult BeginWrite(byte[] buffer, int offset, int count, AsyncCallback? callback, object? state) + => _bufferedStream.BeginWrite(buffer, offset, count, callback, state); + + public override void EndWrite(IAsyncResult asyncResult) + => _bufferedStream.EndWrite(asyncResult); + + public override void WriteByte(byte value) => _bufferedStream.WriteByte(value); + + public override void Write(byte[] buffer, int offset, int count) => _bufferedStream.Write(buffer, offset, count); + + public override void Write(ReadOnlySpan buffer) => _bufferedStream.Write(buffer); + + public override Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) + => _bufferedStream.WriteAsync(buffer, offset, count, cancellationToken); + + public override ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationToken cancellationToken = default) + => _bufferedStream.WriteAsync(buffer, cancellationToken); + + public override void Flush() => _bufferedStream.Flush(); + + internal override void Flush(bool flushToDisk) => _bufferedStream.Flush(flushToDisk); + + public override Task FlushAsync(CancellationToken cancellationToken) + => _bufferedStream.FlushAsync(cancellationToken); + + public override Task CopyToAsync(Stream destination, int bufferSize, CancellationToken cancellationToken) + => _bufferedStream.CopyToAsync(destination, bufferSize, cancellationToken); + + public override ValueTask DisposeAsync() + => _bufferedStream.DisposeAsync(); + + internal override void DisposeInternal(bool disposing) + { + try + { + // the finalizer must at least try to flush the write buffer + // so we enforce it by passing always true + _bufferedStream.DisposeInternal(true); + } + catch (Exception e) when (!disposing && FileStream.IsIoRelatedException(e)) + { + // On finalization, ignore failures from trying to flush the write buffer, + // e.g. if this stream is wrapping a pipe and the pipe is now broken. + } + + if (disposing) + { + GC.SuppressFinalize(this); + } + } + } +} 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 bc1ea76470377d..926a1a2f326ebe 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/BufferedStream.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/BufferedStream.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Diagnostics; +using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Tasks; @@ -51,6 +52,12 @@ public sealed class BufferedStream : Stream private Stream? _stream; // Underlying stream. Close sets _stream to null. private byte[]? _buffer; // Shared read/write buffer. Alloc on first use. private readonly int _bufferSize; // Length of internal buffer (not counting the shadow buffer). + /// + /// allows for: + /// 1. blocking zero byte reads + /// 2. skipping the serialization of async operations that don't use the buffer + /// + private readonly bool _actLikeFileStream; private int _readPos; // Read pointer within shared buffer. private int _readLen; // Number of bytes read in buffer from _stream. private int _writePos; // Write pointer within shared buffer. @@ -82,10 +89,17 @@ public BufferedStream(Stream stream, int bufferSize) throw new ObjectDisposedException(null, SR.ObjectDisposed_StreamClosed); } + internal BufferedStream(Stream stream, int bufferSize, bool actLikeFileStream) : this(stream, bufferSize) + { + _actLikeFileStream = actLikeFileStream; + } + private void EnsureNotClosed() { if (_stream == null) - throw new ObjectDisposedException(null, SR.ObjectDisposed_StreamClosed); + Throw(); + + static void Throw() => throw new ObjectDisposedException(null, SR.ObjectDisposed_StreamClosed); } private void EnsureCanSeek() @@ -93,7 +107,9 @@ private void EnsureCanSeek() Debug.Assert(_stream != null); if (!_stream.CanSeek) - throw new NotSupportedException(SR.NotSupported_UnseekableStream); + Throw(); + + static void Throw() => throw new NotSupportedException(SR.NotSupported_UnseekableStream); } private void EnsureCanRead() @@ -101,7 +117,9 @@ private void EnsureCanRead() Debug.Assert(_stream != null); if (!_stream.CanRead) - throw new NotSupportedException(SR.NotSupported_UnreadableStream); + Throw(); + + static void Throw() => throw new NotSupportedException(SR.NotSupported_UnreadableStream); } private void EnsureCanWrite() @@ -109,7 +127,9 @@ private void EnsureCanWrite() Debug.Assert(_stream != null); if (!_stream.CanWrite) - throw new NotSupportedException(SR.NotSupported_UnwritableStream); + Throw(); + + static void Throw() => throw new NotSupportedException(SR.NotSupported_UnwritableStream); } private void EnsureShadowBufferAllocated() @@ -127,13 +147,23 @@ private void EnsureShadowBufferAllocated() _buffer = shadowBuffer; } + [MethodImpl(MethodImplOptions.AggressiveInlining)] private void EnsureBufferAllocated() { Debug.Assert(_bufferSize > 0); // BufferedStream is not intended for multi-threaded use, so no worries about the get/set race on _buffer. if (_buffer == null) + { _buffer = new byte[_bufferSize]; + +#if TARGET_WINDOWS + if (_actLikeFileStream && _stream is AsyncWindowsFileStreamStrategy asyncStrategy) + { + asyncStrategy.OnBufferAllocated(_buffer); + } +#endif + } } public Stream UnderlyingStream @@ -184,12 +214,27 @@ public override long Length EnsureNotClosed(); if (_writePos > 0) - FlushWrite(); + FlushWrite(true); return _stream!.Length; } } + // this method exists to keep old FileStream behaviour and don't perform a Flush when getting Length + internal long GetLengthWithoutFlushing() + { + Debug.Assert(_actLikeFileStream); + + EnsureNotClosed(); + + long len = _stream!.Length; + + if (_writePos > 0 && _stream!.Position + _writePos > len) + len = _writePos + _stream!.Position; + + return len; + } + public override long Position { get @@ -209,7 +254,7 @@ public override long Position EnsureCanSeek(); if (_writePos > 0) - FlushWrite(); + FlushWrite(true); _readPos = 0; _readLen = 0; @@ -217,6 +262,19 @@ 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) { try @@ -266,16 +324,24 @@ public override async ValueTask DisposeAsync() } } - public override void Flush() + public override void Flush() => Flush(true); + + internal void Flush(bool performActualFlush) { EnsureNotClosed(); // Has write data in the buffer: if (_writePos > 0) { - FlushWrite(); - Debug.Assert(_writePos == 0 && _readPos == 0 && _readLen == 0); - return; + // EnsureNotClosed does not guarantee that the Stream has not been closed + // an example could be a call to fileStream.SafeFileHandle.Dispose() + // so to avoid getting exception here, we just ensure that we can Write before doing it + if (_stream!.CanWrite) + { + FlushWrite(performActualFlush); + Debug.Assert(_writePos == 0 && _readPos == 0 && _readLen == 0); + return; + } } // Has read data in the buffer: @@ -292,7 +358,7 @@ public override void Flush() // 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 (_stream.CanWrite) + if (performActualFlush && _stream.CanWrite) _stream.Flush(); // If the Stream was seekable, then we should have called FlushRead which resets _readPos & _readLen. @@ -301,7 +367,7 @@ public override void Flush() } // We had no data in the buffer, but we still need to tell the underlying stream to flush. - if (_stream!.CanWrite) + if (performActualFlush && _stream!.CanWrite) _stream.Flush(); _writePos = _readPos = _readLen = 0; @@ -314,14 +380,32 @@ public override Task FlushAsync(CancellationToken cancellationToken) EnsureNotClosed(); - return FlushAsyncInternal(cancellationToken); + // try to get the lock and exit in synchronous way if there is nothing to Flush + SemaphoreSlim sem = EnsureAsyncActiveSemaphoreInitialized(); + Task semaphoreLockTask = sem.WaitAsync(cancellationToken); + bool lockAcquired = semaphoreLockTask.IsCompletedSuccessfully; + if (lockAcquired) + { + if (_writePos == 0 && _readPos == _readLen) + { + sem.Release(); + + return CanWrite ? _stream!.FlushAsync(cancellationToken) : Task.CompletedTask; + } + } + + return FlushAsyncInternal(semaphoreLockTask, lockAcquired, cancellationToken); } - private async Task FlushAsyncInternal(CancellationToken cancellationToken) + private async Task FlushAsyncInternal(Task semaphoreLockTask, bool lockAcquired, CancellationToken cancellationToken) { Debug.Assert(_stream != null); - await EnsureAsyncActiveSemaphoreInitialized().WaitAsync(cancellationToken).ConfigureAwait(false); + if (!lockAcquired) + { + await semaphoreLockTask.ConfigureAwait(false); + } + try { if (_writePos > 0) @@ -362,7 +446,7 @@ private async Task FlushAsyncInternal(CancellationToken cancellationToken) } finally { - _asyncActiveSemaphore.Release(); + _asyncActiveSemaphore!.Release(); } } @@ -384,6 +468,7 @@ private void FlushRead() /// /// Called by Write methods to clear the Read Buffer /// + [MethodImpl(MethodImplOptions.AggressiveInlining)] private void ClearReadBufferBeforeWrite() { Debug.Assert(_stream != null); @@ -403,12 +488,14 @@ 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 new NotSupportedException(SR.NotSupported_CannotWriteToBufferedStreamIfReadBufferCannotBeFlushed); + Throw(); FlushRead(); + + static void Throw() => throw new NotSupportedException(SR.NotSupported_CannotWriteToBufferedStreamIfReadBufferCannotBeFlushed); } - private void FlushWrite() + private void FlushWrite(bool performActualFlush) { Debug.Assert(_stream != null); Debug.Assert(_readPos == 0 && _readLen == 0, @@ -418,7 +505,11 @@ private void FlushWrite() _stream.Write(_buffer, 0, _writePos); _writePos = 0; - _stream.Flush(); + + if (performActualFlush) + { + _stream.Flush(); + } } private async ValueTask FlushWriteAsync(CancellationToken cancellationToken) @@ -434,6 +525,7 @@ private async ValueTask FlushWriteAsync(CancellationToken cancellationToken) await _stream.FlushAsync(cancellationToken).ConfigureAwait(false); } + [MethodImpl(MethodImplOptions.AggressiveInlining)] private int ReadFromBuffer(byte[] buffer, int offset, int count) { int readbytes = _readLen - _readPos; @@ -493,7 +585,7 @@ public override int Read(byte[] buffer, int offset, int count) // BUT - this is a breaking change. // So: If we could not read all bytes the user asked for from the buffer, we will try once from the underlying // stream thus ensuring the same blocking behaviour as if the underlying stream was not wrapped in this BufferedStream. - if (bytesFromBuffer == count) + if (bytesFromBuffer == count && (count > 0 || !_actLikeFileStream)) return bytesFromBuffer; int alreadySatisfied = bytesFromBuffer; @@ -509,7 +601,7 @@ public override int Read(byte[] buffer, int offset, int count) // If there was anything in the write buffer, clear it. if (_writePos > 0) - FlushWrite(); + FlushWrite(true); // If the requested read is larger than buffer size, avoid the buffer and still use a single read: if (count >= _bufferSize) @@ -541,7 +633,7 @@ public override int Read(Span destination) // Try to read from the buffer. int bytesFromBuffer = ReadFromBuffer(destination); - if (bytesFromBuffer == destination.Length) + if (bytesFromBuffer == destination.Length && (destination.Length > 0 || !_actLikeFileStream)) { // We got as many bytes as were asked for; we're done. return bytesFromBuffer; @@ -561,7 +653,7 @@ public override int Read(Span destination) // If there was anything in the write buffer, clear it. if (_writePos > 0) { - FlushWrite(); + FlushWrite(true); } if (destination.Length >= _bufferSize) @@ -609,13 +701,13 @@ public override Task ReadAsync(byte[] buffer, int offset, int count, Cancel // an Async operation. SemaphoreSlim sem = EnsureAsyncActiveSemaphoreInitialized(); Task semaphoreLockTask = sem.WaitAsync(cancellationToken); - if (semaphoreLockTask.IsCompletedSuccessfully) + bool locked = semaphoreLockTask.IsCompletedSuccessfully; + if (locked) { - bool completeSynchronously = true; - try + // hot path #1: there is data in the buffer + if (_readLen - _readPos > 0 || (count == 0 && !_actLikeFileStream)) { - Exception? error; - bytesFromBuffer = ReadFromBuffer(buffer, offset, count, out error); + bytesFromBuffer = ReadFromBuffer(buffer, offset, count, out Exception? error); // If we satisfied enough data from the buffer, we can complete synchronously. // Reading again for more data may cause us to block if we're using a device with no clear end of file, @@ -624,27 +716,37 @@ public override Task ReadAsync(byte[] buffer, int offset, int count, Cancel // BUT - this is a breaking change. // So: If we could not read all bytes the user asked for from the buffer, we will try once from the underlying // stream thus ensuring the same blocking behaviour as if the underlying stream was not wrapped in this BufferedStream. - completeSynchronously = (bytesFromBuffer == count || error != null); - - if (completeSynchronously) + if (bytesFromBuffer == count || error != null) { + // if the above is false, we will be entering ReadFromUnderlyingStreamAsync and releasing there. + sem.Release(); return (error == null) - ? LastSyncCompletedReadTask(bytesFromBuffer) - : Task.FromException(error); + ? LastSyncCompletedReadTask(bytesFromBuffer) + : Task.FromException(error); } } - finally + // hot path #2: there is nothing to Flush and buffering would not be beneficial + // it's allowed only for FileStream, as the public contract is that BufferedStream + // serializes ALL async operations + else if (_actLikeFileStream && _writePos == 0 && count >= _bufferSize) { - if (completeSynchronously) // if this is FALSE, we will be entering ReadFromUnderlyingStreamAsync and releasing there. - sem.Release(); + _readPos = _readLen = 0; + + // start the async operation + ValueTask result = _stream!.ReadAsync(new Memory(buffer, offset, count), cancellationToken); + + // release the lock (we are not using shared state anymore) + sem.Release(); + + return result.AsTask(); } } // Delegate to the async implementation. return ReadFromUnderlyingStreamAsync( new Memory(buffer, offset + bytesFromBuffer, count - bytesFromBuffer), - cancellationToken, bytesFromBuffer, semaphoreLockTask).AsTask(); + cancellationToken, bytesFromBuffer, semaphoreLockTask, locked).AsTask(); } public override ValueTask ReadAsync(Memory buffer, CancellationToken cancellationToken = default) @@ -660,30 +762,44 @@ public override ValueTask ReadAsync(Memory buffer, CancellationToken int bytesFromBuffer = 0; SemaphoreSlim sem = EnsureAsyncActiveSemaphoreInitialized(); Task semaphoreLockTask = sem.WaitAsync(cancellationToken); - if (semaphoreLockTask.IsCompletedSuccessfully) + bool locked = semaphoreLockTask.IsCompletedSuccessfully; + if (locked) { - bool completeSynchronously = true; - try + // hot path #1: there is data in the buffer + if (_readLen - _readPos > 0 || (buffer.Length == 0 && !_actLikeFileStream)) { bytesFromBuffer = ReadFromBuffer(buffer.Span); - completeSynchronously = bytesFromBuffer == buffer.Length; - if (completeSynchronously) + + if (bytesFromBuffer == buffer.Length) { + // if above is FALSE, we will be entering ReadFromUnderlyingStreamAsync and releasing there. + sem.Release(); + // If we satisfied enough data from the buffer, we can complete synchronously. return new ValueTask(bytesFromBuffer); } + + buffer = buffer.Slice(bytesFromBuffer); } - finally + // hot path #2: there is nothing to Flush and buffering would not be beneficial + // it's allowed only for FileStream, as the public contract is that BufferedStream + // serializes ALL async operations + else if (_actLikeFileStream && _writePos == 0 && buffer.Length >= _bufferSize) { - if (completeSynchronously) // if this is FALSE, we will be entering ReadFromUnderlyingStreamAsync and releasing there. - { - sem.Release(); - } + _readPos = _readLen = 0; + + // start the async operation + ValueTask result = _stream!.ReadAsync(buffer, cancellationToken); + + // release the lock (we are not using shared state anymore) + sem.Release(); + + return result; } } // Delegate to the async implementation. - return ReadFromUnderlyingStreamAsync(buffer.Slice(bytesFromBuffer), cancellationToken, bytesFromBuffer, semaphoreLockTask); + return ReadFromUnderlyingStreamAsync(buffer, cancellationToken, bytesFromBuffer, semaphoreLockTask, locked); } /// BufferedStream should be as thin a wrapper as possible. We want ReadAsync to delegate to @@ -691,7 +807,7 @@ public override ValueTask ReadAsync(Memory buffer, CancellationToken /// This allows BufferedStream to affect the semantics of the stream it wraps as little as possible. /// -2 if _bufferSize was set to 0 while waiting on the semaphore; otherwise num of bytes read. private async ValueTask ReadFromUnderlyingStreamAsync( - Memory buffer, CancellationToken cancellationToken, int bytesAlreadySatisfied, Task semaphoreLockTask) + Memory buffer, CancellationToken cancellationToken, int bytesAlreadySatisfied, Task semaphoreLockTask, bool locked) { // Same conditions validated with exceptions in ReadAsync: Debug.Assert(_stream != null); @@ -700,22 +816,32 @@ private async ValueTask ReadFromUnderlyingStreamAsync( Debug.Assert(_asyncActiveSemaphore != null); Debug.Assert(semaphoreLockTask != null); - // Employ async waiting based on the same synchronization used in BeginRead of the abstract Stream. - await semaphoreLockTask.ConfigureAwait(false); + if (!locked) + { + // Employ async waiting based on the same synchronization used in BeginRead of the abstract Stream. + await semaphoreLockTask.ConfigureAwait(false); + } + try { - // The buffer might have been changed by another async task while we were waiting on the semaphore. - // Check it now again. - int bytesFromBuffer = ReadFromBuffer(buffer.Span); - if (bytesFromBuffer == buffer.Length) - { - return bytesAlreadySatisfied + bytesFromBuffer; - } + int bytesFromBuffer = 0; - if (bytesFromBuffer > 0) + // we have already tried to read it from the buffer + if (!locked && (buffer.Length > 0 || !_actLikeFileStream)) { - buffer = buffer.Slice(bytesFromBuffer); - bytesAlreadySatisfied += bytesFromBuffer; + // The buffer might have been changed by another async task while we were waiting on the semaphore. + // Check it now again. + bytesFromBuffer = ReadFromBuffer(buffer.Span); + if (bytesFromBuffer == buffer.Length) + { + return bytesAlreadySatisfied + bytesFromBuffer; + } + + if (bytesFromBuffer > 0) + { + buffer = buffer.Slice(bytesFromBuffer); + bytesAlreadySatisfied += bytesFromBuffer; + } } Debug.Assert(_readLen == _readPos); @@ -773,7 +899,7 @@ private int ReadByteSlow() Debug.Assert(_stream != null); if (_writePos > 0) - FlushWrite(); + FlushWrite(true); EnsureBufferAllocated(); _readLen = _stream.Read(_buffer!, 0, _bufferSize); @@ -1035,7 +1161,8 @@ public override ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationTo // Try to satisfy the request from the buffer synchronously. SemaphoreSlim sem = EnsureAsyncActiveSemaphoreInitialized(); Task semaphoreLockTask = sem.WaitAsync(cancellationToken); - if (semaphoreLockTask.IsCompletedSuccessfully) + bool locked = semaphoreLockTask.IsCompletedSuccessfully; + if (locked) { bool completeSynchronously = true; try @@ -1047,7 +1174,7 @@ public override ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationTo Debug.Assert(_writePos < _bufferSize); - // If the write completely fits into the buffer, we can complete synchronously: + // hot path #1 If the write completely fits into the buffer, we can complete synchronously: completeSynchronously = buffer.Length < _bufferSize - _writePos; if (completeSynchronously) { @@ -1061,10 +1188,22 @@ public override ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationTo if (completeSynchronously) // if this is FALSE, we will be entering WriteToUnderlyingStreamAsync and releasing there. sem.Release(); } + + // hot path #2: there is nothing to Flush and buffering would not be beneficial + // it's allowed only for FileStream, as the public contract is that BufferedStream + // serializes ALL async operations + if (_actLikeFileStream && _writePos == 0 && buffer.Length >= _bufferSize) + { + ValueTask result = _stream!.WriteAsync(buffer, cancellationToken); + + sem.Release(); + + return result; + } } // Delegate to the async implementation. - return WriteToUnderlyingStreamAsync(buffer, cancellationToken, semaphoreLockTask); + return WriteToUnderlyingStreamAsync(buffer, cancellationToken, semaphoreLockTask, locked); } /// BufferedStream should be as thin a wrapper as possible. We want WriteAsync to delegate to @@ -1073,7 +1212,7 @@ public override ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationTo /// little as possible. /// private async ValueTask WriteToUnderlyingStreamAsync( - ReadOnlyMemory buffer, CancellationToken cancellationToken, Task semaphoreLockTask) + ReadOnlyMemory buffer, CancellationToken cancellationToken, Task semaphoreLockTask, bool locked) { Debug.Assert(_stream != null); Debug.Assert(_stream.CanWrite); @@ -1083,14 +1222,23 @@ private async ValueTask WriteToUnderlyingStreamAsync( // See the LARGE COMMENT in Write(..) for the explanation of the write buffer algorithm. - await semaphoreLockTask.ConfigureAwait(false); + if (!locked) + { + await semaphoreLockTask.ConfigureAwait(false); + } + try { - // The buffer might have been changed by another async task while we were waiting on the semaphore. - // However, note that if we recalculate the sync completion condition to TRUE, then useBuffer will also be TRUE. + if (!locked) + { + // The buffer might have been changed by another async task while we were waiting on the semaphore. + // However, note that if we recalculate the sync completion condition to TRUE, then useBuffer will also be TRUE. - if (_writePos == 0) - ClearReadBufferBeforeWrite(); + if (_writePos == 0) + { + ClearReadBufferBeforeWrite(); + } + } int totalUserBytes; bool useBuffer; @@ -1164,6 +1312,18 @@ public override void EndWrite(IAsyncResult asyncResult) => TaskToApm.End(asyncResult); public override void WriteByte(byte value) + { + if (_writePos > 0 && _writePos < _bufferSize - 1) + { + _buffer![_writePos++] = value; + } + else + { + WriteByteSlow(value); + } + } + + private void WriteByteSlow(byte value) { EnsureNotClosed(); @@ -1176,7 +1336,7 @@ public override void WriteByte(byte value) // We should not be flushing here, but only writing to the underlying stream, but previous version flushed, so we keep this. if (_writePos >= _bufferSize - 1) - FlushWrite(); + FlushWrite(true); _buffer![_writePos++] = value; @@ -1194,7 +1354,7 @@ public override long Seek(long offset, SeekOrigin origin) { // We should be only writing the buffer and not flushing, // but the previous version did flush and we stick to it for back-compat reasons. - FlushWrite(); + FlushWrite(true); return _stream.Seek(offset, origin); } @@ -1267,7 +1427,7 @@ public override void CopyTo(Stream destination, int bufferSize) else if (_writePos > 0) { // If there's write data in the buffer, flush it back to the underlying stream, as does ReadAsync. - FlushWrite(); + FlushWrite(true); } // Our buffer is now clear. Copy data directly from the source stream to the destination stream. 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 03620f33bf5552..6109cf014298c8 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs @@ -477,5 +477,21 @@ internal IAsyncResult BaseBeginWrite(byte[] buffer, int offset, int count, Async => base.BeginWrite(buffer, offset, count, callback, state); internal void BaseEndWrite(IAsyncResult asyncResult) => base.EndWrite(asyncResult); + + internal static bool IsIoRelatedException(Exception e) => + // These all derive from IOException + // DirectoryNotFoundException + // DriveNotFoundException + // EndOfStreamException + // FileLoadException + // FileNotFoundException + // PathTooLongException + // PipeException + e is IOException || + // Note that SecurityException is only thrown on runtimes that support CAS + // e is SecurityException || + e is UnauthorizedAccessException || + e is NotSupportedException || + (e is ArgumentException && !(e is ArgumentNullException)); } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs index 6eff2056257ed9..2541f5ec96d169 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs @@ -23,28 +23,36 @@ internal static class FileStreamHelpers internal static FileStreamStrategy ChooseStrategy(SafeFileHandle handle, FileAccess access, int bufferSize, bool isAsync) { - if (!UseLegacyStrategy && bufferSize == 1) + if (UseLegacyStrategy) { - return isAsync - ? new AsyncWindowsFileStreamStrategy(handle, access) - : new SyncWindowsFileStreamStrategy(handle, access); + return new LegacyFileStreamStrategy(handle, access, bufferSize, isAsync); } - return new LegacyFileStreamStrategy(handle, access, bufferSize, isAsync); + WindowsFileStreamStrategy strategy = isAsync + ? new AsyncWindowsFileStreamStrategy(handle, access) + : new SyncWindowsFileStreamStrategy(handle, access); + + return EnableBufferingIfNeeded(strategy, bufferSize); } internal static FileStreamStrategy ChooseStrategy(string path, FileMode mode, FileAccess access, FileShare share, int bufferSize, FileOptions options) { - if (!UseLegacyStrategy && bufferSize == 1) + if (UseLegacyStrategy) { - return (options & FileOptions.Asynchronous) != 0 - ? new AsyncWindowsFileStreamStrategy(path, mode, access, share, options) - : new SyncWindowsFileStreamStrategy(path, mode, access, share, options); + return new LegacyFileStreamStrategy(path, mode, access, share, bufferSize, options); } - return new LegacyFileStreamStrategy(path, mode, access, share, bufferSize, options); + WindowsFileStreamStrategy strategy = (options & FileOptions.Asynchronous) != 0 + ? new AsyncWindowsFileStreamStrategy(path, mode, access, share, options) + : new SyncWindowsFileStreamStrategy(path, mode, access, share, options); + + return EnableBufferingIfNeeded(strategy, bufferSize); } + // TODO: we might want to consider strategy.IsPipe here and never enable buffering for async pipes + internal static FileStreamStrategy EnableBufferingIfNeeded(WindowsFileStreamStrategy strategy, int bufferSize) + => bufferSize == 1 ? strategy : new BufferedFileStreamStrategy(strategy, bufferSize); + internal static SafeFileHandle OpenHandle(string path, FileMode mode, FileAccess access, FileShare share, FileOptions options) => CreateFileOpenHandle(path, mode, access, share, options); diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/LegacyFileStreamStrategy.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/LegacyFileStreamStrategy.Unix.cs index 550b2acab25051..c4cbd842c1ee7a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/LegacyFileStreamStrategy.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/LegacyFileStreamStrategy.Unix.cs @@ -172,7 +172,7 @@ protected override void Dispose(bool disposing) { FlushWriteBuffer(); } - catch (Exception e) when (IsIoRelatedException(e) && !disposing) + catch (Exception e) when (!disposing && FileStream.IsIoRelatedException(e)) { // On finalization, ignore failures from trying to flush the write buffer, // e.g. if this stream is wrapping a pipe and the pipe is now broken. 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 3849f620ebfc30..0e236cfa5097ff 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 @@ -192,7 +192,7 @@ protected override void Dispose(bool disposing) { FlushWriteBuffer(!disposing); } - catch (Exception e) when (IsIoRelatedException(e) && !disposing) + catch (Exception e) when (!disposing && FileStream.IsIoRelatedException(e)) { // On finalization, ignore failures from trying to flush the write buffer, // e.g. if this stream is wrapping a pipe and the pipe is now broken. diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/LegacyFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/LegacyFileStreamStrategy.cs index ea7b982da30b34..e11d395d936b9d 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/LegacyFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/LegacyFileStreamStrategy.cs @@ -377,22 +377,6 @@ public override long Position internal override bool IsClosed => _fileHandle.IsClosed; - private static bool IsIoRelatedException(Exception e) => - // These all derive from IOException - // DirectoryNotFoundException - // DriveNotFoundException - // EndOfStreamException - // FileLoadException - // FileNotFoundException - // PathTooLongException - // PipeException - e is IOException || - // Note that SecurityException is only thrown on runtimes that support CAS - // e is SecurityException || - e is UnauthorizedAccessException || - e is NotSupportedException || - (e is ArgumentException && !(e is ArgumentNullException)); - /// /// Gets the array used for buffering reading and writing. /// If the array hasn't been allocated, this will lazily allocate it. From 9e9cb1f194a364f172dae945914eeec92b729967 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 1 Mar 2021 17:47:22 +0100 Subject: [PATCH 10/20] fix the Mono build --- .../src/System/Threading/ClrThreadPoolPreAllocatedOverlapped.cs | 2 ++ .../src/System/IO/FileStreamCompletionSource.Windows.cs | 2 +- .../src/System/Threading/PreAllocatedOverlapped.cs | 1 + 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Threading/ClrThreadPoolPreAllocatedOverlapped.cs b/src/coreclr/System.Private.CoreLib/src/System/Threading/ClrThreadPoolPreAllocatedOverlapped.cs index 06df433df46603..d856d1d9231b3f 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Threading/ClrThreadPoolPreAllocatedOverlapped.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Threading/ClrThreadPoolPreAllocatedOverlapped.cs @@ -95,5 +95,7 @@ unsafe void IDeferredDisposable.OnFinalRelease(bool disposed) } } } + + internal bool IsUserObject(byte[]? buffer) => _overlapped.IsUserObject(buffer); } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamCompletionSource.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamCompletionSource.Windows.cs index da3075211567db..edee6fce1315d2 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamCompletionSource.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamCompletionSource.Windows.cs @@ -237,7 +237,7 @@ public static FileStreamCompletionSource Create(IFileStreamCompletionSourceStrat // MemoryFileStreamCompletionSource, which Retains the memory, which will result in less pinning in the case // where the underlying memory is backed by pre-pinned buffers. return preallocatedOverlapped != null && MemoryMarshal.TryGetArray(memory, out ArraySegment buffer) - && preallocatedOverlapped._overlapped.IsUserObject(buffer.Array) // preallocatedOverlapped is allocated when BufferedStream|LegacyFileStreamStrategy allocates the buffer + && preallocatedOverlapped.IsUserObject(buffer.Array) // preallocatedOverlapped is allocated when BufferedStream|LegacyFileStreamStrategy allocates the buffer ? new FileStreamCompletionSource(strategy, preallocatedOverlapped, numBufferedBytesRead, buffer.Array) : new MemoryFileStreamCompletionSource(strategy, numBufferedBytesRead, memory); } diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/PreAllocatedOverlapped.cs b/src/mono/System.Private.CoreLib/src/System/Threading/PreAllocatedOverlapped.cs index 2bd19d2e9a2e6f..d4a0daeff29c78 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/PreAllocatedOverlapped.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/PreAllocatedOverlapped.cs @@ -8,5 +8,6 @@ public sealed class PreAllocatedOverlapped : System.IDisposable [System.CLSCompliantAttribute(false)] public PreAllocatedOverlapped(System.Threading.IOCompletionCallback callback, object? state, object? pinData) { } public void Dispose() { } + internal bool IsUserObject(byte[]? buffer) => false; } } From 53752e7f30289884a521e308644618298f6111f9 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 2 Mar 2021 11:54:25 +0100 Subject: [PATCH 11/20] use the Legacy strategy by default for now, add tests for the other implementation --- .../TestUtilities/System/PlatformDetection.cs | 12 ++++-- .../System.IO.FileSystem.sln | 7 ++++ .../tests/FileStream/WriteAsync.cs | 2 +- .../tests/LegacyTests/LegacySwitchTests.cs | 29 +++++++++++++ .../System.IO.FileSystem.Legacy.Tests.csproj | 42 +++++++++++++++++++ .../LegacyTests/runtimeconfig.template.json | 5 +++ src/libraries/System.IO/System.IO.sln | 7 ++++ .../LegacyTests/System.IO.Legacy.Tests.csproj | 24 +++++++++++ .../LegacyTests/runtimeconfig.template.json | 5 +++ .../src/System/AppContextConfigHelper.cs | 14 +++++++ .../GlobalizationMode.Windows.cs | 2 +- .../System/Globalization/GlobalizationMode.cs | 2 +- .../System/IO/FileStreamHelpers.Windows.cs | 4 +- 13 files changed, 147 insertions(+), 8 deletions(-) create mode 100644 src/libraries/System.IO.FileSystem/tests/LegacyTests/LegacySwitchTests.cs create mode 100644 src/libraries/System.IO.FileSystem/tests/LegacyTests/System.IO.FileSystem.Legacy.Tests.csproj create mode 100644 src/libraries/System.IO.FileSystem/tests/LegacyTests/runtimeconfig.template.json create mode 100644 src/libraries/System.IO/tests/LegacyTests/System.IO.Legacy.Tests.csproj create mode 100644 src/libraries/System.IO/tests/LegacyTests/runtimeconfig.template.json diff --git a/src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs b/src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs index c41a3df0cd5acb..32801918ebfe21 100644 --- a/src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs +++ b/src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs @@ -187,14 +187,14 @@ public static string GetDistroVersionString() } } - private static readonly Lazy m_isInvariant = new Lazy(GetIsInvariantGlobalization); + private static readonly Lazy m_isInvariant = new Lazy(() => GetStaticNonPublicBooleanPropertyValue("System.Globalization.GlobalizationMode", "Invariant")); - private static bool GetIsInvariantGlobalization() + private static bool GetStaticNonPublicBooleanPropertyValue(string typeName, string propertyName) { - Type globalizationMode = Type.GetType("System.Globalization.GlobalizationMode"); + Type globalizationMode = Type.GetType(typeName); if (globalizationMode != null) { - MethodInfo methodInfo = globalizationMode.GetProperty("Invariant", BindingFlags.NonPublic | BindingFlags.Static)?.GetMethod; + MethodInfo methodInfo = globalizationMode.GetProperty(propertyName, BindingFlags.NonPublic | BindingFlags.Static)?.GetMethod; if (methodInfo != null) { return (bool)methodInfo.Invoke(null, null); @@ -235,6 +235,10 @@ private static Version GetICUVersion() version & 0xFF); } + private static readonly Lazy _legacyFileStream = new Lazy(() => GetStaticNonPublicBooleanPropertyValue("System.IO.FileStreamHelpers", "UseLegacyStrategy")); + + public static bool IsLegacyFileStreamEnabled => _legacyFileStream.Value; + private static bool GetIsInContainer() { if (IsWindows) diff --git a/src/libraries/System.IO.FileSystem/System.IO.FileSystem.sln b/src/libraries/System.IO.FileSystem/System.IO.FileSystem.sln index 4c3dc58180ee89..91fcdfa5375e5f 100644 --- a/src/libraries/System.IO.FileSystem/System.IO.FileSystem.sln +++ b/src/libraries/System.IO.FileSystem/System.IO.FileSystem.sln @@ -25,6 +25,8 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "ref", "ref", "{32A31E04-255 EndProject Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "src", "src", "{D9FB1730-B750-4C0D-8D24-8C992DEB6034}" EndProject +Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "System.IO.FileSystem.Legacy.Tests", "tests\LegacyTests\System.IO.FileSystem.Legacy.Tests.csproj", "{48E07F12-8597-40DE-8A37-CCBEB9D54012}" +EndProject Global GlobalSection(NestedProjects) = preSolution {D350D6E7-52F1-40A4-B646-C178F6BBB689} = {1A727AF9-4F39-4109-BB8F-813286031DC9} @@ -37,6 +39,7 @@ Global {06DD5AA8-FDA6-495B-A8D1-8CE83C78DE6C} = {32A31E04-2554-4223-BED8-45757408B4F6} {877E39A8-51CB-463A-AF4C-6FAE4F438075} = {D9FB1730-B750-4C0D-8D24-8C992DEB6034} {D7DF8034-3AE5-4DEF-BCC4-6353239391BF} = {D9FB1730-B750-4C0D-8D24-8C992DEB6034} + {48E07F12-8597-40DE-8A37-CCBEB9D54012} = {1A727AF9-4F39-4109-BB8F-813286031DC9} EndGlobalSection GlobalSection(SolutionConfigurationPlatforms) = preSolution Debug|Any CPU = Debug|Any CPU @@ -83,6 +86,10 @@ Global {06DD5AA8-FDA6-495B-A8D1-8CE83C78DE6C}.Debug|Any CPU.Build.0 = Debug|Any CPU {06DD5AA8-FDA6-495B-A8D1-8CE83C78DE6C}.Release|Any CPU.ActiveCfg = Release|Any CPU {06DD5AA8-FDA6-495B-A8D1-8CE83C78DE6C}.Release|Any CPU.Build.0 = Release|Any CPU + {48E07F12-8597-40DE-8A37-CCBEB9D54012}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {48E07F12-8597-40DE-8A37-CCBEB9D54012}.Debug|Any CPU.Build.0 = Debug|Any CPU + {48E07F12-8597-40DE-8A37-CCBEB9D54012}.Release|Any CPU.ActiveCfg = Release|Any CPU + {48E07F12-8597-40DE-8A37-CCBEB9D54012}.Release|Any CPU.Build.0 = Release|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE diff --git a/src/libraries/System.IO.FileSystem/tests/FileStream/WriteAsync.cs b/src/libraries/System.IO.FileSystem/tests/FileStream/WriteAsync.cs index 6ed4d1db2c4cc6..358cf11310838f 100644 --- a/src/libraries/System.IO.FileSystem/tests/FileStream/WriteAsync.cs +++ b/src/libraries/System.IO.FileSystem/tests/FileStream/WriteAsync.cs @@ -220,7 +220,7 @@ public async Task ManyConcurrentWriteAsyncs_OuterLoop( { writes[i] = WriteAsync(fs, expectedData, i * writeSize, writeSize, cancellationToken); Assert.Null(writes[i].Exception); - if (useAsync) + if (useAsync && PlatformDetection.IsLegacyFileStreamEnabled) { Assert.Equal((i + 1) * writeSize, fs.Position); } diff --git a/src/libraries/System.IO.FileSystem/tests/LegacyTests/LegacySwitchTests.cs b/src/libraries/System.IO.FileSystem/tests/LegacyTests/LegacySwitchTests.cs new file mode 100644 index 00000000000000..55d162306989ca --- /dev/null +++ b/src/libraries/System.IO.FileSystem/tests/LegacyTests/LegacySwitchTests.cs @@ -0,0 +1,29 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Reflection; +using Xunit; + +namespace System.IO.Tests +{ + public class LegacySwitchTests + { + [Fact] + public static void LegacySwitchIsHonored() + { + string filePath = Path.Combine(Path.GetTempPath(), Path.GetTempFileName()); + + using (FileStream fileStream = File.Create(filePath)) + { + Stream strategy = fileStream + .GetType() + .GetField("_strategy", BindingFlags.NonPublic | BindingFlags.Instance) + .GetValue(fileStream) as Stream; + + Assert.DoesNotContain(strategy.GetType().FullName, "Legacy"); + } + + File.Delete(filePath); + } + } +} diff --git a/src/libraries/System.IO.FileSystem/tests/LegacyTests/System.IO.FileSystem.Legacy.Tests.csproj b/src/libraries/System.IO.FileSystem/tests/LegacyTests/System.IO.FileSystem.Legacy.Tests.csproj new file mode 100644 index 00000000000000..71efe7c433e43f --- /dev/null +++ b/src/libraries/System.IO.FileSystem/tests/LegacyTests/System.IO.FileSystem.Legacy.Tests.csproj @@ -0,0 +1,42 @@ + + + true + true + + $(NetCoreAppCurrent)-windows + + --working-dir=/test-dir + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/src/libraries/System.IO.FileSystem/tests/LegacyTests/runtimeconfig.template.json b/src/libraries/System.IO.FileSystem/tests/LegacyTests/runtimeconfig.template.json new file mode 100644 index 00000000000000..010891ed8fc6ca --- /dev/null +++ b/src/libraries/System.IO.FileSystem/tests/LegacyTests/runtimeconfig.template.json @@ -0,0 +1,5 @@ +{ + "configProperties": { + "System.IO.UseLegacyFileStream": true + } +} diff --git a/src/libraries/System.IO/System.IO.sln b/src/libraries/System.IO/System.IO.sln index fa248eb5af8221..7d8ebc2c3da28e 100644 --- a/src/libraries/System.IO/System.IO.sln +++ b/src/libraries/System.IO/System.IO.sln @@ -23,6 +23,8 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "ref", "ref", "{9FDAA57A-696 EndProject Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "src", "src", "{D9FD8082-D04C-4DA8-9F4C-261D1C65A6D3}" EndProject +Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "System.IO.Legacy.Tests", "tests\LegacyTests\System.IO.Legacy.Tests.csproj", "{0217540D-FA86-41B3-9754-7BB5096ABA3E}" +EndProject Global GlobalSection(NestedProjects) = preSolution {D11D3624-1322-45D1-A604-7E68CDB85BE8} = {5AD2C433-C661-4AD1-BD9F-D164ADC43512} @@ -34,6 +36,7 @@ Global {D0D1CDAC-16F8-4382-A219-74A513CC1790} = {9FDAA57A-696B-4CB1-99AE-BCDF91848B75} {0769544B-1A5D-4D74-94FD-899DF6C39D62} = {D9FD8082-D04C-4DA8-9F4C-261D1C65A6D3} {AA5E80B2-A0AA-46F1-B319-5B528BAC382B} = {D9FD8082-D04C-4DA8-9F4C-261D1C65A6D3} + {0217540D-FA86-41B3-9754-7BB5096ABA3E} = {5AD2C433-C661-4AD1-BD9F-D164ADC43512} EndGlobalSection GlobalSection(SolutionConfigurationPlatforms) = preSolution Debug|Any CPU = Debug|Any CPU @@ -76,6 +79,10 @@ Global {D0D1CDAC-16F8-4382-A219-74A513CC1790}.Debug|Any CPU.Build.0 = Debug|Any CPU {D0D1CDAC-16F8-4382-A219-74A513CC1790}.Release|Any CPU.ActiveCfg = Release|Any CPU {D0D1CDAC-16F8-4382-A219-74A513CC1790}.Release|Any CPU.Build.0 = Release|Any CPU + {0217540D-FA86-41B3-9754-7BB5096ABA3E}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {0217540D-FA86-41B3-9754-7BB5096ABA3E}.Debug|Any CPU.Build.0 = Debug|Any CPU + {0217540D-FA86-41B3-9754-7BB5096ABA3E}.Release|Any CPU.ActiveCfg = Release|Any CPU + {0217540D-FA86-41B3-9754-7BB5096ABA3E}.Release|Any CPU.Build.0 = Release|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE diff --git a/src/libraries/System.IO/tests/LegacyTests/System.IO.Legacy.Tests.csproj b/src/libraries/System.IO/tests/LegacyTests/System.IO.Legacy.Tests.csproj new file mode 100644 index 00000000000000..370bf05d6fa5dc --- /dev/null +++ b/src/libraries/System.IO/tests/LegacyTests/System.IO.Legacy.Tests.csproj @@ -0,0 +1,24 @@ + + + System.IO + true + true + true + + $(NetCoreAppCurrent)-windows + + + + + + + + + + + + + + + + diff --git a/src/libraries/System.IO/tests/LegacyTests/runtimeconfig.template.json b/src/libraries/System.IO/tests/LegacyTests/runtimeconfig.template.json new file mode 100644 index 00000000000000..010891ed8fc6ca --- /dev/null +++ b/src/libraries/System.IO/tests/LegacyTests/runtimeconfig.template.json @@ -0,0 +1,5 @@ +{ + "configProperties": { + "System.IO.UseLegacyFileStream": true + } +} diff --git a/src/libraries/System.Private.CoreLib/src/System/AppContextConfigHelper.cs b/src/libraries/System.Private.CoreLib/src/System/AppContextConfigHelper.cs index 9175a3e8ba9129..331ef281bb32a6 100644 --- a/src/libraries/System.Private.CoreLib/src/System/AppContextConfigHelper.cs +++ b/src/libraries/System.Private.CoreLib/src/System/AppContextConfigHelper.cs @@ -10,6 +10,20 @@ internal static class AppContextConfigHelper internal static bool GetBooleanConfig(string configName, bool defaultValue) => AppContext.TryGetSwitch(configName, out bool value) ? value : defaultValue; + internal static bool GetBooleanConfig(string switchName, string envVariable) + { + if (!AppContext.TryGetSwitch(switchName, out bool ret)) + { + string? switchValue = Environment.GetEnvironmentVariable(envVariable); + if (switchValue != null) + { + ret = bool.IsTrueStringIgnoreCase(switchValue) || switchValue.Equals("1"); + } + } + + return ret; + } + internal static int GetInt32Config(string configName, int defaultValue, bool allowNegative = true) { try diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Windows.cs index 1a0bf66c42e566..590aae9b3bdc71 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Windows.cs @@ -10,7 +10,7 @@ internal static partial class GlobalizationMode internal static bool Invariant { get; } = GetInvariantSwitchValue(); internal static bool UseNls { get; } = !Invariant && - (GetSwitchValue("System.Globalization.UseNls", "DOTNET_SYSTEM_GLOBALIZATION_USENLS") || + (AppContextConfigHelper.GetBooleanConfig("System.Globalization.UseNls", "DOTNET_SYSTEM_GLOBALIZATION_USENLS") || !LoadIcu()); private static bool LoadIcu() diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs index 7183786cf0d225..3032a81eaa74ef 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs @@ -9,7 +9,7 @@ namespace System.Globalization internal static partial class GlobalizationMode { private static bool GetInvariantSwitchValue() => - GetSwitchValue("System.Globalization.Invariant", "DOTNET_SYSTEM_GLOBALIZATION_INVARIANT"); + AppContextConfigHelper.GetBooleanConfig("System.Globalization.Invariant", "DOTNET_SYSTEM_GLOBALIZATION_INVARIANT"); private static bool TryGetAppLocalIcuSwitchValue([NotNullWhen(true)] out string? value) => TryGetStringValue("System.Globalization.AppLocalIcu", "DOTNET_SYSTEM_GLOBALIZATION_APPLOCALICU", out value); diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs index 2541f5ec96d169..840d62cb03b80e 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs @@ -19,7 +19,9 @@ internal static class FileStreamHelpers private const int ERROR_HANDLE_EOF = 38; private const int ERROR_IO_PENDING = 997; - private static readonly bool UseLegacyStrategy = Environment.GetEnvironmentVariable("DOTNET_LEGACY_FILE_IO") == "1"; + // It's enabled by default. We are going to change that (by removing !) once we fix #16354, #25905 and #24847. + internal static bool UseLegacyStrategy { get; } + = !AppContextConfigHelper.GetBooleanConfig("System.IO.UseLegacyFileStream", "DOTNET_SYSTEM_IO_USELEGACYFILESTREAM"); internal static FileStreamStrategy ChooseStrategy(SafeFileHandle handle, FileAccess access, int bufferSize, bool isAsync) { From e2eec8b84324938ec07afd8cb13a44ce9bccfe95 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 2 Mar 2021 13:08:47 +0100 Subject: [PATCH 12/20] restore old file name to make it easier to review the code (as diff within the file, not a removal and addition) --- .../src/System.Private.CoreLib.Shared.projitems | 2 +- ...ionSource.Windows.cs => FileStreamCompletionSource.Win32.cs} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename src/libraries/System.Private.CoreLib/src/System/IO/{FileStreamCompletionSource.Windows.cs => FileStreamCompletionSource.Win32.cs} (100%) diff --git a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems index 635da72ed67a2b..b4664a9ff50df1 100644 --- a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems +++ b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems @@ -1637,7 +1637,7 @@ - + diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamCompletionSource.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamCompletionSource.Win32.cs similarity index 100% rename from src/libraries/System.Private.CoreLib/src/System/IO/FileStreamCompletionSource.Windows.cs rename to src/libraries/System.Private.CoreLib/src/System/IO/FileStreamCompletionSource.Win32.cs From 20640fa29ba4daa3b22db1eac5c081288ba5d3d0 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 2 Mar 2021 15:09:13 +0100 Subject: [PATCH 13/20] Don't set the buffer to null, to avoid a NullReferenceException when users have a race condition in their code (i.e. they call Close when calling another method on Stream like Read). --- .../src/System/IO/BufferedStream.cs | 24 ++++++++++++++++--- .../System/IO/WindowsFileStreamStrategy.cs | 10 ++------ 2 files changed, 23 insertions(+), 11 deletions(-) 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 926a1a2f326ebe..401eb8ce2fc3ed 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/BufferedStream.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/BufferedStream.cs @@ -147,13 +147,17 @@ private void EnsureShadowBufferAllocated() _buffer = shadowBuffer; } - [MethodImpl(MethodImplOptions.AggressiveInlining)] private void EnsureBufferAllocated() { Debug.Assert(_bufferSize > 0); // BufferedStream is not intended for multi-threaded use, so no worries about the get/set race on _buffer. if (_buffer == null) + { + AllocateBuffer(); + } + + void AllocateBuffer() // logic kept in a separate method to get the method inlined { _buffer = new byte[_bufferSize]; @@ -294,7 +298,14 @@ protected override void Dispose(bool disposing) finally { _stream = null; - _buffer = null; + + // Don't set the buffer to null, to avoid a NullReferenceException + // when users have a race condition in their code (i.e. they call + // Close when calling another method on Stream like Read). + if (!_actLikeFileStream) + { + _buffer = null; + } // Call base.Dispose(bool) to cleanup async IO resources base.Dispose(disposing); @@ -320,7 +331,14 @@ public override async ValueTask DisposeAsync() finally { _stream = null; - _buffer = null; + + // Don't set the buffer to null, to avoid a NullReferenceException + // when users have a race condition in their code (i.e. they call + // Close when calling another method on Stream like Read). + if (!_actLikeFileStream) + { + _buffer = null; + } } } 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 a3100794ca3d8d..e6f8116ddddb22 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/WindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/WindowsFileStreamStrategy.cs @@ -130,8 +130,7 @@ public override void WriteByte(byte value) Write(buffer); } - // this method just disposes everything as there is no buffer here - // and we don't really need to Flush anything here + // this method just disposes everything (no buffer, no need to flush) public override ValueTask DisposeAsync() { if (_fileHandle != null && !_fileHandle.IsClosed) @@ -143,10 +142,9 @@ public override ValueTask DisposeAsync() return ValueTask.CompletedTask; } - // this method in the future will be called in no-buffering scenarios internal sealed override void DisposeInternal(bool disposing) => Dispose(disposing); - // this method is called from BufferedStream.Dispose so the content is already flushed + // this method just disposes everything (no buffer, no need to flush) protected override void Dispose(bool disposing) { if (_fileHandle != null && !_fileHandle.IsClosed) @@ -154,10 +152,6 @@ protected override void Dispose(bool disposing) _fileHandle.ThreadPoolBinding?.Dispose(); _fileHandle.Dispose(); } - - // Don't set the buffer to null, to avoid a NullReferenceException - // when users have a race condition in their code (i.e. they call - // Close when calling another method on Stream like Read). } public sealed override void Flush() => Flush(flushToDisk: false); // we have nothing to flush as there is no buffer here From dd7e9ba64883bf7e0456479185a9813992ff5621 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 2 Mar 2021 15:11:58 +0100 Subject: [PATCH 14/20] fix the browser build? --- .../src/System.Private.CoreLib.Shared.projitems | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems index b4664a9ff50df1..103d749d5f3e46 100644 --- a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems +++ b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems @@ -1894,6 +1894,7 @@ + From 2a35ae6735d493cba510f6b87d2e83cb08b1748d Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 2 Mar 2021 20:20:06 +0100 Subject: [PATCH 15/20] reverting the flushing changes as it looks that they have introduced a bug --- .../src/System/IO/BufferedStream.cs | 26 +++---------------- 1 file changed, 4 insertions(+), 22 deletions(-) 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 401eb8ce2fc3ed..3260e1e4d79095 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/BufferedStream.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/BufferedStream.cs @@ -398,32 +398,14 @@ public override Task FlushAsync(CancellationToken cancellationToken) EnsureNotClosed(); - // try to get the lock and exit in synchronous way if there is nothing to Flush - SemaphoreSlim sem = EnsureAsyncActiveSemaphoreInitialized(); - Task semaphoreLockTask = sem.WaitAsync(cancellationToken); - bool lockAcquired = semaphoreLockTask.IsCompletedSuccessfully; - if (lockAcquired) - { - if (_writePos == 0 && _readPos == _readLen) - { - sem.Release(); - - return CanWrite ? _stream!.FlushAsync(cancellationToken) : Task.CompletedTask; - } - } - - return FlushAsyncInternal(semaphoreLockTask, lockAcquired, cancellationToken); + return FlushAsyncInternal(cancellationToken); } - private async Task FlushAsyncInternal(Task semaphoreLockTask, bool lockAcquired, CancellationToken cancellationToken) + private async Task FlushAsyncInternal(CancellationToken cancellationToken) { Debug.Assert(_stream != null); - if (!lockAcquired) - { - await semaphoreLockTask.ConfigureAwait(false); - } - + await EnsureAsyncActiveSemaphoreInitialized().WaitAsync(cancellationToken).ConfigureAwait(false); try { if (_writePos > 0) @@ -464,7 +446,7 @@ private async Task FlushAsyncInternal(Task semaphoreLockTask, bool lockAcquired, } finally { - _asyncActiveSemaphore!.Release(); + _asyncActiveSemaphore.Release(); } } From b9afbffa6ddb538cab0d14bc72e694b8c3afadc7 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Wed, 3 Mar 2021 12:21:58 +0100 Subject: [PATCH 16/20] simplify the source code by removing some of the optimizations that are not providing too much gains --- .../src/System/IO/BufferedStream.cs | 93 ++++++++----------- 1 file changed, 41 insertions(+), 52 deletions(-) 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 3260e1e4d79095..f5469d93ce1a57 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/BufferedStream.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/BufferedStream.cs @@ -468,7 +468,6 @@ private void FlushRead() /// /// Called by Write methods to clear the Read Buffer /// - [MethodImpl(MethodImplOptions.AggressiveInlining)] private void ClearReadBufferBeforeWrite() { Debug.Assert(_stream != null); @@ -525,7 +524,6 @@ private async ValueTask FlushWriteAsync(CancellationToken cancellationToken) await _stream.FlushAsync(cancellationToken).ConfigureAwait(false); } - [MethodImpl(MethodImplOptions.AggressiveInlining)] private int ReadFromBuffer(byte[] buffer, int offset, int count) { int readbytes = _readLen - _readPos; @@ -701,8 +699,7 @@ public override Task ReadAsync(byte[] buffer, int offset, int count, Cancel // an Async operation. SemaphoreSlim sem = EnsureAsyncActiveSemaphoreInitialized(); Task semaphoreLockTask = sem.WaitAsync(cancellationToken); - bool locked = semaphoreLockTask.IsCompletedSuccessfully; - if (locked) + if (semaphoreLockTask.IsCompletedSuccessfully) { // hot path #1: there is data in the buffer if (_readLen - _readPos > 0 || (count == 0 && !_actLikeFileStream)) @@ -733,20 +730,21 @@ public override Task ReadAsync(byte[] buffer, int offset, int count, Cancel { _readPos = _readLen = 0; - // start the async operation - ValueTask result = _stream!.ReadAsync(new Memory(buffer, offset, count), cancellationToken); - - // release the lock (we are not using shared state anymore) - sem.Release(); - - return result.AsTask(); + try + { + return _stream!.ReadAsync(new Memory(buffer, offset, count), cancellationToken).AsTask(); + } + finally + { + sem.Release(); + } } } // Delegate to the async implementation. return ReadFromUnderlyingStreamAsync( new Memory(buffer, offset + bytesFromBuffer, count - bytesFromBuffer), - cancellationToken, bytesFromBuffer, semaphoreLockTask, locked).AsTask(); + cancellationToken, bytesFromBuffer, semaphoreLockTask).AsTask(); } public override ValueTask ReadAsync(Memory buffer, CancellationToken cancellationToken = default) @@ -762,8 +760,7 @@ public override ValueTask ReadAsync(Memory buffer, CancellationToken int bytesFromBuffer = 0; SemaphoreSlim sem = EnsureAsyncActiveSemaphoreInitialized(); Task semaphoreLockTask = sem.WaitAsync(cancellationToken); - bool locked = semaphoreLockTask.IsCompletedSuccessfully; - if (locked) + if (semaphoreLockTask.IsCompletedSuccessfully) { // hot path #1: there is data in the buffer if (_readLen - _readPos > 0 || (buffer.Length == 0 && !_actLikeFileStream)) @@ -788,18 +785,19 @@ public override ValueTask ReadAsync(Memory buffer, CancellationToken { _readPos = _readLen = 0; - // start the async operation - ValueTask result = _stream!.ReadAsync(buffer, cancellationToken); - - // release the lock (we are not using shared state anymore) - sem.Release(); - - return result; + try + { + return _stream!.ReadAsync(buffer, cancellationToken); + } + finally + { + sem.Release(); + } } } // Delegate to the async implementation. - return ReadFromUnderlyingStreamAsync(buffer, cancellationToken, bytesFromBuffer, semaphoreLockTask, locked); + return ReadFromUnderlyingStreamAsync(buffer, cancellationToken, bytesFromBuffer, semaphoreLockTask); } /// BufferedStream should be as thin a wrapper as possible. We want ReadAsync to delegate to @@ -807,7 +805,7 @@ public override ValueTask ReadAsync(Memory buffer, CancellationToken /// This allows BufferedStream to affect the semantics of the stream it wraps as little as possible. /// -2 if _bufferSize was set to 0 while waiting on the semaphore; otherwise num of bytes read. private async ValueTask ReadFromUnderlyingStreamAsync( - Memory buffer, CancellationToken cancellationToken, int bytesAlreadySatisfied, Task semaphoreLockTask, bool locked) + Memory buffer, CancellationToken cancellationToken, int bytesAlreadySatisfied, Task semaphoreLockTask) { // Same conditions validated with exceptions in ReadAsync: Debug.Assert(_stream != null); @@ -816,18 +814,14 @@ private async ValueTask ReadFromUnderlyingStreamAsync( Debug.Assert(_asyncActiveSemaphore != null); Debug.Assert(semaphoreLockTask != null); - if (!locked) - { - // Employ async waiting based on the same synchronization used in BeginRead of the abstract Stream. - await semaphoreLockTask.ConfigureAwait(false); - } + // Employ async waiting based on the same synchronization used in BeginRead of the abstract Stream. + await semaphoreLockTask.ConfigureAwait(false); try { int bytesFromBuffer = 0; - // we have already tried to read it from the buffer - if (!locked && (buffer.Length > 0 || !_actLikeFileStream)) + if (!(buffer.Length == 0 && _actLikeFileStream && _readLen == _readPos)) { // The buffer might have been changed by another async task while we were waiting on the semaphore. // Check it now again. @@ -1161,8 +1155,7 @@ public override ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationTo // Try to satisfy the request from the buffer synchronously. SemaphoreSlim sem = EnsureAsyncActiveSemaphoreInitialized(); Task semaphoreLockTask = sem.WaitAsync(cancellationToken); - bool locked = semaphoreLockTask.IsCompletedSuccessfully; - if (locked) + if (semaphoreLockTask.IsCompletedSuccessfully) { bool completeSynchronously = true; try @@ -1194,16 +1187,19 @@ public override ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationTo // serializes ALL async operations if (_actLikeFileStream && _writePos == 0 && buffer.Length >= _bufferSize) { - ValueTask result = _stream!.WriteAsync(buffer, cancellationToken); - - sem.Release(); - - return result; + try + { + return _stream!.WriteAsync(buffer, cancellationToken); + } + finally + { + sem.Release(); + } } } // Delegate to the async implementation. - return WriteToUnderlyingStreamAsync(buffer, cancellationToken, semaphoreLockTask, locked); + return WriteToUnderlyingStreamAsync(buffer, cancellationToken, semaphoreLockTask); } /// BufferedStream should be as thin a wrapper as possible. We want WriteAsync to delegate to @@ -1212,7 +1208,7 @@ public override ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationTo /// little as possible. /// private async ValueTask WriteToUnderlyingStreamAsync( - ReadOnlyMemory buffer, CancellationToken cancellationToken, Task semaphoreLockTask, bool locked) + ReadOnlyMemory buffer, CancellationToken cancellationToken, Task semaphoreLockTask) { Debug.Assert(_stream != null); Debug.Assert(_stream.CanWrite); @@ -1222,22 +1218,15 @@ private async ValueTask WriteToUnderlyingStreamAsync( // See the LARGE COMMENT in Write(..) for the explanation of the write buffer algorithm. - if (!locked) - { - await semaphoreLockTask.ConfigureAwait(false); - } - + await semaphoreLockTask.ConfigureAwait(false); try { - if (!locked) - { - // The buffer might have been changed by another async task while we were waiting on the semaphore. - // However, note that if we recalculate the sync completion condition to TRUE, then useBuffer will also be TRUE. + // The buffer might have been changed by another async task while we were waiting on the semaphore. + // However, note that if we recalculate the sync completion condition to TRUE, then useBuffer will also be TRUE. - if (_writePos == 0) - { - ClearReadBufferBeforeWrite(); - } + if (_writePos == 0) + { + ClearReadBufferBeforeWrite(); } int totalUserBytes; From 202931757be2bf61f530046ae8e5e667f0e2c0cb Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 4 Mar 2021 09:19:30 +0100 Subject: [PATCH 17/20] don't verify OS handle position --- .../tests/FileStream/SafeFileHandle.cs | 4 +- .../IO/AsyncWindowsFileStreamStrategy.cs | 15 +---- .../IO/SyncWindowsFileStreamStrategy.cs | 6 -- .../System/IO/WindowsFileStreamStrategy.cs | 59 ++----------------- 4 files changed, 7 insertions(+), 77 deletions(-) diff --git a/src/libraries/System.IO.FileSystem/tests/FileStream/SafeFileHandle.cs b/src/libraries/System.IO.FileSystem/tests/FileStream/SafeFileHandle.cs index 020a25e48871ed..61a7d5f0b8f0cc 100644 --- a/src/libraries/System.IO.FileSystem/tests/FileStream/SafeFileHandle.cs +++ b/src/libraries/System.IO.FileSystem/tests/FileStream/SafeFileHandle.cs @@ -58,13 +58,13 @@ public void AccessFlushesFileClosesHandle() } } - [Fact] + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsLegacyFileStreamEnabled))] public async Task ThrowWhenHandlePositionIsChanged_sync() { await ThrowWhenHandlePositionIsChanged(useAsync: false); } - [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))] + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported), nameof(PlatformDetection.IsLegacyFileStreamEnabled))] public async Task ThrowWhenHandlePositionIsChanged_async() { await ThrowWhenHandlePositionIsChanged(useAsync: true); 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 dadffb0164ffb6..7cde9a24730ff4 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/AsyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/AsyncWindowsFileStreamStrategy.cs @@ -96,7 +96,6 @@ protected override void OnInit() if (_fileHandle.ThreadPoolBinding == null) { // We should close the handle so that the handle is not open until SafeFileHandle GC - Debug.Assert(!_exposedHandle, "Are we closing handle that we exposed/not own, how?"); _fileHandle.Dispose(); } } @@ -145,9 +144,6 @@ private unsafe Task ReadAsyncInternal(Memory destination, Cancellatio { long len = Length; - // Make sure we are reading from the position that we think we are - VerifyOSHandlePosition(); - if (_filePosition + destination.Length > len) { if (_filePosition <= len) @@ -273,9 +269,6 @@ private unsafe Task WriteAsyncInternalCore(ReadOnlyMemory source, Cancella // Make sure we set the length of the file appropriately. long len = Length; - // Make sure we are writing to the position that we think we are - VerifyOSHandlePosition(); - if (_filePosition + source.Length > len) { SetLengthCore(_filePosition + source.Length); @@ -381,16 +374,10 @@ private async Task AsyncModeCopyToAsync(Stream destination, int bufferSize, Canc Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed"); Debug.Assert(CanRead, "_parent.CanRead"); - bool canSeek = CanSeek; - if (canSeek) - { - VerifyOSHandlePosition(); - } - try { #pragma warning disable CA2007 // Consider calling ConfigureAwait on the awaited task - await FileStreamHelpers.AsyncModeCopyToAsync(_fileHandle, _path, canSeek, _filePosition, destination, bufferSize, cancellationToken); + await FileStreamHelpers.AsyncModeCopyToAsync(_fileHandle, _path, CanSeek, _filePosition, destination, bufferSize, cancellationToken); #pragma warning restore CA2007 // Consider calling ConfigureAwait on the awaited task } finally diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/SyncWindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/SyncWindowsFileStreamStrategy.cs index 44563299c579f4..5198b512c3a9a5 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/SyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/SyncWindowsFileStreamStrategy.cs @@ -104,9 +104,6 @@ private unsafe int ReadSpan(Span destination) Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed"); - // Make sure we are reading from the right spot - VerifyOSHandlePosition(); - int r = FileStreamHelpers.ReadFileNative(_fileHandle, destination, null, out int errorCode); if (r == -1) @@ -139,9 +136,6 @@ private unsafe void WriteSpan(ReadOnlySpan source) Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed"); - // Make sure we are writing to the position that we think we are - VerifyOSHandlePosition(); - int r = FileStreamHelpers.WriteFileNative(_fileHandle, source, null, out int errorCode); if (r == -1) 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 e6f8116ddddb22..654f18bb81de45 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/WindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/WindowsFileStreamStrategy.cs @@ -31,15 +31,10 @@ internal abstract class WindowsFileStreamStrategy : FileStreamStrategy 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; - private long _appendStart; // When appending, prevent overwriting file. internal WindowsFileStreamStrategy(SafeFileHandle handle, FileAccess access) { - _exposedHandle = true; - InitFromHandle(handle, access, out _canSeek, out _isPipe); // Note: Cleaner to set the following fields in ValidateAndInitFromHandle, @@ -87,31 +82,16 @@ internal WindowsFileStreamStrategy(string path, FileMode mode, FileAccess access /// Gets or sets the position within the current stream public override long Position { - get - { - VerifyOSHandlePosition(); - - return _filePosition; - } - set - { - Seek(value, SeekOrigin.Begin); - } + get => _filePosition; + set => Seek(value, SeekOrigin.Begin); } internal sealed override string Name => _path ?? SR.IO_UnknownFileName; internal sealed override bool IsClosed => _fileHandle.IsClosed; - internal sealed override SafeFileHandle SafeFileHandle - { - get - { - // Flushing is the responsibility of BufferedFileStreamStrategy - _exposedHandle = true; - return _fileHandle; - } - } + // Flushing is the responsibility of BufferedFileStreamStrategy + internal sealed override SafeFileHandle SafeFileHandle => _fileHandle; // ReadByte and WriteByte methods are used only when the user has disabled buffering on purpose // their performance is going to be horrible @@ -171,9 +151,6 @@ public sealed override long Seek(long offset, SeekOrigin origin) if (_fileHandle.IsClosed) throw Error.GetFileNotOpen(); if (!CanSeek) throw Error.GetSeekNotSupported(); - // Verify that internal position is in sync with the handle - VerifyOSHandlePosition(); - long oldPos = _filePosition; long pos = SeekCore(_fileHandle, offset, origin); @@ -267,7 +244,6 @@ public sealed override void SetLength(long value) protected unsafe void SetLengthCore(long value) { Debug.Assert(value >= 0, "value >= 0"); - VerifyOSHandlePosition(); FileStreamHelpers.SetLength(_fileHandle, _path, value); @@ -276,32 +252,5 @@ protected unsafe void SetLengthCore(long value) SeekCore(_fileHandle, 0, SeekOrigin.End); } } - - /// - /// Verify that the actual position of the OS's handle equals what we expect it to. - /// This will fail if someone else moved the UnixFileStream's handle or if - /// our position updating code is incorrect. - /// - [MethodImpl(MethodImplOptions.AggressiveInlining)] - protected void VerifyOSHandlePosition() - { - bool verifyPosition = _exposedHandle; // in release, only verify if we've given out the handle such that someone else could be manipulating it -#if DEBUG - verifyPosition = true; // in debug, always make sure our position matches what the OS says it should be -#endif - if (verifyPosition && CanSeek) - { - long oldPos = _filePosition; // SeekCore will override the current _position, so save it now - long curPos = SeekCore(_fileHandle, 0, SeekOrigin.Current); - if (oldPos != curPos) - { - // For reads, this is non-fatal but we still could have returned corrupted - // data in some cases, so discard the internal buffer. For writes, - // this is a problem; discard the buffer and error out. - - throw new IOException(SR.IO_FileStreamHandlePosition); - } - } - } } } From f6c8647c1903de317f7b9e9aaf3af412dc85319f Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 4 Mar 2021 10:22:05 +0100 Subject: [PATCH 18/20] track the file offset in memory, don't use expensive sys calls to synchronize it --- ...op.ReadFile_SafeHandle_NativeOverlapped.cs | 8 +++ ...p.WriteFile_SafeHandle_NativeOverlapped.cs | 3 ++ .../IO/AsyncWindowsFileStreamStrategy.cs | 44 ++++++++-------- .../System/IO/FileStreamHelpers.Windows.cs | 36 ++++++++++--- .../IO/LegacyFileStreamStrategy.Windows.cs | 4 +- .../IO/SyncWindowsFileStreamStrategy.cs | 16 +++++- .../System/IO/WindowsFileStreamStrategy.cs | 50 +++++++++++-------- 7 files changed, 106 insertions(+), 55 deletions(-) diff --git a/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.ReadFile_SafeHandle_NativeOverlapped.cs b/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.ReadFile_SafeHandle_NativeOverlapped.cs index 84199322492994..c6fb2c4311cf31 100644 --- a/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.ReadFile_SafeHandle_NativeOverlapped.cs +++ b/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.ReadFile_SafeHandle_NativeOverlapped.cs @@ -16,5 +16,13 @@ internal static extern unsafe int ReadFile( int numBytesToRead, IntPtr numBytesRead_mustBeZero, NativeOverlapped* overlapped); + + [DllImport(Libraries.Kernel32, SetLastError = true)] + internal static extern unsafe int ReadFile( + SafeHandle handle, + byte* bytes, + int numBytesToRead, + out int numBytesRead, + NativeOverlapped* overlapped); } } diff --git a/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.WriteFile_SafeHandle_NativeOverlapped.cs b/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.WriteFile_SafeHandle_NativeOverlapped.cs index cca93e0b3ac895..3eaff9ca076260 100644 --- a/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.WriteFile_SafeHandle_NativeOverlapped.cs +++ b/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.WriteFile_SafeHandle_NativeOverlapped.cs @@ -17,5 +17,8 @@ internal static partial class Kernel32 // and pass in an address for the numBytesRead parameter. [DllImport(Libraries.Kernel32, SetLastError = true)] internal static extern unsafe int WriteFile(SafeHandle handle, byte* bytes, int numBytesToWrite, IntPtr numBytesWritten_mustBeZero, NativeOverlapped* lpOverlapped); + + [DllImport(Libraries.Kernel32, SetLastError = true)] + internal static extern unsafe int WriteFile(SafeHandle handle, byte* bytes, int numBytesToWrite, out int numBytesWritten, NativeOverlapped* lpOverlapped); } } 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 7cde9a24730ff4..abc30c94c33fda 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/AsyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/AsyncWindowsFileStreamStrategy.cs @@ -140,15 +140,16 @@ private unsafe Task ReadAsyncInternal(Memory destination, Cancellatio NativeOverlapped* intOverlapped = completionSource.Overlapped; // Calculate position in the file we should be at after the read is done + long positionBefore = _filePosition; if (CanSeek) { long len = Length; - if (_filePosition + destination.Length > len) + if (positionBefore + destination.Length > len) { - if (_filePosition <= len) + if (positionBefore <= len) { - destination = destination.Slice(0, (int)(len - _filePosition)); + destination = destination.Slice(0, (int)(len - positionBefore)); } else { @@ -158,23 +159,17 @@ private unsafe Task ReadAsyncInternal(Memory destination, Cancellatio // Now set the position to read from in the NativeOverlapped struct // For pipes, we should leave the offset fields set to 0. - intOverlapped->OffsetLow = unchecked((int)_filePosition); - intOverlapped->OffsetHigh = (int)(_filePosition >> 32); + intOverlapped->OffsetLow = unchecked((int)positionBefore); + intOverlapped->OffsetHigh = (int)(positionBefore >> 32); // When using overlapped IO, the OS is not supposed to // touch the file pointer location at all. We will adjust it - // ourselves. This isn't threadsafe. - - // WriteFile should not update the file pointer when writing - // in overlapped mode, according to MSDN. But it does update - // the file pointer when writing to a UNC path! - // So changed the code below to seek to an absolute - // location, not a relative one. ReadFile seems consistent though. - SeekCore(_fileHandle, destination.Length, SeekOrigin.Current); + // ourselves, but only in memory. This isn't threadsafe. + _filePosition += destination.Length; } // queue an async ReadFile operation and pass in a packed overlapped - int r = FileStreamHelpers.ReadFileNative(_fileHandle, destination.Span, intOverlapped, out int errorCode); + int r = FileStreamHelpers.ReadFileNative(_fileHandle, destination.Span, false, intOverlapped, out int errorCode); // ReadFile, the OS version, will return 0 on failure. But // my ReadFileNative wrapper returns -1. My wrapper will return @@ -203,7 +198,7 @@ private unsafe Task ReadAsyncInternal(Memory destination, Cancellatio { if (!_fileHandle.IsClosed && CanSeek) // Update Position - It could be anywhere. { - SeekCore(_fileHandle, 0, SeekOrigin.Current); + _filePosition = positionBefore; } completionSource.ReleaseNativeResource(); @@ -264,29 +259,30 @@ private unsafe Task WriteAsyncInternalCore(ReadOnlyMemory source, Cancella FileStreamCompletionSource completionSource = FileStreamCompletionSource.Create(this, _preallocatedOverlapped, 0, source); NativeOverlapped* intOverlapped = completionSource.Overlapped; + long positionBefore = _filePosition; if (CanSeek) { // Make sure we set the length of the file appropriately. long len = Length; - if (_filePosition + source.Length > len) + if (positionBefore + source.Length > len) { - SetLengthCore(_filePosition + source.Length); + SetLengthCore(positionBefore + source.Length); } // Now set the position to read from in the NativeOverlapped struct // For pipes, we should leave the offset fields set to 0. - intOverlapped->OffsetLow = (int)_filePosition; - intOverlapped->OffsetHigh = (int)(_filePosition >> 32); + intOverlapped->OffsetLow = (int)positionBefore; + intOverlapped->OffsetHigh = (int)(positionBefore >> 32); // When using overlapped IO, the OS is not supposed to // touch the file pointer location at all. We will adjust it - // ourselves. This isn't threadsafe. - SeekCore(_fileHandle, source.Length, SeekOrigin.Current); + // ourselves, but only in memory. This isn't threadsafe. + _filePosition += source.Length; } // queue an async WriteFile operation and pass in a packed overlapped - int r = FileStreamHelpers.WriteFileNative(_fileHandle, source.Span, intOverlapped, out int errorCode); + int r = FileStreamHelpers.WriteFileNative(_fileHandle, source.Span, false, intOverlapped, out int errorCode); // WriteFile, the OS version, will return 0 on failure. But // my WriteFileNative wrapper returns -1. My wrapper will return @@ -312,7 +308,7 @@ private unsafe Task WriteAsyncInternalCore(ReadOnlyMemory source, Cancella { if (!_fileHandle.IsClosed && CanSeek) // Update Position - It could be anywhere. { - SeekCore(_fileHandle, 0, SeekOrigin.Current); + _filePosition = positionBefore; } completionSource.ReleaseNativeResource(); @@ -385,7 +381,7 @@ private async Task AsyncModeCopyToAsync(Stream destination, int bufferSize, Canc // Make sure the stream's current position reflects where we ended up if (!_fileHandle.IsClosed && CanSeek) { - SeekCore(_fileHandle, 0, SeekOrigin.End); + _filePosition = Length; } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs index 840d62cb03b80e..d1d2cdb42686ac 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs @@ -337,7 +337,7 @@ internal static unsafe void SetLength(SafeFileHandle handle, string? path, long } // __ConsoleStream also uses this code. - internal static unsafe int ReadFileNative(SafeFileHandle handle, Span bytes, NativeOverlapped* overlapped, out int errorCode) + internal static unsafe int ReadFileNative(SafeFileHandle handle, Span bytes, bool syncUsingOverlapped, NativeOverlapped* overlapped, out int errorCode) { Debug.Assert(handle != null, "handle != null"); @@ -347,13 +347,24 @@ internal static unsafe int ReadFileNative(SafeFileHandle handle, Span byte fixed (byte* p = &MemoryMarshal.GetReference(bytes)) { r = overlapped != null ? - Interop.Kernel32.ReadFile(handle, p, bytes.Length, IntPtr.Zero, overlapped) : - Interop.Kernel32.ReadFile(handle, p, bytes.Length, out numBytesRead, IntPtr.Zero); + (syncUsingOverlapped + ? Interop.Kernel32.ReadFile(handle, p, bytes.Length, out numBytesRead, overlapped) + : Interop.Kernel32.ReadFile(handle, p, bytes.Length, IntPtr.Zero, overlapped)) + : Interop.Kernel32.ReadFile(handle, p, bytes.Length, out numBytesRead, IntPtr.Zero); } if (r == 0) { errorCode = GetLastWin32ErrorAndDisposeHandleIfInvalid(handle); + + if (syncUsingOverlapped && errorCode == Interop.Errors.ERROR_HANDLE_EOF) + { + // https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-readfile#synchronization-and-file-position : + // "If lpOverlapped is not NULL, then when a synchronous read operation reaches the end of a file, + // ReadFile returns FALSE and GetLastError returns ERROR_HANDLE_EOF" + return numBytesRead; + } + return -1; } else @@ -363,7 +374,7 @@ internal static unsafe int ReadFileNative(SafeFileHandle handle, Span byte } } - internal static unsafe int WriteFileNative(SafeFileHandle handle, ReadOnlySpan buffer, NativeOverlapped* overlapped, out int errorCode) + internal static unsafe int WriteFileNative(SafeFileHandle handle, ReadOnlySpan buffer, bool syncUsingOverlapped, NativeOverlapped* overlapped, out int errorCode) { Debug.Assert(handle != null, "handle != null"); @@ -373,13 +384,24 @@ internal static unsafe int WriteFileNative(SafeFileHandle handle, ReadOnlySpan= 0; + synchronousSuccess = ReadFileNative(handle, copyBuffer, false, readAwaitable._nativeOverlapped, out errorCode) >= 0; } // If the operation did not synchronously succeed, it either failed or initiated the asynchronous operation. 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 0e236cfa5097ff..0fd86c91dd8017 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 @@ -1079,14 +1079,14 @@ private unsafe int ReadFileNative(SafeFileHandle handle, Span bytes, Nativ { Debug.Assert((_useAsyncIO && overlapped != null) || (!_useAsyncIO && overlapped == null), "Async IO and overlapped parameters inconsistent in call to ReadFileNative."); - return FileStreamHelpers.ReadFileNative(handle, bytes, overlapped, out errorCode); + return FileStreamHelpers.ReadFileNative(handle, bytes, false, overlapped, out errorCode); } private unsafe int WriteFileNative(SafeFileHandle handle, ReadOnlySpan buffer, NativeOverlapped* overlapped, out int errorCode) { Debug.Assert((_useAsyncIO && overlapped != null) || (!_useAsyncIO && overlapped == null), "Async IO and overlapped parameters inconsistent in call to WriteFileNative."); - return FileStreamHelpers.WriteFileNative(handle, buffer, overlapped, out errorCode); + return FileStreamHelpers.WriteFileNative(handle, buffer, false, overlapped, out errorCode); } public override Task CopyToAsync(Stream destination, int bufferSize, CancellationToken cancellationToken) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/SyncWindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/SyncWindowsFileStreamStrategy.cs index 5198b512c3a9a5..e722867e3b9ccc 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/SyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/SyncWindowsFileStreamStrategy.cs @@ -104,7 +104,8 @@ private unsafe int ReadSpan(Span destination) Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed"); - int r = FileStreamHelpers.ReadFileNative(_fileHandle, destination, null, out int errorCode); + NativeOverlapped nativeOverlapped = GetNativeOverlappedForCurrentPosition(); + int r = FileStreamHelpers.ReadFileNative(_fileHandle, destination, true, &nativeOverlapped, out int errorCode); if (r == -1) { @@ -136,7 +137,8 @@ private unsafe void WriteSpan(ReadOnlySpan source) Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed"); - int r = FileStreamHelpers.WriteFileNative(_fileHandle, source, null, out int errorCode); + NativeOverlapped nativeOverlapped = GetNativeOverlappedForCurrentPosition(); + int r = FileStreamHelpers.WriteFileNative(_fileHandle, source, true, &nativeOverlapped, out int errorCode); if (r == -1) { @@ -159,5 +161,15 @@ private unsafe void WriteSpan(ReadOnlySpan source) _filePosition += r; return; } + + private NativeOverlapped GetNativeOverlappedForCurrentPosition() + { + NativeOverlapped nativeOverlapped = default; + // For pipes the offsets are ignored by the OS + nativeOverlapped.OffsetLow = unchecked((int)_filePosition); + nativeOverlapped.OffsetHigh = (int)(_filePosition >> 32); + + return nativeOverlapped; + } } } 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 654f18bb81de45..15c4581069df25 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/WindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/WindowsFileStreamStrategy.cs @@ -83,7 +83,7 @@ internal WindowsFileStreamStrategy(string path, FileMode mode, FileAccess access public override long Position { get => _filePosition; - set => Seek(value, SeekOrigin.Begin); + set => _filePosition = value; } internal sealed override string Name => _path ?? SR.IO_UnknownFileName; @@ -91,6 +91,8 @@ public override long Position internal sealed override bool IsClosed => _fileHandle.IsClosed; // Flushing is the responsibility of BufferedFileStreamStrategy + // TODO: we might consider calling FileStreamHelpers.Seek(handle, _path, _filePosition, SeekOrigin.Begin); + // here to set the actual file offset internal sealed override SafeFileHandle SafeFileHandle => _fileHandle; // ReadByte and WriteByte methods are used only when the user has disabled buffering on purpose @@ -152,29 +154,33 @@ public sealed override long Seek(long offset, SeekOrigin origin) if (!CanSeek) throw Error.GetSeekNotSupported(); long oldPos = _filePosition; - long pos = SeekCore(_fileHandle, offset, origin); + long pos = origin switch + { + SeekOrigin.Begin => offset, + SeekOrigin.End => FileStreamHelpers.GetFileLength(_fileHandle, _path) + offset, + _ => _filePosition + offset // SeekOrigin.Current + }; + + if (pos >= 0) + { + _filePosition = pos; + } + else + { + // keep throwing the same exception we did when seek was causing actual offset change + throw Win32Marshal.GetExceptionForWin32Error(Interop.Errors.ERROR_INVALID_PARAMETER); + } - // Prevent users from overwriting data in a file that was opened in - // append mode. + // Prevent users from overwriting data in a file that was opened in append mode. if (_appendStart != -1 && pos < _appendStart) { - SeekCore(_fileHandle, oldPos, SeekOrigin.Begin); + _filePosition = oldPos; throw new IOException(SR.IO_SeekAppendOverwrite); } return pos; } - // This doesn't do argument checking. Necessary for SetLength, which must - // set the file pointer beyond the end of the file. This will update the - // internal position - protected long SeekCore(SafeFileHandle fileHandle, long offset, SeekOrigin origin, bool closeInvalidHandle = false) - { - Debug.Assert(!fileHandle.IsClosed && _canSeek, "!fileHandle.IsClosed && _canSeek"); - - return _filePosition = FileStreamHelpers.Seek(fileHandle, _path, offset, origin, closeInvalidHandle); - } - internal sealed override void Lock(long position, long length) => FileStreamHelpers.Lock(_fileHandle, _path, position, length); internal sealed override void Unlock(long position, long length) => FileStreamHelpers.Unlock(_fileHandle, _path, position, length); @@ -192,7 +198,7 @@ private void Init(FileMode mode, string originalPath) // For Append mode... if (mode == FileMode.Append) { - _appendStart = SeekCore(_fileHandle, 0, SeekOrigin.End); + _appendStart = _filePosition = Length; } else { @@ -226,9 +232,15 @@ private void InitFromHandleImpl(SafeFileHandle handle, out bool canSeek, out boo OnInitFromHandle(handle); if (_canSeek) - SeekCore(handle, 0, SeekOrigin.Current); + { + // given strategy was created out of existing handle, so we have to perform + // a syscall to get the current handle offset + _filePosition = FileStreamHelpers.Seek(handle, _path, 0, SeekOrigin.Current); + } else + { _filePosition = 0; + } } public sealed override void SetLength(long value) @@ -239,8 +251,6 @@ public sealed override void SetLength(long value) SetLengthCore(value); } - // We absolutely need this method broken out so that WriteInternalCoreAsync can call - // a method without having to go through buffering code that might call FlushWrite. protected unsafe void SetLengthCore(long value) { Debug.Assert(value >= 0, "value >= 0"); @@ -249,7 +259,7 @@ protected unsafe void SetLengthCore(long value) if (_filePosition > value) { - SeekCore(_fileHandle, 0, SeekOrigin.End); + _filePosition = value; } } } From 60eedb4e756cc27b76a063882338547b5dc5ef99 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 5 Mar 2021 11:17:44 +0100 Subject: [PATCH 19/20] there is no need to set the Length since we are now tracking the offset in memory --- .../src/System/IO/AsyncWindowsFileStreamStrategy.cs | 8 -------- 1 file changed, 8 deletions(-) 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 abc30c94c33fda..baa0bae4eb2b03 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/AsyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/AsyncWindowsFileStreamStrategy.cs @@ -262,14 +262,6 @@ private unsafe Task WriteAsyncInternalCore(ReadOnlyMemory source, Cancella long positionBefore = _filePosition; if (CanSeek) { - // Make sure we set the length of the file appropriately. - long len = Length; - - if (positionBefore + source.Length > len) - { - SetLengthCore(positionBefore + source.Length); - } - // Now set the position to read from in the NativeOverlapped struct // For pipes, we should leave the offset fields set to 0. intOverlapped->OffsetLow = (int)positionBefore; From 72a4e94f84a1017dc8c780397dacd7781a82fd6d Mon Sep 17 00:00:00 2001 From: David Cantu Date: Mon, 15 Mar 2021 01:26:47 -0700 Subject: [PATCH 20/20] Cache GetFileInformationByHandleEx (Length) when FileShare does not allow other processes to modify it --- .../IO/AsyncWindowsFileStreamStrategy.cs | 1 + .../src/System/IO/FileStream.cs | 2 +- .../IO/SyncWindowsFileStreamStrategy.cs | 2 +- .../System/IO/WindowsFileStreamStrategy.cs | 26 ++++++++++++++++++- 4 files changed, 28 insertions(+), 3 deletions(-) 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 baa0bae4eb2b03..7c7bcecd894236 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/AsyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/AsyncWindowsFileStreamStrategy.cs @@ -271,6 +271,7 @@ private unsafe Task WriteAsyncInternalCore(ReadOnlyMemory source, Cancella // touch the file pointer location at all. We will adjust it // ourselves, but only in memory. This isn't threadsafe. _filePosition += source.Length; + UpdateLengthOnChangePosition(); } // queue an async WriteFile operation and pass in a packed overlapped 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 6109cf014298c8..35b3c6a5e8ba11 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs @@ -12,7 +12,7 @@ namespace System.IO public class FileStream : Stream { internal const int DefaultBufferSize = 4096; - private const FileShare DefaultShare = FileShare.Read; + internal const FileShare DefaultShare = FileShare.Read; private const bool DefaultIsAsync = false; /// Caches whether Serialization Guard has been disabled for file writes diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/SyncWindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/SyncWindowsFileStreamStrategy.cs index e722867e3b9ccc..85900b82e2d83f 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/SyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/SyncWindowsFileStreamStrategy.cs @@ -159,7 +159,7 @@ private unsafe void WriteSpan(ReadOnlySpan source) } Debug.Assert(r >= 0, "FileStream's WriteCore is likely broken."); _filePosition += r; - return; + UpdateLengthOnChangePosition(); } private NativeOverlapped GetNativeOverlappedForCurrentPosition() 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 15c4581069df25..0badeb1cc74b63 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/WindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/WindowsFileStreamStrategy.cs @@ -23,6 +23,8 @@ internal abstract class WindowsFileStreamStrategy : FileStreamStrategy /// Whether the file is opened for reading, writing, or both. private readonly FileAccess _access; + private readonly FileShare _share; + /// The path to the opened file. protected readonly string? _path; @@ -32,6 +34,7 @@ internal abstract class WindowsFileStreamStrategy : FileStreamStrategy private readonly bool _isPipe; // Whether to disable async buffering code. private long _appendStart; // When appending, prevent overwriting file. + private long? _length; internal WindowsFileStreamStrategy(SafeFileHandle handle, FileAccess access) { @@ -40,6 +43,7 @@ internal WindowsFileStreamStrategy(SafeFileHandle handle, FileAccess access) // Note: Cleaner to set the following fields in ValidateAndInitFromHandle, // but we can't as they're readonly. _access = access; + _share = FileStream.DefaultShare; // As the handle was passed in, we must set the handle field at the very end to // avoid the finalizer closing the handle when we throw errors. @@ -52,6 +56,7 @@ internal WindowsFileStreamStrategy(string path, FileMode mode, FileAccess access _path = fullPath; _access = access; + _share = share; _fileHandle = FileStreamHelpers.OpenHandle(fullPath, mode, access, share, options); @@ -77,7 +82,25 @@ internal WindowsFileStreamStrategy(string path, FileMode mode, FileAccess access public sealed override bool CanWrite => !_fileHandle.IsClosed && (_access & FileAccess.Write) != 0; - public unsafe sealed override long Length => FileStreamHelpers.GetFileLength(_fileHandle, _path); + public unsafe sealed override long Length => _share > FileShare.Read ? + FileStreamHelpers.GetFileLength(_fileHandle, _path) : + _length ??= FileStreamHelpers.GetFileLength(_fileHandle, _path); + + protected void UpdateLengthOnChangePosition() + { + // Do not update the cached length if the file can be written somewhere else + // or if the length has not been queried. + if (_share > FileShare.Read || _length is null) + { + Debug.Assert(_length is null); + return; + } + + if (_filePosition > _length) + { + _length = _filePosition; + } + } /// Gets or sets the position within the current stream public override long Position @@ -256,6 +279,7 @@ protected unsafe void SetLengthCore(long value) Debug.Assert(value >= 0, "value >= 0"); FileStreamHelpers.SetLength(_fileHandle, _path, value); + _length = value; if (_filePosition > value) {