From bbc28e97c3e21b781cb9ac335bc2900ae102054e Mon Sep 17 00:00:00 2001 From: carlossanlop Date: Sat, 3 Apr 2021 15:52:02 -0700 Subject: [PATCH 01/19] Remove IFileStreamCompletionStrategy. This makes FileStreamCompletionSource only needed by Net5CompatFileStreamStrategy. --- .../FileStreamCompletionSource.Win32.cs | 33 +++++++------------ .../Net5CompatFileStreamStrategy.Windows.cs | 10 ++---- .../Net5CompatFileStreamStrategy.cs | 2 +- 3 files changed, 15 insertions(+), 30 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamCompletionSource.Win32.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamCompletionSource.Win32.cs index 679d62422a4aef..377137215e07f1 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamCompletionSource.Win32.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamCompletionSource.Win32.cs @@ -10,17 +10,6 @@ namespace System.IO.Strategies { - // to avoid code duplicaiton of FileStreamCompletionSource for Net5CompatFileStreamStrategy 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. @@ -39,7 +28,7 @@ internal unsafe class FileStreamCompletionSource : TaskCompletionSource private static Action? s_cancelCallback; - private readonly IFileStreamCompletionSourceStrategy _strategy; + private readonly Net5CompatFileStreamStrategy _strategy; private readonly int _numBufferedBytes; private CancellationTokenRegistration _cancellationRegistration; #if DEBUG @@ -49,7 +38,7 @@ internal unsafe class FileStreamCompletionSource : TaskCompletionSource 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, + internal FileStreamCompletionSource(Net5CompatFileStreamStrategy strategy, PreAllocatedOverlapped? preallocatedOverlapped, int numBufferedBytes, byte[]? bytes) : base(TaskCreationOptions.RunContinuationsAsynchronously) { _numBufferedBytes = numBufferedBytes; @@ -59,8 +48,8 @@ internal FileStreamCompletionSource(IFileStreamCompletionSourceStrategy strategy // 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); + 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"); } @@ -119,7 +108,7 @@ internal virtual void ReleaseNativeResource() // (this is why we disposed the registration above). if (_overlapped != null) { - _strategy.FileHandle.ThreadPoolBinding!.FreeNativeOverlapped(_overlapped); + _strategy._fileHandle.ThreadPoolBinding!.FreeNativeOverlapped(_overlapped); _overlapped = null; } @@ -139,10 +128,10 @@ internal static void IOCallback(uint errorCode, uint numBytes, NativeOverlapped* // 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); + Debug.Assert(state is Net5CompatFileStreamStrategy || state is FileStreamCompletionSource); FileStreamCompletionSource completionSource = state switch { - IFileStreamCompletionSourceStrategy strategy => strategy.CurrentOverlappedOwner!, // must be owned + Net5CompatFileStreamStrategy strategy => strategy._currentOverlappedOwner!, // must be owned _ => (FileStreamCompletionSource)state }; Debug.Assert(completionSource != null); @@ -215,8 +204,8 @@ private static void Cancel(object? 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)) + if (!completionSource._strategy._fileHandle.IsInvalid && + !Interop.Kernel32.CancelIoEx(completionSource._strategy._fileHandle, completionSource._overlapped)) { int errorCode = Marshal.GetLastWin32Error(); @@ -229,7 +218,7 @@ private static void Cancel(object? state) } } - public static FileStreamCompletionSource Create(IFileStreamCompletionSourceStrategy strategy, PreAllocatedOverlapped? preallocatedOverlapped, + public static FileStreamCompletionSource Create(Net5CompatFileStreamStrategy strategy, PreAllocatedOverlapped? preallocatedOverlapped, int numBufferedBytesRead, ReadOnlyMemory memory) { // If the memory passed in is the strategy's internal buffer, we can use the base FileStreamCompletionSource, @@ -252,7 +241,7 @@ internal sealed class MemoryFileStreamCompletionSource : FileStreamCompletionSou { private MemoryHandle _handle; // mutable struct; do not make this readonly - internal MemoryFileStreamCompletionSource(IFileStreamCompletionSourceStrategy strategy, int numBufferedBytes, ReadOnlyMemory memory) + internal MemoryFileStreamCompletionSource(Net5CompatFileStreamStrategy strategy, int numBufferedBytes, ReadOnlyMemory memory) : base(strategy, null, numBufferedBytes, null) // this type handles the pinning, so null is passed for bytes { _handle = memory.Pin(); diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.Windows.cs index 5ed9f5937aa21a..9b0849980c4813 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.Windows.cs @@ -36,7 +36,7 @@ namespace System.IO.Strategies { - internal sealed partial class Net5CompatFileStreamStrategy : FileStreamStrategy, IFileStreamCompletionSourceStrategy + internal sealed partial class Net5CompatFileStreamStrategy : FileStreamStrategy { private bool _canSeek; private bool _isPipe; // Whether to disable async buffering code. @@ -44,7 +44,7 @@ internal sealed partial class Net5CompatFileStreamStrategy : FileStreamStrategy, 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 + internal FileStreamCompletionSource? _currentOverlappedOwner; // async op currently using the preallocated overlapped private void Init(FileMode mode, FileShare share, string originalPath, FileOptions options) { @@ -547,11 +547,7 @@ partial void OnBufferAllocated() _preallocatedOverlapped = new PreAllocatedOverlapped(FileStreamCompletionSource.s_ioCallback, this, _buffer); } - SafeFileHandle IFileStreamCompletionSourceStrategy.FileHandle => _fileHandle; - - FileStreamCompletionSource? IFileStreamCompletionSourceStrategy.CurrentOverlappedOwner => _currentOverlappedOwner; - - FileStreamCompletionSource? IFileStreamCompletionSourceStrategy.CompareExchangeCurrentOverlappedOwner(FileStreamCompletionSource? newSource, FileStreamCompletionSource? existingSource) + internal FileStreamCompletionSource? CompareExchangeCurrentOverlappedOwner(FileStreamCompletionSource? newSource, FileStreamCompletionSource? existingSource) => Interlocked.CompareExchange(ref _currentOverlappedOwner, newSource, existingSource); private void WriteSpan(ReadOnlySpan source) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.cs index 03ca3533e717b7..ca4e5a3e0dad9f 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.cs @@ -14,7 +14,7 @@ internal sealed partial class Net5CompatFileStreamStrategy : FileStreamStrategy { private byte[]? _buffer; private readonly int _bufferLength; - private readonly SafeFileHandle _fileHandle; // only ever null if ctor throws + internal readonly SafeFileHandle _fileHandle; // only ever null if ctor throws /// Whether the file is opened for reading, writing, or both. private readonly FileAccess _access; From 7449c79942641895db8817c53779d3fcbe1ff65e Mon Sep 17 00:00:00 2001 From: carlossanlop Date: Sat, 3 Apr 2021 16:58:03 -0700 Subject: [PATCH 02/19] Use AwaitableProvider for AsyncWindowsFileStreamStrategy. --- .../System.Private.CoreLib.Shared.projitems | 5 +- ...owsFileStreamStrategy.AwaitableProvider.cs | 449 ++++++++++++++++++ .../AsyncWindowsFileStreamStrategy.cs | 201 +------- 3 files changed, 466 insertions(+), 189 deletions(-) create mode 100644 src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.AwaitableProvider.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 c67c432163c73d..b37121054fac6c 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 @@ -1100,10 +1100,10 @@ Common\Interop\Interop.ResultCode.cs - + Common\Interop\Interop.TimeZoneDisplayNameType.cs - + Common\Interop\Interop.TimeZoneInfo.cs @@ -1652,6 +1652,7 @@ + diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.AwaitableProvider.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.AwaitableProvider.cs new file mode 100644 index 00000000000000..5863e08c84b2d5 --- /dev/null +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.AwaitableProvider.cs @@ -0,0 +1,449 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Internal; +using System.Buffers; +using System.Diagnostics; +using System.Runtime.ExceptionServices; +using System.Runtime.InteropServices; +using System.Threading; +using System.Threading.Tasks; +using System.Threading.Tasks.Sources; + +namespace System.IO.Strategies +{ + internal sealed partial class AsyncWindowsFileStreamStrategy : WindowsFileStreamStrategy + { + internal sealed class ManualResetValueTaskSource : IValueTaskSource, IValueTaskSource + { + private ManualResetValueTaskSourceCore _core; // mutable struct; do not make this readonly + + public bool RunContinuationsAsynchronously + { + get => _core.RunContinuationsAsynchronously; + set => _core.RunContinuationsAsynchronously = value; + } + + public short Version => _core.Version; + public void Reset() => _core.Reset(); + public void SetResult(T result) => _core.SetResult(result); + public void SetException(Exception error) => _core.SetException(error); + + public T GetResult(short token) => _core.GetResult(token); + void IValueTaskSource.GetResult(short token) => _core.GetResult(token); + public ValueTaskSourceStatus GetStatus(short token) => _core.GetStatus(token); + public void OnCompleted(Action continuation, object? state, short token, ValueTaskSourceOnCompletedFlags flags) => _core.OnCompleted(continuation, state, token, flags); + + public ValueTaskSourceStatus GetStatus() => _core.GetStatus(_core.Version); + } + + internal unsafe class AwaitableProvider + { + 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; + + protected readonly AsyncWindowsFileStreamStrategy _strategy; + protected readonly int _numBufferedBytes; + + protected NativeOverlapped* _overlapped; + private ManualResetValueTaskSource _source; + private CancellationTokenRegistration _cancellationRegistration; + private long _result; // Using long since this needs to be used in Interlocked APIs +#if DEBUG + private bool _cancellationHasBeenRegistered; +#endif + + public static AwaitableProvider Create( + AsyncWindowsFileStreamStrategy strategy, + PreAllocatedOverlapped? preallocatedOverlapped, + int numBufferedBytesRead, + ReadOnlyMemory memory) + { + // If the memory passed in is the strategy's internal buffer, we can use the base AwaitableProvider, + // which has a PreAllocatedOverlapped with the memory already pinned. Otherwise, we use the derived + // MemoryAwaitableProvider, 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.IsUserObject(buffer.Array) ? + new AwaitableProvider(strategy, preallocatedOverlapped, numBufferedBytesRead, buffer.Array) : + new MemoryAwaitableProvider(strategy, numBufferedBytesRead, memory); + } + + protected AwaitableProvider( + AsyncWindowsFileStreamStrategy strategy, + PreAllocatedOverlapped? preallocatedOverlapped, + int numBufferedBytes, + byte[]? bytes) + { + _strategy = strategy; + _numBufferedBytes = numBufferedBytes; + + _result = NoResult; + + _source = new ManualResetValueTaskSource(); + // Using RunContinuationsAsynchronously for compat reasons (old API used Task.Factory.StartNew for continuations) + _source.RunContinuationsAsynchronously = true; + + _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"); + } + + public ValueTask ReadAsync(Memory destination, CancellationToken cancellationToken) + { + // Calculate position in the file we should be at after the read is done + long positionBefore = _strategy._filePosition; + if (_strategy.CanSeek) + { + long len = _strategy.Length; + + if (positionBefore + destination.Length > len) + { + if (positionBefore <= len) + { + destination = destination.Slice(0, (int)(len - positionBefore)); + } + 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. + _overlapped->OffsetLow = unchecked((int)positionBefore); + _overlapped->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, but only in memory. This isn't threadsafe. + _strategy._filePosition += destination.Length; + } + + // queue an async ReadFile operation and pass in a packed overlapped + int r = FileStreamHelpers.ReadFileNative(_strategy._fileHandle, destination.Span, false, _overlapped, 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. + _overlapped->InternalLow = IntPtr.Zero; + ReleaseNativeResource(); + return new ValueTask(_numBufferedBytes); + } + else if (errorCode != ERROR_IO_PENDING) + { + if (!_strategy._fileHandle.IsClosed && _strategy.CanSeek) // Update Position - It could be anywhere. + { + _strategy._filePosition = positionBefore; + } + + ReleaseNativeResource(); + + if (errorCode == ERROR_HANDLE_EOF) + { + ThrowHelper.ThrowEndOfFileException(); + } + else + { + throw Win32Marshal.GetExceptionForWin32Error(errorCode, _strategy._path); + } + } + else if (cancellationToken.CanBeCanceled) // ERROR_IO_PENDING + { + // Only once the IO is pending do we register for cancellation + 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 new ValueTask(_source, _source.Version); + } + + public ValueTask WriteAsync(ReadOnlyMemory source, CancellationToken cancellationToken) + { + long positionBefore = _strategy._filePosition; + if (_strategy.CanSeek) + { + // Now set the position to read from in the NativeOverlapped struct + // For pipes, we should leave the offset fields set to 0. + _overlapped->OffsetLow = (int)positionBefore; + _overlapped->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, but only in memory. This isn't threadsafe. + _strategy._filePosition += source.Length; + _strategy.UpdateLengthOnChangePosition(); + } + + // queue an async WriteFile operation and pass in a packed overlapped + int r = FileStreamHelpers.WriteFileNative(_strategy._fileHandle, source.Span, false, _overlapped, 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. + ReleaseNativeResource(); + return ValueTask.CompletedTask; + } + else if (errorCode != ERROR_IO_PENDING) + { + if (!_strategy._fileHandle.IsClosed && _strategy.CanSeek) // Update Position - It could be anywhere. + { + _strategy._filePosition = positionBefore; + } + + ReleaseNativeResource(); + + if (errorCode == ERROR_HANDLE_EOF) + { + ThrowHelper.ThrowEndOfFileException(); + } + else + { + throw Win32Marshal.GetExceptionForWin32Error(errorCode, _strategy._path); + } + } + else if (cancellationToken.CanBeCanceled) // ERROR_IO_PENDING + { + // Only once the IO is pending do we register for cancellation + 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 new ValueTask(_source, _source.Version); + } + + private static void IOCallback(uint errorCode, uint numBytes, NativeOverlapped* pOverlapped) + { + // Extract the AwaitableProvider from the overlapped. The state in the overlapped + // will either be a AsyncWindowsFileStreamStrategy (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 AwaitableProvider 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 (AsyncWindowsFileStreamStrategy or AwaitableProvider)); + AwaitableProvider provider = state switch + { + AsyncWindowsFileStreamStrategy strategy => strategy._currentOverlappedOwner!, // must be owned + _ => (AwaitableProvider)state + }; + Debug.Assert(provider != null); + Debug.Assert(provider._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 (Interlocked.Exchange(ref provider._result, (long)packedResult) == NoResult) + { + // Successfully set the state, attempt to take back the callback + if (Interlocked.Exchange(ref provider._result, CompletedCallback) != NoResult) + { + // Successfully got the callback, finish the callback + provider.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) + { + _source.SetException(ExceptionDispatchInfo.SetCurrentStackTrace(new OperationCanceledException(cancellationToken.IsCancellationRequested ? cancellationToken : new CancellationToken(true)))); + } + else + { + Exception e = Win32Marshal.GetExceptionForWin32Error(errorCode); + e.SetCurrentStackTrace(); + _source.SetException(ExceptionDispatchInfo.SetCurrentStackTrace(e)); + } + } + else + { + Debug.Assert(result == ResultSuccess, "Unknown result"); + _source.SetResult((int)(packedResult & uint.MaxValue) + _numBufferedBytes); + } + } + + private 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) + { + // Register the cancellation only if the IO hasn't completed + long packedResult = Interlocked.CompareExchange(ref _result, RegisteringCancellation, NoResult); + if (packedResult == NoResult) + { + _cancellationRegistration = cancellationToken.UnsafeRegister(static (s, token) => ((AwaitableProvider)s!).Cancel(token), 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); + } + } + } + + private void Cancel(CancellationToken token) + { + // If the handle is still valid, attempt to cancel the IO + if (!_strategy._fileHandle.IsInvalid && + !Interop.Kernel32.CancelIoEx(_strategy._fileHandle, _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) + { + _source.SetException(ExceptionDispatchInfo.SetCurrentStackTrace( // TODO: Resource string for exception + new OperationCanceledException("IO operation cancelled.", Win32Marshal.GetExceptionForWin32Error(errorCode), token))); + } + } + } + + protected 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 AwaitableProvider (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); + } + } + + /// + /// 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 MemoryAwaitableProvider : AwaitableProvider + { + private MemoryHandle _handle; // mutable struct; do not make this readonly + + // this type handles the pinning, so bytes are null + internal unsafe MemoryAwaitableProvider(AsyncWindowsFileStreamStrategy strategy, int numBufferedBytes, ReadOnlyMemory memory) + : base (strategy, null, numBufferedBytes, null) // this type handles the pinning, so null is passed for bytes + { + _handle = memory.Pin(); + } + + protected override void ReleaseNativeResource() + { + _handle.Dispose(); + base.ReleaseNativeResource(); + } + } + } +} diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs index b7f24351a0bda4..716dbc994b1a4c 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs @@ -8,10 +8,10 @@ namespace System.IO.Strategies { - internal sealed partial class AsyncWindowsFileStreamStrategy : WindowsFileStreamStrategy, IFileStreamCompletionSourceStrategy + internal sealed partial class AsyncWindowsFileStreamStrategy : WindowsFileStreamStrategy { private PreAllocatedOverlapped? _preallocatedOverlapped; // optimization for async ops to avoid per-op allocations - private FileStreamCompletionSource? _currentOverlappedOwner; // async op currently using the preallocated overlapped + private AwaitableProvider? _currentOverlappedOwner; // async op currently using the preallocated overlapped internal AsyncWindowsFileStreamStrategy(SafeFileHandle handle, FileAccess access, FileShare share) : base(handle, access, share) @@ -108,26 +108,22 @@ internal override void OnBufferAllocated(byte[] buffer) { Debug.Assert(_preallocatedOverlapped == null); - _preallocatedOverlapped = new PreAllocatedOverlapped(FileStreamCompletionSource.s_ioCallback, this, buffer); + _preallocatedOverlapped = new PreAllocatedOverlapped(AwaitableProvider.s_ioCallback, this, buffer); } - SafeFileHandle IFileStreamCompletionSourceStrategy.FileHandle => _fileHandle; - - FileStreamCompletionSource? IFileStreamCompletionSourceStrategy.CurrentOverlappedOwner => _currentOverlappedOwner; - - FileStreamCompletionSource? IFileStreamCompletionSourceStrategy.CompareExchangeCurrentOverlappedOwner(FileStreamCompletionSource? newSource, FileStreamCompletionSource? existingSource) + internal AwaitableProvider? CompareExchangeCurrentOverlappedOwner(AwaitableProvider? newSource, AwaitableProvider? 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(); + => ReadAsyncInternal(new Memory(buffer, offset, count)).AsTask().GetAwaiter().GetResult(); public override Task ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) - => ReadAsyncInternal(new Memory(buffer, offset, count), cancellationToken); + => ReadAsyncInternal(new Memory(buffer, offset, count), cancellationToken).AsTask(); public override ValueTask ReadAsync(Memory destination, CancellationToken cancellationToken = default) - => new ValueTask(ReadAsyncInternal(destination, cancellationToken)); + => ReadAsyncInternal(destination, cancellationToken); - private unsafe Task ReadAsyncInternal(Memory destination, CancellationToken cancellationToken = default) + private unsafe ValueTask ReadAsyncInternal(Memory destination, CancellationToken cancellationToken = default) { if (!CanRead) { @@ -137,100 +133,8 @@ private unsafe Task ReadAsyncInternal(Memory destination, Cancellatio 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 - long positionBefore = _filePosition; - if (CanSeek) - { - long len = Length; - - if (positionBefore + destination.Length > len) - { - if (positionBefore <= len) - { - destination = destination.Slice(0, (int)(len - positionBefore)); - } - 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)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, 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, false, 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. - { - _filePosition = positionBefore; - } - - completionSource.ReleaseNativeResource(); - - if (errorCode == ERROR_HANDLE_EOF) - { - ThrowHelper.ThrowEndOfFileException(); - } - 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; + AwaitableProvider provider = AwaitableProvider.Create(this, _preallocatedOverlapped, 0, destination); + return provider.ReadAsync(destination, cancellationToken); } public override void Write(byte[] buffer, int offset, int count) @@ -244,10 +148,7 @@ public override ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationTo 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)); - - private unsafe Task WriteAsyncInternalCore(ReadOnlyMemory source, CancellationToken cancellationToken) + private unsafe ValueTask WriteAsyncInternal(ReadOnlyMemory source, CancellationToken cancellationToken) { if (!CanWrite) { @@ -257,85 +158,11 @@ private unsafe Task WriteAsyncInternalCore(ReadOnlyMemory source, Cancella 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; - - long positionBefore = _filePosition; - if (CanSeek) - { - // 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; - 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, but only in memory. This isn't threadsafe. - _filePosition += source.Length; - UpdateLengthOnChangePosition(); - } - - // queue an async WriteFile operation and pass in a packed overlapped - 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 - // 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. - { - _filePosition = positionBefore; - } - - completionSource.ReleaseNativeResource(); - - if (errorCode == ERROR_HANDLE_EOF) - { - ThrowHelper.ThrowEndOfFileException(); - } - 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; + AwaitableProvider provider = AwaitableProvider.Create(this, _preallocatedOverlapped, 0, source); + return provider.WriteAsync(source, cancellationToken); } + public override Task CopyToAsync(Stream destination, int bufferSize, CancellationToken cancellationToken) { ValidateCopyToArguments(destination, bufferSize); From 4ee6d26296d14c64d7b502c399d25ccaea8ef9e2 Mon Sep 17 00:00:00 2001 From: carlossanlop Date: Mon, 5 Apr 2021 18:35:15 -0700 Subject: [PATCH 03/19] No need for ManualResetValueTaskSource class. Remove it to get rid of one allocation and get the actual profiling improvement. Rename FileStreamAwaitableProvider. --- .../System.Private.CoreLib.Shared.projitems | 2 +- ...amStrategy.FileStreamAwaitableProvider.cs} | 60 +++++++------------ .../AsyncWindowsFileStreamStrategy.cs | 11 ++-- 3 files changed, 28 insertions(+), 45 deletions(-) rename src/libraries/System.Private.CoreLib/src/System/IO/Strategies/{AsyncWindowsFileStreamStrategy.AwaitableProvider.cs => AsyncWindowsFileStreamStrategy.FileStreamAwaitableProvider.cs} (90%) 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 b37121054fac6c..962ef332f3b95a 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 @@ -1652,7 +1652,7 @@ - + diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.AwaitableProvider.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.FileStreamAwaitableProvider.cs similarity index 90% rename from src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.AwaitableProvider.cs rename to src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.FileStreamAwaitableProvider.cs index 5863e08c84b2d5..3fefb79e01d924 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.AwaitableProvider.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.FileStreamAwaitableProvider.cs @@ -1,7 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using Internal; using System.Buffers; using System.Diagnostics; using System.Runtime.ExceptionServices; @@ -14,54 +13,36 @@ namespace System.IO.Strategies { internal sealed partial class AsyncWindowsFileStreamStrategy : WindowsFileStreamStrategy { - internal sealed class ManualResetValueTaskSource : IValueTaskSource, IValueTaskSource + internal unsafe class FileStreamAwaitableProvider : IValueTaskSource, IValueTaskSource { - private ManualResetValueTaskSourceCore _core; // mutable struct; do not make this readonly + private ManualResetValueTaskSourceCore _source; // mutable struct; do not make this readonly - public bool RunContinuationsAsynchronously - { - get => _core.RunContinuationsAsynchronously; - set => _core.RunContinuationsAsynchronously = value; - } + public ValueTaskSourceStatus GetStatus(short token) => _source.GetStatus(token); + public void OnCompleted(Action continuation, object? state, short token, ValueTaskSourceOnCompletedFlags flags) => _source.OnCompleted(continuation, state, token, flags); + void IValueTaskSource.GetResult(short token) => _source.GetResult(token); + int IValueTaskSource.GetResult(short token) => _source.GetResult(token); - public short Version => _core.Version; - public void Reset() => _core.Reset(); - public void SetResult(T result) => _core.SetResult(result); - public void SetException(Exception error) => _core.SetException(error); - public T GetResult(short token) => _core.GetResult(token); - void IValueTaskSource.GetResult(short token) => _core.GetResult(token); - public ValueTaskSourceStatus GetStatus(short token) => _core.GetStatus(token); - public void OnCompleted(Action continuation, object? state, short token, ValueTaskSourceOnCompletedFlags flags) => _core.OnCompleted(continuation, state, token, flags); - - public ValueTaskSourceStatus GetStatus() => _core.GetStatus(_core.Version); - } - - internal unsafe class AwaitableProvider - { 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; + internal static readonly IOCompletionCallback s_ioCallback = IOCallback; protected readonly AsyncWindowsFileStreamStrategy _strategy; protected readonly int _numBufferedBytes; protected NativeOverlapped* _overlapped; - private ManualResetValueTaskSource _source; private CancellationTokenRegistration _cancellationRegistration; private long _result; // Using long since this needs to be used in Interlocked APIs #if DEBUG private bool _cancellationHasBeenRegistered; #endif - public static AwaitableProvider Create( + public static FileStreamAwaitableProvider Create( AsyncWindowsFileStreamStrategy strategy, PreAllocatedOverlapped? preallocatedOverlapped, int numBufferedBytesRead, @@ -74,11 +55,11 @@ public static AwaitableProvider Create( return preallocatedOverlapped != null && MemoryMarshal.TryGetArray(memory, out ArraySegment buffer) && preallocatedOverlapped.IsUserObject(buffer.Array) ? - new AwaitableProvider(strategy, preallocatedOverlapped, numBufferedBytesRead, buffer.Array) : + new FileStreamAwaitableProvider(strategy, preallocatedOverlapped, numBufferedBytesRead, buffer.Array) : new MemoryAwaitableProvider(strategy, numBufferedBytesRead, memory); } - protected AwaitableProvider( + protected FileStreamAwaitableProvider( AsyncWindowsFileStreamStrategy strategy, PreAllocatedOverlapped? preallocatedOverlapped, int numBufferedBytes, @@ -89,7 +70,7 @@ protected AwaitableProvider( _result = NoResult; - _source = new ManualResetValueTaskSource(); + _source = default; // Using RunContinuationsAsynchronously for compat reasons (old API used Task.Factory.StartNew for continuations) _source.RunContinuationsAsynchronously = true; @@ -194,7 +175,7 @@ public ValueTask ReadAsync(Memory destination, CancellationToken canc // results. } - return new ValueTask(_source, _source.Version); + return new ValueTask(this, _source.Version); } public ValueTask WriteAsync(ReadOnlyMemory source, CancellationToken cancellationToken) @@ -272,7 +253,7 @@ public ValueTask WriteAsync(ReadOnlyMemory source, CancellationToken cance // results. } - return new ValueTask(_source, _source.Version); + return new ValueTask(this, _source.Version); } private static void IOCallback(uint errorCode, uint numBytes, NativeOverlapped* pOverlapped) @@ -283,11 +264,11 @@ private static void IOCallback(uint errorCode, uint numBytes, NativeOverlapped* // be directly the AwaitableProvider 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 (AsyncWindowsFileStreamStrategy or AwaitableProvider)); - AwaitableProvider provider = state switch + Debug.Assert(state is (AsyncWindowsFileStreamStrategy or FileStreamAwaitableProvider)); + FileStreamAwaitableProvider provider = state switch { AsyncWindowsFileStreamStrategy strategy => strategy._currentOverlappedOwner!, // must be owned - _ => (AwaitableProvider)state + _ => (FileStreamAwaitableProvider)state }; Debug.Assert(provider != null); Debug.Assert(provider._overlapped == pOverlapped, "Overlaps don't match"); @@ -334,7 +315,7 @@ private void CompleteCallback(ulong packedResult) int errorCode = unchecked((int)(packedResult & uint.MaxValue)); if (errorCode == Interop.Errors.ERROR_OPERATION_ABORTED) { - _source.SetException(ExceptionDispatchInfo.SetCurrentStackTrace(new OperationCanceledException(cancellationToken.IsCancellationRequested ? cancellationToken : new CancellationToken(true)))); + _source.SetException(ExceptionDispatchInfo.SetCurrentStackTrace(new OperationCanceledException(cancellationToken.IsCancellationRequested ? cancellationToken : new CancellationToken(canceled: true)))); } else { @@ -365,7 +346,7 @@ private void RegisterForCancellation(CancellationToken cancellationToken) long packedResult = Interlocked.CompareExchange(ref _result, RegisteringCancellation, NoResult); if (packedResult == NoResult) { - _cancellationRegistration = cancellationToken.UnsafeRegister(static (s, token) => ((AwaitableProvider)s!).Cancel(token), this); + _cancellationRegistration = cancellationToken.UnsafeRegister(static (s, token) => ((FileStreamAwaitableProvider)s!).Cancel(token), this); // Switch the result, just in case IO completed while we were setting the registration packedResult = Interlocked.Exchange(ref _result, NoResult); @@ -421,14 +402,15 @@ protected virtual void ReleaseNativeResource() // Only one operation at a time is eligible to use the preallocated overlapped _strategy.CompareExchangeCurrentOverlappedOwner(null, this); } + } /// - /// Extends with to support disposing of a + /// 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 MemoryAwaitableProvider : AwaitableProvider + internal sealed class MemoryAwaitableProvider : FileStreamAwaitableProvider { private MemoryHandle _handle; // mutable struct; do not make this readonly diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs index 716dbc994b1a4c..3e110f5cda287a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs @@ -4,6 +4,7 @@ using System.Diagnostics; using System.Threading; using System.Threading.Tasks; +using System.Threading.Tasks.Sources; using Microsoft.Win32.SafeHandles; namespace System.IO.Strategies @@ -11,7 +12,7 @@ namespace System.IO.Strategies internal sealed partial class AsyncWindowsFileStreamStrategy : WindowsFileStreamStrategy { private PreAllocatedOverlapped? _preallocatedOverlapped; // optimization for async ops to avoid per-op allocations - private AwaitableProvider? _currentOverlappedOwner; // async op currently using the preallocated overlapped + private FileStreamAwaitableProvider? _currentOverlappedOwner; // async op currently using the preallocated overlapped internal AsyncWindowsFileStreamStrategy(SafeFileHandle handle, FileAccess access, FileShare share) : base(handle, access, share) @@ -108,10 +109,10 @@ internal override void OnBufferAllocated(byte[] buffer) { Debug.Assert(_preallocatedOverlapped == null); - _preallocatedOverlapped = new PreAllocatedOverlapped(AwaitableProvider.s_ioCallback, this, buffer); + _preallocatedOverlapped = new PreAllocatedOverlapped(FileStreamAwaitableProvider.s_ioCallback, this, buffer); } - internal AwaitableProvider? CompareExchangeCurrentOverlappedOwner(AwaitableProvider? newSource, AwaitableProvider? existingSource) + internal FileStreamAwaitableProvider? CompareExchangeCurrentOverlappedOwner(FileStreamAwaitableProvider? newSource, FileStreamAwaitableProvider? existingSource) => Interlocked.CompareExchange(ref _currentOverlappedOwner, newSource, existingSource); public override int Read(byte[] buffer, int offset, int count) @@ -133,7 +134,7 @@ private unsafe ValueTask ReadAsyncInternal(Memory destination, Cancel Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed"); // Create and store async stream class library specific data in the async result - AwaitableProvider provider = AwaitableProvider.Create(this, _preallocatedOverlapped, 0, destination); + FileStreamAwaitableProvider provider = FileStreamAwaitableProvider.Create(this, _preallocatedOverlapped, 0, destination); return provider.ReadAsync(destination, cancellationToken); } @@ -158,7 +159,7 @@ private unsafe ValueTask WriteAsyncInternal(ReadOnlyMemory source, Cancell Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed"); // Create and store async stream class library specific data in the async result - AwaitableProvider provider = AwaitableProvider.Create(this, _preallocatedOverlapped, 0, source); + FileStreamAwaitableProvider provider = FileStreamAwaitableProvider.Create(this, _preallocatedOverlapped, 0, source); return provider.WriteAsync(source, cancellationToken); } From c586540821c22f8fe8d2586e399c30415ee569c8 Mon Sep 17 00:00:00 2001 From: carlossanlop Date: Tue, 6 Apr 2021 08:26:58 -0700 Subject: [PATCH 04/19] Bring back ReadAsync/WriteAsync code to the strategy for simpler code review. Shorter filename for awaitable provider. --- .../System.Private.CoreLib.Shared.projitems | 2 +- .../AsyncWindowsFileStreamStrategy.cs | 174 ++++++++++- ...ider.cs => FileStreamAwaitableProvider.cs} | 279 ++++-------------- 3 files changed, 223 insertions(+), 232 deletions(-) rename src/libraries/System.Private.CoreLib/src/System/IO/Strategies/{AsyncWindowsFileStreamStrategy.FileStreamAwaitableProvider.cs => FileStreamAwaitableProvider.cs} (56%) 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 962ef332f3b95a..c9cd7a7c0ed447 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 @@ -1652,7 +1652,7 @@ - + diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs index 3e110f5cda287a..b382d61bc0151a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs @@ -4,7 +4,6 @@ using System.Diagnostics; using System.Threading; using System.Threading.Tasks; -using System.Threading.Tasks.Sources; using Microsoft.Win32.SafeHandles; namespace System.IO.Strategies @@ -112,7 +111,7 @@ internal override void OnBufferAllocated(byte[] buffer) _preallocatedOverlapped = new PreAllocatedOverlapped(FileStreamAwaitableProvider.s_ioCallback, this, buffer); } - internal FileStreamAwaitableProvider? CompareExchangeCurrentOverlappedOwner(FileStreamAwaitableProvider? newSource, FileStreamAwaitableProvider? existingSource) + private FileStreamAwaitableProvider? CompareExchangeCurrentOverlappedOwner(FileStreamAwaitableProvider? newSource, FileStreamAwaitableProvider? existingSource) => Interlocked.CompareExchange(ref _currentOverlappedOwner, newSource, existingSource); public override int Read(byte[] buffer, int offset, int count) @@ -135,7 +134,100 @@ private unsafe ValueTask ReadAsyncInternal(Memory destination, Cancel // Create and store async stream class library specific data in the async result FileStreamAwaitableProvider provider = FileStreamAwaitableProvider.Create(this, _preallocatedOverlapped, 0, destination); - return provider.ReadAsync(destination, cancellationToken); + + + // Calculate position in the file we should be at after the read is done + long positionBefore = _filePosition; + if (CanSeek) + { + long len = Length; + + if (positionBefore + destination.Length > len) + { + if (positionBefore <= len) + { + destination = destination.Slice(0, (int)(len - positionBefore)); + } + 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. + provider.Overlapped->OffsetLow = unchecked((int)positionBefore); + provider.Overlapped->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, 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, false, provider.Overlapped, 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. + provider.Overlapped->InternalLow = IntPtr.Zero; + provider.ReleaseNativeResource(); + return new ValueTask(provider.NumBufferedBytes); + } + else if (errorCode != ERROR_IO_PENDING) + { + if (!_fileHandle.IsClosed && CanSeek) // Update Position - It could be anywhere. + { + _filePosition = positionBefore; + } + + provider.ReleaseNativeResource(); + + if (errorCode == ERROR_HANDLE_EOF) + { + ThrowHelper.ThrowEndOfFileException(); + } + else + { + throw Win32Marshal.GetExceptionForWin32Error(errorCode, _path); + } + } + else if (cancellationToken.CanBeCanceled) // ERROR_IO_PENDING + { + // Only once the IO is pending do we register for cancellation + provider.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 new ValueTask(provider, provider.Version); } public override void Write(byte[] buffer, int offset, int count) @@ -160,7 +252,81 @@ private unsafe ValueTask WriteAsyncInternal(ReadOnlyMemory source, Cancell // Create and store async stream class library specific data in the async result FileStreamAwaitableProvider provider = FileStreamAwaitableProvider.Create(this, _preallocatedOverlapped, 0, source); - return provider.WriteAsync(source, cancellationToken); + + long positionBefore = _filePosition; + if (CanSeek) + { + // Now set the position to read from in the NativeOverlapped struct + // For pipes, we should leave the offset fields set to 0. + provider.Overlapped->OffsetLow = (int)positionBefore; + provider.Overlapped->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, but only in memory. This isn't threadsafe. + _filePosition += source.Length; + UpdateLengthOnChangePosition(); + } + + // queue an async WriteFile operation and pass in a packed overlapped + int r = FileStreamHelpers.WriteFileNative(_fileHandle, source.Span, false, provider.Overlapped, 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. + provider.ReleaseNativeResource(); + return ValueTask.CompletedTask; + } + else if (errorCode != ERROR_IO_PENDING) + { + if (!_fileHandle.IsClosed && CanSeek) // Update Position - It could be anywhere. + { + _filePosition = positionBefore; + } + + provider.ReleaseNativeResource(); + + if (errorCode == ERROR_HANDLE_EOF) + { + ThrowHelper.ThrowEndOfFileException(); + } + else + { + throw Win32Marshal.GetExceptionForWin32Error(errorCode, _path); + } + } + else if (cancellationToken.CanBeCanceled) // ERROR_IO_PENDING + { + // Only once the IO is pending do we register for cancellation + provider.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 new ValueTask(provider, provider.Version); } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.FileStreamAwaitableProvider.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamAwaitableProvider.cs similarity index 56% rename from src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.FileStreamAwaitableProvider.cs rename to src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamAwaitableProvider.cs index 3fefb79e01d924..455dddccbcc100 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.FileStreamAwaitableProvider.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamAwaitableProvider.cs @@ -13,16 +13,10 @@ namespace System.IO.Strategies { internal sealed partial class AsyncWindowsFileStreamStrategy : WindowsFileStreamStrategy { - internal unsafe class FileStreamAwaitableProvider : IValueTaskSource, IValueTaskSource + private unsafe class FileStreamAwaitableProvider : IValueTaskSource, IValueTaskSource { private ManualResetValueTaskSourceCore _source; // mutable struct; do not make this readonly - public ValueTaskSourceStatus GetStatus(short token) => _source.GetStatus(token); - public void OnCompleted(Action continuation, object? state, short token, ValueTaskSourceOnCompletedFlags flags) => _source.OnCompleted(continuation, state, token, flags); - void IValueTaskSource.GetResult(short token) => _source.GetResult(token); - int IValueTaskSource.GetResult(short token) => _source.GetResult(token); - - private const long NoResult = 0; private const long ResultSuccess = (long)1 << 32; private const long ResultError = (long)2 << 32; @@ -31,11 +25,9 @@ internal unsafe class FileStreamAwaitableProvider : IValueTaskSource, IValu private const ulong ResultMask = ((ulong)uint.MaxValue) << 32; internal static readonly IOCompletionCallback s_ioCallback = IOCallback; - - protected readonly AsyncWindowsFileStreamStrategy _strategy; - protected readonly int _numBufferedBytes; - - protected NativeOverlapped* _overlapped; + private NativeOverlapped* _overlapped; + private readonly AsyncWindowsFileStreamStrategy _strategy; + private readonly int _numBufferedBytes; private CancellationTokenRegistration _cancellationRegistration; private long _result; // Using long since this needs to be used in Interlocked APIs #if DEBUG @@ -45,7 +37,7 @@ internal unsafe class FileStreamAwaitableProvider : IValueTaskSource, IValu public static FileStreamAwaitableProvider Create( AsyncWindowsFileStreamStrategy strategy, PreAllocatedOverlapped? preallocatedOverlapped, - int numBufferedBytesRead, + int numBufferedBytes, ReadOnlyMemory memory) { // If the memory passed in is the strategy's internal buffer, we can use the base AwaitableProvider, @@ -55,8 +47,8 @@ public static FileStreamAwaitableProvider Create( return preallocatedOverlapped != null && MemoryMarshal.TryGetArray(memory, out ArraySegment buffer) && preallocatedOverlapped.IsUserObject(buffer.Array) ? - new FileStreamAwaitableProvider(strategy, preallocatedOverlapped, numBufferedBytesRead, buffer.Array) : - new MemoryAwaitableProvider(strategy, numBufferedBytesRead, memory); + new FileStreamAwaitableProvider(strategy, preallocatedOverlapped, numBufferedBytes, buffer.Array) : + new MemoryAwaitableProvider(strategy, numBufferedBytes, memory); } protected FileStreamAwaitableProvider( @@ -82,178 +74,66 @@ protected FileStreamAwaitableProvider( Debug.Assert(_overlapped != null, "AllocateNativeOverlapped returned null"); } - public ValueTask ReadAsync(Memory destination, CancellationToken cancellationToken) - { - // Calculate position in the file we should be at after the read is done - long positionBefore = _strategy._filePosition; - if (_strategy.CanSeek) - { - long len = _strategy.Length; - - if (positionBefore + destination.Length > len) - { - if (positionBefore <= len) - { - destination = destination.Slice(0, (int)(len - positionBefore)); - } - 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. - _overlapped->OffsetLow = unchecked((int)positionBefore); - _overlapped->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, but only in memory. This isn't threadsafe. - _strategy._filePosition += destination.Length; - } + internal NativeOverlapped* Overlapped => _overlapped; + internal int NumBufferedBytes => _numBufferedBytes; + public ValueTaskSourceStatus GetStatus(short token) => _source.GetStatus(token); + public void OnCompleted(Action continuation, object? state, short token, ValueTaskSourceOnCompletedFlags flags) => _source.OnCompleted(continuation, state, token, flags); + void IValueTaskSource.GetResult(short token) => _source.GetResult(token); + int IValueTaskSource.GetResult(short token) => _source.GetResult(token); + internal short Version => _source.Version; - // queue an async ReadFile operation and pass in a packed overlapped - int r = FileStreamHelpers.ReadFileNative(_strategy._fileHandle, destination.Span, false, _overlapped, out int errorCode); + internal void RegisterForCancellation(CancellationToken cancellationToken) + { +#if DEBUG + Debug.Assert(cancellationToken.CanBeCanceled); + Debug.Assert(!_cancellationHasBeenRegistered, "Cannot register for cancellation twice"); + _cancellationHasBeenRegistered = true; +#endif - // 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) + // Quick check to make sure the IO hasn't completed + if (_overlapped != null) { - // For pipes, when they hit EOF, they will come here. - if (errorCode == ERROR_BROKEN_PIPE) + // Register the cancellation only if the IO hasn't completed + long packedResult = Interlocked.CompareExchange(ref _result, RegisteringCancellation, NoResult); + if (packedResult == NoResult) { - // Not an error, but EOF. AsyncFSCallback will NOT be - // called. Call the user callback here. + _cancellationRegistration = cancellationToken.UnsafeRegister(static (s, token) => ((FileStreamAwaitableProvider)s!).Cancel(token), this); - // We clear the overlapped status bit for this special case. - // Failure to do so looks like we are freeing a pending overlapped later. - _overlapped->InternalLow = IntPtr.Zero; - ReleaseNativeResource(); - return new ValueTask(_numBufferedBytes); + // Switch the result, just in case IO completed while we were setting the registration + packedResult = Interlocked.Exchange(ref _result, NoResult); } - else if (errorCode != ERROR_IO_PENDING) + else if (packedResult != CompletedCallback) { - if (!_strategy._fileHandle.IsClosed && _strategy.CanSeek) // Update Position - It could be anywhere. - { - _strategy._filePosition = positionBefore; - } - - ReleaseNativeResource(); - - if (errorCode == ERROR_HANDLE_EOF) - { - ThrowHelper.ThrowEndOfFileException(); - } - else - { - throw Win32Marshal.GetExceptionForWin32Error(errorCode, _strategy._path); - } + // Failed to set the result, IO is in the process of completing + // Attempt to take the packed result + packedResult = Interlocked.Exchange(ref _result, NoResult); } - else if (cancellationToken.CanBeCanceled) // ERROR_IO_PENDING + + // If we have a callback that needs to be completed + if ((packedResult != NoResult) && (packedResult != CompletedCallback) && (packedResult != RegisteringCancellation)) { - // Only once the IO is pending do we register for cancellation - RegisterForCancellation(cancellationToken); + CompleteCallback((ulong)packedResult); } } - 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 new ValueTask(this, _source.Version); } - public ValueTask WriteAsync(ReadOnlyMemory source, CancellationToken cancellationToken) + internal virtual void ReleaseNativeResource() { - long positionBefore = _strategy._filePosition; - if (_strategy.CanSeek) - { - // Now set the position to read from in the NativeOverlapped struct - // For pipes, we should leave the offset fields set to 0. - _overlapped->OffsetLow = (int)positionBefore; - _overlapped->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, but only in memory. This isn't threadsafe. - _strategy._filePosition += source.Length; - _strategy.UpdateLengthOnChangePosition(); - } - - // queue an async WriteFile operation and pass in a packed overlapped - int r = FileStreamHelpers.WriteFileNative(_strategy._fileHandle, source.Span, false, _overlapped, 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. - ReleaseNativeResource(); - return ValueTask.CompletedTask; - } - else if (errorCode != ERROR_IO_PENDING) - { - if (!_strategy._fileHandle.IsClosed && _strategy.CanSeek) // Update Position - It could be anywhere. - { - _strategy._filePosition = positionBefore; - } - - ReleaseNativeResource(); + // Ensure that cancellation has been completed and cleaned up. + _cancellationRegistration.Dispose(); - if (errorCode == ERROR_HANDLE_EOF) - { - ThrowHelper.ThrowEndOfFileException(); - } - else - { - throw Win32Marshal.GetExceptionForWin32Error(errorCode, _strategy._path); - } - } - else if (cancellationToken.CanBeCanceled) // ERROR_IO_PENDING - { - // Only once the IO is pending do we register for cancellation - RegisterForCancellation(cancellationToken); - } - } - else + // 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) { - // 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. + _strategy._fileHandle.ThreadPoolBinding!.FreeNativeOverlapped(_overlapped); + _overlapped = null; } - return new ValueTask(this, _source.Version); + // Ensure we're no longer set as the current AwaitableProvider (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); } private static void IOCallback(uint errorCode, uint numBytes, NativeOverlapped* pOverlapped) @@ -331,41 +211,6 @@ private void CompleteCallback(ulong packedResult) } } - private 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) - { - // Register the cancellation only if the IO hasn't completed - long packedResult = Interlocked.CompareExchange(ref _result, RegisteringCancellation, NoResult); - if (packedResult == NoResult) - { - _cancellationRegistration = cancellationToken.UnsafeRegister(static (s, token) => ((FileStreamAwaitableProvider)s!).Cancel(token), 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); - } - } - } - private void Cancel(CancellationToken token) { // If the handle is still valid, attempt to cancel the IO @@ -383,26 +228,6 @@ private void Cancel(CancellationToken token) } } } - - protected 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 AwaitableProvider (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); - } - } /// @@ -410,18 +235,18 @@ protected virtual void ReleaseNativeResource() /// when the operation has completed. This should only be used /// when memory doesn't wrap a byte[]. /// - internal sealed class MemoryAwaitableProvider : FileStreamAwaitableProvider + private sealed class MemoryAwaitableProvider : FileStreamAwaitableProvider { private MemoryHandle _handle; // mutable struct; do not make this readonly // this type handles the pinning, so bytes are null internal unsafe MemoryAwaitableProvider(AsyncWindowsFileStreamStrategy strategy, int numBufferedBytes, ReadOnlyMemory memory) - : base (strategy, null, numBufferedBytes, null) // this type handles the pinning, so null is passed for bytes + : base(strategy, null, numBufferedBytes, null) // this type handles the pinning, so null is passed for bytes to the base { _handle = memory.Pin(); } - protected override void ReleaseNativeResource() + internal override void ReleaseNativeResource() { _handle.Dispose(); base.ReleaseNativeResource(); From 84b2d30f9f873f1b2f8950d46ae9ba7f5b4d229d Mon Sep 17 00:00:00 2001 From: carlossanlop Date: Tue, 6 Apr 2021 08:32:20 -0700 Subject: [PATCH 05/19] FileStreamCompletionSource can now be nested in Net5CompatFileStreamStrategy, and its _fileHandle can now be private. --- .../FileStreamCompletionSource.Win32.cs | 384 +++++++++--------- .../Net5CompatFileStreamStrategy.Windows.cs | 4 +- .../Net5CompatFileStreamStrategy.cs | 2 +- 3 files changed, 196 insertions(+), 194 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamCompletionSource.Win32.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamCompletionSource.Win32.cs index 377137215e07f1..74314592acf802 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamCompletionSource.Win32.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamCompletionSource.Win32.cs @@ -6,251 +6,253 @@ using System.Runtime.InteropServices; using System.Threading; using System.Threading.Tasks; -using Microsoft.Win32.SafeHandles; namespace System.IO.Strategies { // 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 + internal sealed partial class Net5CompatFileStreamStrategy : FileStreamStrategy { - 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; + 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 const int ERROR_BROKEN_PIPE = 109; + private const int ERROR_NO_DATA = 232; - internal static readonly unsafe IOCompletionCallback s_ioCallback = IOCallback; + internal static readonly unsafe IOCompletionCallback s_ioCallback = IOCallback; - private static Action? s_cancelCallback; + private static Action? s_cancelCallback; - private readonly Net5CompatFileStreamStrategy _strategy; - private readonly int _numBufferedBytes; - private CancellationTokenRegistration _cancellationRegistration; + private readonly Net5CompatFileStreamStrategy _strategy; + private readonly int _numBufferedBytes; + private CancellationTokenRegistration _cancellationRegistration; #if DEBUG - private bool _cancellationHasBeenRegistered; + 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 + 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) - internal FileStreamCompletionSource(Net5CompatFileStreamStrategy strategy, PreAllocatedOverlapped? preallocatedOverlapped, - int numBufferedBytes, byte[]? bytes) : base(TaskCreationOptions.RunContinuationsAsynchronously) - { - _numBufferedBytes = numBufferedBytes; - _strategy = strategy; - _result = NoResult; + // Using RunContinuationsAsynchronously for compat reasons (old API used Task.Factory.StartNew for continuations) + internal FileStreamCompletionSource(Net5CompatFileStreamStrategy 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"); - } + // 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; + internal NativeOverlapped* Overlapped => _overlapped; - public void SetCompletedSynchronously(int numBytes) - { - ReleaseNativeResource(); - TrySetResult(numBytes + _numBufferedBytes); - } + public void SetCompletedSynchronously(int numBytes) + { + ReleaseNativeResource(); + TrySetResult(numBytes + _numBufferedBytes); + } - public void RegisterForCancellation(CancellationToken cancellationToken) - { + public void RegisterForCancellation(CancellationToken cancellationToken) + { #if DEBUG - Debug.Assert(cancellationToken.CanBeCanceled); - Debug.Assert(!_cancellationHasBeenRegistered, "Cannot register for cancellation twice"); - _cancellationHasBeenRegistered = true; + 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) + // Quick check to make sure the IO hasn't completed + if (_overlapped != null) { - _cancellationRegistration = cancellationToken.UnsafeRegister(cancelCallback, this); + Action? cancelCallback = s_cancelCallback ??= Cancel; - // 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); - } + // 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); - // If we have a callback that needs to be completed - if ((packedResult != NoResult) && (packedResult != CompletedCallback) && (packedResult != RegisteringCancellation)) - { - CompleteCallback((ulong)packedResult); + // 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) + internal virtual void ReleaseNativeResource() { - _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); - } + // Ensure that cancellation has been completed and cleaned up. + _cancellationRegistration.Dispose(); - // 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 Net5CompatFileStreamStrategy || state is FileStreamCompletionSource); - FileStreamCompletionSource completionSource = state switch - { - Net5CompatFileStreamStrategy strategy => strategy._currentOverlappedOwner!, // must be owned - _ => (FileStreamCompletionSource)state - }; - Debug.Assert(completionSource != null); - Debug.Assert(completionSource._overlapped == pOverlapped, "Overlaps don't match"); + // 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; + } - // 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); + // 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); } - // 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)) + // 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) { - // Successfully set the state, attempt to take back the callback - if (Interlocked.Exchange(ref completionSource._result, CompletedCallback) != NoResult) + // 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 Net5CompatFileStreamStrategy || state is FileStreamCompletionSource); + FileStreamCompletionSource completionSource = state switch + { + Net5CompatFileStreamStrategy 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) { - // Successfully got the callback, finish the callback - completionSource.CompleteCallback(packedResult); + packedResult = ((ulong)ResultError | errorCode); + } + else + { + packedResult = ((ulong)ResultSuccess | numBytes); } - // 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(); + // 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 + } - // Unpack the result and send it to the user - long result = (long)(packedResult & ResultMask); - if (result == ResultError) + private void CompleteCallback(ulong packedResult) { - int errorCode = unchecked((int)(packedResult & uint.MaxValue)); - if (errorCode == Interop.Errors.ERROR_OPERATION_ABORTED) + // 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) { - TrySetCanceled(cancellationToken.IsCancellationRequested ? cancellationToken : new CancellationToken(true)); + 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 { - Exception e = Win32Marshal.GetExceptionForWin32Error(errorCode); - e.SetCurrentStackTrace(); - TrySetException(e); + Debug.Assert(result == ResultSuccess, "Unknown result"); + TrySetResult((int)(packedResult & uint.MaxValue) + _numBufferedBytes); } } - 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)) + private static void Cancel(object? state) { - int errorCode = Marshal.GetLastWin32Error(); + // WARNING: This may potentially be called under a lock (during cancellation registration) - // 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) + 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)) { - throw Win32Marshal.GetExceptionForWin32Error(errorCode); + 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(Net5CompatFileStreamStrategy 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.IsUserObject(buffer.Array) // preallocatedOverlapped is allocated when BufferedStream|Net5CompatFileStreamStrategy allocates the buffer - ? new FileStreamCompletionSource(strategy, preallocatedOverlapped, numBufferedBytesRead, buffer.Array) - : new MemoryFileStreamCompletionSource(strategy, numBufferedBytesRead, memory); + public static FileStreamCompletionSource Create(Net5CompatFileStreamStrategy 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.IsUserObject(buffer.Array) // preallocatedOverlapped is allocated when BufferedStream|Net5CompatFileStreamStrategy 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(Net5CompatFileStreamStrategy strategy, int numBufferedBytes, ReadOnlyMemory memory) - : base(strategy, null, numBufferedBytes, null) // this type handles the pinning, so null is passed for bytes + /// + /// 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 { - _handle = memory.Pin(); - } + private MemoryHandle _handle; // mutable struct; do not make this readonly - internal override void ReleaseNativeResource() - { - _handle.Dispose(); - base.ReleaseNativeResource(); + internal MemoryFileStreamCompletionSource(Net5CompatFileStreamStrategy 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/Strategies/Net5CompatFileStreamStrategy.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.Windows.cs index 9b0849980c4813..f6052c8fce18cc 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.Windows.cs @@ -44,7 +44,7 @@ internal sealed partial class Net5CompatFileStreamStrategy : FileStreamStrategy 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 - internal FileStreamCompletionSource? _currentOverlappedOwner; // async op currently using the preallocated overlapped + private FileStreamCompletionSource? _currentOverlappedOwner; // async op currently using the preallocated overlapped private void Init(FileMode mode, FileShare share, string originalPath, FileOptions options) { @@ -547,7 +547,7 @@ partial void OnBufferAllocated() _preallocatedOverlapped = new PreAllocatedOverlapped(FileStreamCompletionSource.s_ioCallback, this, _buffer); } - internal FileStreamCompletionSource? CompareExchangeCurrentOverlappedOwner(FileStreamCompletionSource? newSource, FileStreamCompletionSource? existingSource) + private FileStreamCompletionSource? CompareExchangeCurrentOverlappedOwner(FileStreamCompletionSource? newSource, FileStreamCompletionSource? existingSource) => Interlocked.CompareExchange(ref _currentOverlappedOwner, newSource, existingSource); private void WriteSpan(ReadOnlySpan source) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.cs index ca4e5a3e0dad9f..03ca3533e717b7 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.cs @@ -14,7 +14,7 @@ internal sealed partial class Net5CompatFileStreamStrategy : FileStreamStrategy { private byte[]? _buffer; private readonly int _bufferLength; - internal readonly SafeFileHandle _fileHandle; // only ever null if ctor throws + private readonly SafeFileHandle _fileHandle; // only ever null if ctor throws /// Whether the file is opened for reading, writing, or both. private readonly FileAccess _access; From 3f842c4f3da10851cd2ae0de6179a23957658db1 Mon Sep 17 00:00:00 2001 From: carlossanlop Date: Tue, 6 Apr 2021 08:46:31 -0700 Subject: [PATCH 06/19] Remove duplicate error definitions, use centralized Interop.Errors. --- .../AsyncWindowsFileStreamStrategy.cs | 13 +++++---- .../Strategies/FileStreamAwaitableProvider.cs | 6 +++-- .../FileStreamCompletionSource.Win32.cs | 4 +-- .../Strategies/FileStreamHelpers.Windows.cs | 15 ++++------- .../Net5CompatFileStreamStrategy.Windows.cs | 27 +++++++------------ .../Strategies/WindowsFileStreamStrategy.cs | 7 ----- 6 files changed, 26 insertions(+), 46 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs index b382d61bc0151a..fdd7c840862d6e 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs @@ -181,7 +181,7 @@ private unsafe ValueTask ReadAsyncInternal(Memory destination, Cancel if (r == -1) { // For pipes, when they hit EOF, they will come here. - if (errorCode == ERROR_BROKEN_PIPE) + if (errorCode == Interop.Errors.ERROR_BROKEN_PIPE) { // Not an error, but EOF. AsyncFSCallback will NOT be // called. Call the user callback here. @@ -192,7 +192,7 @@ private unsafe ValueTask ReadAsyncInternal(Memory destination, Cancel provider.ReleaseNativeResource(); return new ValueTask(provider.NumBufferedBytes); } - else if (errorCode != ERROR_IO_PENDING) + else if (errorCode != Interop.Errors.ERROR_IO_PENDING) { if (!_fileHandle.IsClosed && CanSeek) // Update Position - It could be anywhere. { @@ -201,7 +201,7 @@ private unsafe ValueTask ReadAsyncInternal(Memory destination, Cancel provider.ReleaseNativeResource(); - if (errorCode == ERROR_HANDLE_EOF) + if (errorCode == Interop.Errors.ERROR_HANDLE_EOF) { ThrowHelper.ThrowEndOfFileException(); } @@ -284,14 +284,14 @@ private unsafe ValueTask WriteAsyncInternal(ReadOnlyMemory source, Cancell if (r == -1) { // For pipes, when they are closed on the other side, they will come here. - if (errorCode == ERROR_NO_DATA) + if (errorCode == Interop.Errors.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. provider.ReleaseNativeResource(); return ValueTask.CompletedTask; } - else if (errorCode != ERROR_IO_PENDING) + else if (errorCode != Interop.Errors.ERROR_IO_PENDING) { if (!_fileHandle.IsClosed && CanSeek) // Update Position - It could be anywhere. { @@ -300,7 +300,7 @@ private unsafe ValueTask WriteAsyncInternal(ReadOnlyMemory source, Cancell provider.ReleaseNativeResource(); - if (errorCode == ERROR_HANDLE_EOF) + if (errorCode == Interop.Errors.ERROR_HANDLE_EOF) { ThrowHelper.ThrowEndOfFileException(); } @@ -329,7 +329,6 @@ private unsafe ValueTask WriteAsyncInternal(ReadOnlyMemory source, Cancell return new ValueTask(provider, provider.Version); } - public override Task CopyToAsync(Stream destination, int bufferSize, CancellationToken cancellationToken) { ValidateCopyToArguments(destination, bufferSize); diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamAwaitableProvider.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamAwaitableProvider.cs index 455dddccbcc100..8a3309a109c722 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamAwaitableProvider.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamAwaitableProvider.cs @@ -6,13 +6,15 @@ using System.Runtime.ExceptionServices; using System.Runtime.InteropServices; using System.Threading; -using System.Threading.Tasks; using System.Threading.Tasks.Sources; namespace System.IO.Strategies { internal sealed partial class AsyncWindowsFileStreamStrategy : WindowsFileStreamStrategy { + /// + /// Type that ensures we return a ValueTask when asynchronous work is scheduled when reading/writing inside AsyncWindowsFileStreamStrategy. + /// private unsafe class FileStreamAwaitableProvider : IValueTaskSource, IValueTaskSource { private ManualResetValueTaskSourceCore _source; // mutable struct; do not make this readonly @@ -158,7 +160,7 @@ private static void IOCallback(uint errorCode, uint numBytes, NativeOverlapped* // 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) + if (errorCode != 0 && errorCode != Interop.Errors.ERROR_BROKEN_PIPE && errorCode != Interop.Errors.ERROR_NO_DATA) { packedResult = ((ulong)ResultError | errorCode); } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamCompletionSource.Win32.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamCompletionSource.Win32.cs index 74314592acf802..48301c70ec74ed 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamCompletionSource.Win32.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamCompletionSource.Win32.cs @@ -22,8 +22,6 @@ private unsafe class FileStreamCompletionSource : TaskCompletionSource 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; @@ -143,7 +141,7 @@ internal static void IOCallback(uint errorCode, uint numBytes, NativeOverlapped* // 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) + if (errorCode != 0 && errorCode != Interop.Errors.ERROR_BROKEN_PIPE && errorCode != Interop.Errors.ERROR_NO_DATA) { packedResult = ((ulong)ResultError | errorCode); } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Windows.cs index fd3218f6eb4c3e..725a4c9539afed 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Windows.cs @@ -14,11 +14,6 @@ namespace System.IO.Strategies // this type defines a set of stateless FileStream/FileStreamStrategy helper methods internal static partial 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; - private static FileStreamStrategy ChooseStrategyCore(SafeFileHandle handle, FileAccess access, FileShare share, int bufferSize, bool isAsync) { if (UseNet5CompatStrategy) @@ -481,11 +476,11 @@ internal static async Task AsyncModeCopyToAsync(SafeFileHandle handle, string? p { switch (errorCode) { - case ERROR_IO_PENDING: + case Interop.Errors.ERROR_IO_PENDING: // Async operation in progress. break; - case ERROR_BROKEN_PIPE: - case ERROR_HANDLE_EOF: + case Interop.Errors.ERROR_BROKEN_PIPE: + case Interop.Errors.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. @@ -503,8 +498,8 @@ internal static async Task AsyncModeCopyToAsync(SafeFileHandle handle, string? p { 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) + case Interop.Errors.ERROR_BROKEN_PIPE: // logically success with 0 bytes read (write end of pipe closed) + case Interop.Errors.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 diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.Windows.cs index f6052c8fce18cc..67b639a6829a02 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.Windows.cs @@ -427,13 +427,13 @@ private unsafe int ReadNative(Span buffer) if (r == -1) { // For pipes, ERROR_BROKEN_PIPE is the normal end of the pipe. - if (errorCode == ERROR_BROKEN_PIPE) + if (errorCode == Interop.Errors.ERROR_BROKEN_PIPE) { r = 0; } else { - if (errorCode == ERROR_INVALID_PARAMETER) + if (errorCode == Interop.Errors.ERROR_INVALID_PARAMETER) ThrowHelper.ThrowArgumentException_HandleNotSync(nameof(_fileHandle)); throw Win32Marshal.GetExceptionForWin32Error(errorCode, _path); @@ -629,7 +629,7 @@ private unsafe void WriteCore(ReadOnlySpan source) if (r == -1) { // For pipes, ERROR_NO_DATA is not an error, but the pipe is closing. - if (errorCode == ERROR_NO_DATA) + if (errorCode == Interop.Errors.ERROR_NO_DATA) { r = 0; } @@ -638,7 +638,7 @@ private unsafe void WriteCore(ReadOnlySpan source) // 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) + if (errorCode == Interop.Errors.ERROR_INVALID_PARAMETER) throw new IOException(SR.IO_FileTooLongOrHandleNotSync); throw Win32Marshal.GetExceptionForWin32Error(errorCode, _path); } @@ -813,7 +813,7 @@ private unsafe Task ReadNativeAsync(Memory destination, int numBuffer if (r == -1) { // For pipes, when they hit EOF, they will come here. - if (errorCode == ERROR_BROKEN_PIPE) + if (errorCode == Interop.Errors.ERROR_BROKEN_PIPE) { // Not an error, but EOF. AsyncFSCallback will NOT be // called. Call the user callback here. @@ -823,7 +823,7 @@ private unsafe Task ReadNativeAsync(Memory destination, int numBuffer intOverlapped->InternalLow = IntPtr.Zero; completionSource.SetCompletedSynchronously(0); } - else if (errorCode != ERROR_IO_PENDING) + else if (errorCode != Interop.Errors.ERROR_IO_PENDING) { if (!_fileHandle.IsClosed && CanSeek) // Update Position - It could be anywhere. { @@ -832,7 +832,7 @@ private unsafe Task ReadNativeAsync(Memory destination, int numBuffer completionSource.ReleaseNativeResource(); - if (errorCode == ERROR_HANDLE_EOF) + if (errorCode == Interop.Errors.ERROR_HANDLE_EOF) { ThrowHelper.ThrowEndOfFileException(); } @@ -1018,14 +1018,14 @@ private unsafe Task WriteAsyncInternalCore(ReadOnlyMemory source, Cancella if (r == -1) { // For pipes, when they are closed on the other side, they will come here. - if (errorCode == ERROR_NO_DATA) + if (errorCode == Interop.Errors.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) + else if (errorCode != Interop.Errors.ERROR_IO_PENDING) { if (!_fileHandle.IsClosed && CanSeek) // Update Position - It could be anywhere. { @@ -1034,7 +1034,7 @@ private unsafe Task WriteAsyncInternalCore(ReadOnlyMemory source, Cancella completionSource.ReleaseNativeResource(); - if (errorCode == ERROR_HANDLE_EOF) + if (errorCode == Interop.Errors.ERROR_HANDLE_EOF) { ThrowHelper.ThrowEndOfFileException(); } @@ -1063,13 +1063,6 @@ private unsafe Task WriteAsyncInternalCore(ReadOnlyMemory source, Cancella return completionSource.Task; } - // Error codes (not HRESULTS), from winerror.h - 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_INVALID_PARAMETER = 87; - private const int ERROR_IO_PENDING = 997; - // __ConsoleStream also uses this code. private unsafe int ReadFileNative(SafeFileHandle handle, Span bytes, NativeOverlapped* overlapped, out int errorCode) { diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs index d42494c19086d2..94686aa9c05326 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs @@ -10,13 +10,6 @@ namespace System.IO.Strategies // 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 protected readonly string? _path; // The path to the opened file. private readonly FileAccess _access; // What file was opened for. From 302e2fd1555ed6e10f45008e06be2e356e6b8d79 Mon Sep 17 00:00:00 2001 From: carlossanlop Date: Tue, 6 Apr 2021 08:49:51 -0700 Subject: [PATCH 07/19] Move shared mask values to FileStreamHelpers to avoid duplication. --- .../Strategies/FileStreamAwaitableProvider.cs | 35 ++++++++----------- .../FileStreamCompletionSource.Win32.cs | 35 ++++++++----------- .../Strategies/FileStreamHelpers.Windows.cs | 7 ++++ 3 files changed, 35 insertions(+), 42 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamAwaitableProvider.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamAwaitableProvider.cs index 8a3309a109c722..6a329ec89b3533 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamAwaitableProvider.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamAwaitableProvider.cs @@ -19,13 +19,6 @@ private unsafe class FileStreamAwaitableProvider : IValueTaskSource, IValue { private ManualResetValueTaskSourceCore _source; // mutable struct; do not make this readonly - 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; - internal static readonly IOCompletionCallback s_ioCallback = IOCallback; private NativeOverlapped* _overlapped; private readonly AsyncWindowsFileStreamStrategy _strategy; @@ -62,7 +55,7 @@ protected FileStreamAwaitableProvider( _strategy = strategy; _numBufferedBytes = numBufferedBytes; - _result = NoResult; + _result = FileStreamHelpers.NoResult; _source = default; // Using RunContinuationsAsynchronously for compat reasons (old API used Task.Factory.StartNew for continuations) @@ -96,23 +89,23 @@ internal void RegisterForCancellation(CancellationToken cancellationToken) if (_overlapped != null) { // Register the cancellation only if the IO hasn't completed - long packedResult = Interlocked.CompareExchange(ref _result, RegisteringCancellation, NoResult); - if (packedResult == NoResult) + long packedResult = Interlocked.CompareExchange(ref _result, FileStreamHelpers.RegisteringCancellation, FileStreamHelpers.NoResult); + if (packedResult == FileStreamHelpers.NoResult) { _cancellationRegistration = cancellationToken.UnsafeRegister(static (s, token) => ((FileStreamAwaitableProvider)s!).Cancel(token), this); // Switch the result, just in case IO completed while we were setting the registration - packedResult = Interlocked.Exchange(ref _result, NoResult); + packedResult = Interlocked.Exchange(ref _result, FileStreamHelpers.NoResult); } - else if (packedResult != CompletedCallback) + else if (packedResult != FileStreamHelpers.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); + packedResult = Interlocked.Exchange(ref _result, FileStreamHelpers.NoResult); } // If we have a callback that needs to be completed - if ((packedResult != NoResult) && (packedResult != CompletedCallback) && (packedResult != RegisteringCancellation)) + if ((packedResult != FileStreamHelpers.NoResult) && (packedResult != FileStreamHelpers.CompletedCallback) && (packedResult != FileStreamHelpers.RegisteringCancellation)) { CompleteCallback((ulong)packedResult); } @@ -162,19 +155,19 @@ private static void IOCallback(uint errorCode, uint numBytes, NativeOverlapped* ulong packedResult; if (errorCode != 0 && errorCode != Interop.Errors.ERROR_BROKEN_PIPE && errorCode != Interop.Errors.ERROR_NO_DATA) { - packedResult = ((ulong)ResultError | errorCode); + packedResult = ((ulong)FileStreamHelpers.ResultError | errorCode); } else { - packedResult = ((ulong)ResultSuccess | numBytes); + packedResult = ((ulong)FileStreamHelpers.ResultSuccess | numBytes); } // Stow the result so that other threads can observe it // And, if no other thread is registering cancellation, continue - if (Interlocked.Exchange(ref provider._result, (long)packedResult) == NoResult) + if (Interlocked.Exchange(ref provider._result, (long)packedResult) == FileStreamHelpers.NoResult) { // Successfully set the state, attempt to take back the callback - if (Interlocked.Exchange(ref provider._result, CompletedCallback) != NoResult) + if (Interlocked.Exchange(ref provider._result, FileStreamHelpers.CompletedCallback) != FileStreamHelpers.NoResult) { // Successfully got the callback, finish the callback provider.CompleteCallback(packedResult); @@ -191,8 +184,8 @@ private void CompleteCallback(ulong packedResult) ReleaseNativeResource(); // Unpack the result and send it to the user - long result = (long)(packedResult & ResultMask); - if (result == ResultError) + long result = (long)(packedResult & FileStreamHelpers.ResultMask); + if (result == FileStreamHelpers.ResultError) { int errorCode = unchecked((int)(packedResult & uint.MaxValue)); if (errorCode == Interop.Errors.ERROR_OPERATION_ABORTED) @@ -208,7 +201,7 @@ private void CompleteCallback(ulong packedResult) } else { - Debug.Assert(result == ResultSuccess, "Unknown result"); + Debug.Assert(result == FileStreamHelpers.ResultSuccess, "Unknown result"); _source.SetResult((int)(packedResult & uint.MaxValue) + _numBufferedBytes); } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamCompletionSource.Win32.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamCompletionSource.Win32.cs index 48301c70ec74ed..ae10794982cb56 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamCompletionSource.Win32.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamCompletionSource.Win32.cs @@ -16,13 +16,6 @@ internal sealed partial class Net5CompatFileStreamStrategy : FileStreamStrategy { 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; - internal static readonly unsafe IOCompletionCallback s_ioCallback = IOCallback; private static Action? s_cancelCallback; @@ -42,7 +35,7 @@ internal FileStreamCompletionSource(Net5CompatFileStreamStrategy strategy, PreAl { _numBufferedBytes = numBufferedBytes; _strategy = strategy; - _result = NoResult; + _result = FileStreamHelpers.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 @@ -74,23 +67,23 @@ public void RegisterForCancellation(CancellationToken cancellationToken) 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) + long packedResult = Interlocked.CompareExchange(ref _result, FileStreamHelpers.RegisteringCancellation, FileStreamHelpers.NoResult); + if (packedResult == FileStreamHelpers.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); + packedResult = Interlocked.Exchange(ref _result, FileStreamHelpers.NoResult); } - else if (packedResult != CompletedCallback) + else if (packedResult != FileStreamHelpers.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); + packedResult = Interlocked.Exchange(ref _result, FileStreamHelpers.NoResult); } // If we have a callback that needs to be completed - if ((packedResult != NoResult) && (packedResult != CompletedCallback) && (packedResult != RegisteringCancellation)) + if ((packedResult != FileStreamHelpers.NoResult) && (packedResult != FileStreamHelpers.CompletedCallback) && (packedResult != FileStreamHelpers.RegisteringCancellation)) { CompleteCallback((ulong)packedResult); } @@ -143,19 +136,19 @@ internal static void IOCallback(uint errorCode, uint numBytes, NativeOverlapped* ulong packedResult; if (errorCode != 0 && errorCode != Interop.Errors.ERROR_BROKEN_PIPE && errorCode != Interop.Errors.ERROR_NO_DATA) { - packedResult = ((ulong)ResultError | errorCode); + packedResult = ((ulong)FileStreamHelpers.ResultError | errorCode); } else { - packedResult = ((ulong)ResultSuccess | numBytes); + packedResult = ((ulong)FileStreamHelpers.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)) + if (FileStreamHelpers.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) + if (Interlocked.Exchange(ref completionSource._result, FileStreamHelpers.CompletedCallback) != FileStreamHelpers.NoResult) { // Successfully got the callback, finish the callback completionSource.CompleteCallback(packedResult); @@ -172,8 +165,8 @@ private void CompleteCallback(ulong packedResult) ReleaseNativeResource(); // Unpack the result and send it to the user - long result = (long)(packedResult & ResultMask); - if (result == ResultError) + long result = (long)(packedResult & FileStreamHelpers.ResultMask); + if (result == FileStreamHelpers.ResultError) { int errorCode = unchecked((int)(packedResult & uint.MaxValue)); if (errorCode == Interop.Errors.ERROR_OPERATION_ABORTED) @@ -189,7 +182,7 @@ private void CompleteCallback(ulong packedResult) } else { - Debug.Assert(result == ResultSuccess, "Unknown result"); + Debug.Assert(result == FileStreamHelpers.ResultSuccess, "Unknown result"); TrySetResult((int)(packedResult & uint.MaxValue) + _numBufferedBytes); } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Windows.cs index 725a4c9539afed..23d86c3b8c3209 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Windows.cs @@ -14,6 +14,13 @@ namespace System.IO.Strategies // this type defines a set of stateless FileStream/FileStreamStrategy helper methods internal static partial class FileStreamHelpers { + internal const long NoResult = 0; + internal const long ResultSuccess = (long)1 << 32; + internal const long ResultError = (long)2 << 32; + internal const long RegisteringCancellation = (long)4 << 32; + internal const long CompletedCallback = (long)8 << 32; + internal const ulong ResultMask = ((ulong)uint.MaxValue) << 32; + private static FileStreamStrategy ChooseStrategyCore(SafeFileHandle handle, FileAccess access, FileShare share, int bufferSize, bool isAsync) { if (UseNet5CompatStrategy) From f0af5d7e421d70b0cdd0797864d8bd22d4de5169 Mon Sep 17 00:00:00 2001 From: carlossanlop Date: Tue, 6 Apr 2021 08:52:09 -0700 Subject: [PATCH 08/19] Use Interop.Errors also in SyncWindowsFileStreamStrategy. --- .../System/IO/Strategies/SyncWindowsFileStreamStrategy.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/SyncWindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/SyncWindowsFileStreamStrategy.cs index 3639b4b5fb4daf..ad96cdb4967b44 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/SyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/SyncWindowsFileStreamStrategy.cs @@ -110,13 +110,13 @@ private unsafe int ReadSpan(Span destination) if (r == -1) { // For pipes, ERROR_BROKEN_PIPE is the normal end of the pipe. - if (errorCode == ERROR_BROKEN_PIPE) + if (errorCode == Interop.Errors.ERROR_BROKEN_PIPE) { r = 0; } else { - if (errorCode == ERROR_INVALID_PARAMETER) + if (errorCode == Interop.Errors.ERROR_INVALID_PARAMETER) ThrowHelper.ThrowArgumentException_HandleNotSync(nameof(_fileHandle)); throw Win32Marshal.GetExceptionForWin32Error(errorCode, _path); @@ -143,7 +143,7 @@ private unsafe void WriteSpan(ReadOnlySpan source) if (r == -1) { // For pipes, ERROR_NO_DATA is not an error, but the pipe is closing. - if (errorCode == ERROR_NO_DATA) + if (errorCode == Interop.Errors.ERROR_NO_DATA) { r = 0; } @@ -152,7 +152,7 @@ private unsafe void WriteSpan(ReadOnlySpan source) // 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) + if (errorCode == Interop.Errors.ERROR_INVALID_PARAMETER) throw new IOException(SR.IO_FileTooLongOrHandleNotSync); throw Win32Marshal.GetExceptionForWin32Error(errorCode, _path); } From 47cb5536c989c77889311fd47c082105a481ac68 Mon Sep 17 00:00:00 2001 From: carlossanlop Date: Tue, 6 Apr 2021 09:00:46 -0700 Subject: [PATCH 09/19] Move misplaced comment, move method location. --- .../System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs | 4 ++-- .../IO/Strategies/FileStreamCompletionSource.Win32.cs | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs index fdd7c840862d6e..cabbbc88057557 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs @@ -239,8 +239,6 @@ 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 unsafe ValueTask WriteAsyncInternal(ReadOnlyMemory source, CancellationToken cancellationToken) { if (!CanWrite) @@ -329,6 +327,8 @@ private unsafe ValueTask WriteAsyncInternal(ReadOnlyMemory source, Cancell return new ValueTask(provider, provider.Version); } + public override Task FlushAsync(CancellationToken cancellationToken) => Task.CompletedTask; // no buffering = nothing to flush + public override Task CopyToAsync(Stream destination, int bufferSize, CancellationToken cancellationToken) { ValidateCopyToArguments(destination, bufferSize); diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamCompletionSource.Win32.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamCompletionSource.Win32.cs index ae10794982cb56..bd60664682b9f0 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamCompletionSource.Win32.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamCompletionSource.Win32.cs @@ -9,11 +9,11 @@ namespace System.IO.Strategies { - // 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 sealed partial class Net5CompatFileStreamStrategy : 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 { internal static readonly unsafe IOCompletionCallback s_ioCallback = IOCallback; From dc2fb7ef55a4f73c9786a8efd557c0b0731f634f Mon Sep 17 00:00:00 2001 From: carlossanlop Date: Tue, 6 Apr 2021 10:16:27 -0700 Subject: [PATCH 10/19] Slightly better comment. --- .../src/System/IO/Strategies/FileStreamAwaitableProvider.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamAwaitableProvider.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamAwaitableProvider.cs index 6a329ec89b3533..30f3b1be80972f 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamAwaitableProvider.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamAwaitableProvider.cs @@ -13,7 +13,7 @@ namespace System.IO.Strategies internal sealed partial class AsyncWindowsFileStreamStrategy : WindowsFileStreamStrategy { /// - /// Type that ensures we return a ValueTask when asynchronous work is scheduled when reading/writing inside AsyncWindowsFileStreamStrategy. + /// Type that helps reduce allocations for FileStream.ReadAsync and FileStream.WriteAsync. /// private unsafe class FileStreamAwaitableProvider : IValueTaskSource, IValueTaskSource { From 21fe9af29bd45399ae441f0b6bde177ea2a22d30 Mon Sep 17 00:00:00 2001 From: carlossanlop Date: Tue, 6 Apr 2021 11:32:39 -0700 Subject: [PATCH 11/19] Rename FileStreamAwaitableProvider to FileStreamValueTaskSource to keep in sync with the name of FileStreamCompletionSource. --- .../System.Private.CoreLib.Shared.projitems | 2 +- .../AsyncWindowsFileStreamStrategy.cs | 42 +++++++++---------- ...ovider.cs => FileStreamValueTaskSource.cs} | 37 ++++++++-------- 3 files changed, 41 insertions(+), 40 deletions(-) rename src/libraries/System.Private.CoreLib/src/System/IO/Strategies/{FileStreamAwaitableProvider.cs => FileStreamValueTaskSource.cs} (91%) 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 c9cd7a7c0ed447..85dd8331ebf121 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 @@ -1652,7 +1652,7 @@ - + diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs index cabbbc88057557..0ac1813e45be96 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs @@ -11,7 +11,7 @@ namespace System.IO.Strategies internal sealed partial class AsyncWindowsFileStreamStrategy : WindowsFileStreamStrategy { private PreAllocatedOverlapped? _preallocatedOverlapped; // optimization for async ops to avoid per-op allocations - private FileStreamAwaitableProvider? _currentOverlappedOwner; // async op currently using the preallocated overlapped + private FileStreamValueTaskSource? _currentOverlappedOwner; // async op currently using the preallocated overlapped internal AsyncWindowsFileStreamStrategy(SafeFileHandle handle, FileAccess access, FileShare share) : base(handle, access, share) @@ -108,10 +108,10 @@ internal override void OnBufferAllocated(byte[] buffer) { Debug.Assert(_preallocatedOverlapped == null); - _preallocatedOverlapped = new PreAllocatedOverlapped(FileStreamAwaitableProvider.s_ioCallback, this, buffer); + _preallocatedOverlapped = new PreAllocatedOverlapped(FileStreamValueTaskSource.s_ioCallback, this, buffer); } - private FileStreamAwaitableProvider? CompareExchangeCurrentOverlappedOwner(FileStreamAwaitableProvider? newSource, FileStreamAwaitableProvider? existingSource) + private FileStreamValueTaskSource? CompareExchangeCurrentOverlappedOwner(FileStreamValueTaskSource? newSource, FileStreamValueTaskSource? existingSource) => Interlocked.CompareExchange(ref _currentOverlappedOwner, newSource, existingSource); public override int Read(byte[] buffer, int offset, int count) @@ -133,7 +133,7 @@ private unsafe ValueTask ReadAsyncInternal(Memory destination, Cancel Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed"); // Create and store async stream class library specific data in the async result - FileStreamAwaitableProvider provider = FileStreamAwaitableProvider.Create(this, _preallocatedOverlapped, 0, destination); + FileStreamValueTaskSource valueTaskSource = FileStreamValueTaskSource.Create(this, _preallocatedOverlapped, 0, destination); // Calculate position in the file we should be at after the read is done @@ -156,8 +156,8 @@ private unsafe ValueTask ReadAsyncInternal(Memory destination, Cancel // Now set the position to read from in the NativeOverlapped struct // For pipes, we should leave the offset fields set to 0. - provider.Overlapped->OffsetLow = unchecked((int)positionBefore); - provider.Overlapped->OffsetHigh = (int)(positionBefore >> 32); + valueTaskSource.Overlapped->OffsetLow = unchecked((int)positionBefore); + valueTaskSource.Overlapped->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 @@ -166,7 +166,7 @@ private unsafe ValueTask ReadAsyncInternal(Memory destination, Cancel } // queue an async ReadFile operation and pass in a packed overlapped - int r = FileStreamHelpers.ReadFileNative(_fileHandle, destination.Span, false, provider.Overlapped, out int errorCode); + int r = FileStreamHelpers.ReadFileNative(_fileHandle, destination.Span, false, valueTaskSource.Overlapped, out int errorCode); // ReadFile, the OS version, will return 0 on failure. But // my ReadFileNative wrapper returns -1. My wrapper will return @@ -188,9 +188,9 @@ private unsafe ValueTask ReadAsyncInternal(Memory destination, Cancel // We clear the overlapped status bit for this special case. // Failure to do so looks like we are freeing a pending overlapped later. - provider.Overlapped->InternalLow = IntPtr.Zero; - provider.ReleaseNativeResource(); - return new ValueTask(provider.NumBufferedBytes); + valueTaskSource.Overlapped->InternalLow = IntPtr.Zero; + valueTaskSource.ReleaseNativeResource(); + return new ValueTask(valueTaskSource.NumBufferedBytes); } else if (errorCode != Interop.Errors.ERROR_IO_PENDING) { @@ -199,7 +199,7 @@ private unsafe ValueTask ReadAsyncInternal(Memory destination, Cancel _filePosition = positionBefore; } - provider.ReleaseNativeResource(); + valueTaskSource.ReleaseNativeResource(); if (errorCode == Interop.Errors.ERROR_HANDLE_EOF) { @@ -213,7 +213,7 @@ private unsafe ValueTask ReadAsyncInternal(Memory destination, Cancel else if (cancellationToken.CanBeCanceled) // ERROR_IO_PENDING { // Only once the IO is pending do we register for cancellation - provider.RegisterForCancellation(cancellationToken); + valueTaskSource.RegisterForCancellation(cancellationToken); } } else @@ -227,7 +227,7 @@ private unsafe ValueTask ReadAsyncInternal(Memory destination, Cancel // results. } - return new ValueTask(provider, provider.Version); + return new ValueTask(valueTaskSource, valueTaskSource.Version); } public override void Write(byte[] buffer, int offset, int count) @@ -249,15 +249,15 @@ private unsafe ValueTask WriteAsyncInternal(ReadOnlyMemory source, Cancell Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed"); // Create and store async stream class library specific data in the async result - FileStreamAwaitableProvider provider = FileStreamAwaitableProvider.Create(this, _preallocatedOverlapped, 0, source); + FileStreamValueTaskSource valueTaskSource = FileStreamValueTaskSource.Create(this, _preallocatedOverlapped, 0, source); long positionBefore = _filePosition; if (CanSeek) { // Now set the position to read from in the NativeOverlapped struct // For pipes, we should leave the offset fields set to 0. - provider.Overlapped->OffsetLow = (int)positionBefore; - provider.Overlapped->OffsetHigh = (int)(positionBefore >> 32); + valueTaskSource.Overlapped->OffsetLow = (int)positionBefore; + valueTaskSource.Overlapped->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 @@ -267,7 +267,7 @@ private unsafe ValueTask WriteAsyncInternal(ReadOnlyMemory source, Cancell } // queue an async WriteFile operation and pass in a packed overlapped - int r = FileStreamHelpers.WriteFileNative(_fileHandle, source.Span, false, provider.Overlapped, out int errorCode); + int r = FileStreamHelpers.WriteFileNative(_fileHandle, source.Span, false, valueTaskSource.Overlapped, out int errorCode); // WriteFile, the OS version, will return 0 on failure. But // my WriteFileNative wrapper returns -1. My wrapper will return @@ -286,7 +286,7 @@ private unsafe ValueTask WriteAsyncInternal(ReadOnlyMemory source, Cancell { // Not an error, but EOF. AsyncFSCallback will NOT be called. // Completing TCS and return cached task allowing the GC to collect TCS. - provider.ReleaseNativeResource(); + valueTaskSource.ReleaseNativeResource(); return ValueTask.CompletedTask; } else if (errorCode != Interop.Errors.ERROR_IO_PENDING) @@ -296,7 +296,7 @@ private unsafe ValueTask WriteAsyncInternal(ReadOnlyMemory source, Cancell _filePosition = positionBefore; } - provider.ReleaseNativeResource(); + valueTaskSource.ReleaseNativeResource(); if (errorCode == Interop.Errors.ERROR_HANDLE_EOF) { @@ -310,7 +310,7 @@ private unsafe ValueTask WriteAsyncInternal(ReadOnlyMemory source, Cancell else if (cancellationToken.CanBeCanceled) // ERROR_IO_PENDING { // Only once the IO is pending do we register for cancellation - provider.RegisterForCancellation(cancellationToken); + valueTaskSource.RegisterForCancellation(cancellationToken); } } else @@ -324,7 +324,7 @@ private unsafe ValueTask WriteAsyncInternal(ReadOnlyMemory source, Cancell // results. } - return new ValueTask(provider, provider.Version); + return new ValueTask(valueTaskSource, valueTaskSource.Version); } public override Task FlushAsync(CancellationToken cancellationToken) => Task.CompletedTask; // no buffering = nothing to flush diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamAwaitableProvider.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamValueTaskSource.cs similarity index 91% rename from src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamAwaitableProvider.cs rename to src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamValueTaskSource.cs index 30f3b1be80972f..cbc225b85ac8e9 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamAwaitableProvider.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamValueTaskSource.cs @@ -15,21 +15,22 @@ internal sealed partial class AsyncWindowsFileStreamStrategy : WindowsFileStream /// /// Type that helps reduce allocations for FileStream.ReadAsync and FileStream.WriteAsync. /// - private unsafe class FileStreamAwaitableProvider : IValueTaskSource, IValueTaskSource + private unsafe class FileStreamValueTaskSource : IValueTaskSource, IValueTaskSource { - private ManualResetValueTaskSourceCore _source; // mutable struct; do not make this readonly - internal static readonly IOCompletionCallback s_ioCallback = IOCallback; - private NativeOverlapped* _overlapped; + private readonly AsyncWindowsFileStreamStrategy _strategy; private readonly int _numBufferedBytes; + + private ManualResetValueTaskSourceCore _source; // mutable struct; do not make this readonly + private NativeOverlapped* _overlapped; private CancellationTokenRegistration _cancellationRegistration; private long _result; // Using long since this needs to be used in Interlocked APIs #if DEBUG private bool _cancellationHasBeenRegistered; #endif - public static FileStreamAwaitableProvider Create( + public static FileStreamValueTaskSource Create( AsyncWindowsFileStreamStrategy strategy, PreAllocatedOverlapped? preallocatedOverlapped, int numBufferedBytes, @@ -42,11 +43,11 @@ public static FileStreamAwaitableProvider Create( return preallocatedOverlapped != null && MemoryMarshal.TryGetArray(memory, out ArraySegment buffer) && preallocatedOverlapped.IsUserObject(buffer.Array) ? - new FileStreamAwaitableProvider(strategy, preallocatedOverlapped, numBufferedBytes, buffer.Array) : + new FileStreamValueTaskSource(strategy, preallocatedOverlapped, numBufferedBytes, buffer.Array) : new MemoryAwaitableProvider(strategy, numBufferedBytes, memory); } - protected FileStreamAwaitableProvider( + protected FileStreamValueTaskSource( AsyncWindowsFileStreamStrategy strategy, PreAllocatedOverlapped? preallocatedOverlapped, int numBufferedBytes, @@ -92,7 +93,7 @@ internal void RegisterForCancellation(CancellationToken cancellationToken) long packedResult = Interlocked.CompareExchange(ref _result, FileStreamHelpers.RegisteringCancellation, FileStreamHelpers.NoResult); if (packedResult == FileStreamHelpers.NoResult) { - _cancellationRegistration = cancellationToken.UnsafeRegister(static (s, token) => ((FileStreamAwaitableProvider)s!).Cancel(token), this); + _cancellationRegistration = cancellationToken.UnsafeRegister(static (s, token) => ((FileStreamValueTaskSource)s!).Cancel(token), this); // Switch the result, just in case IO completed while we were setting the registration packedResult = Interlocked.Exchange(ref _result, FileStreamHelpers.NoResult); @@ -139,14 +140,14 @@ private static void IOCallback(uint errorCode, uint numBytes, NativeOverlapped* // be directly the AwaitableProvider 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 (AsyncWindowsFileStreamStrategy or FileStreamAwaitableProvider)); - FileStreamAwaitableProvider provider = state switch + Debug.Assert(state is (AsyncWindowsFileStreamStrategy or FileStreamValueTaskSource)); + FileStreamValueTaskSource valueTaskSource = state switch { AsyncWindowsFileStreamStrategy strategy => strategy._currentOverlappedOwner!, // must be owned - _ => (FileStreamAwaitableProvider)state + _ => (FileStreamValueTaskSource)state }; - Debug.Assert(provider != null); - Debug.Assert(provider._overlapped == pOverlapped, "Overlaps don't match"); + Debug.Assert(valueTaskSource != null); + Debug.Assert(valueTaskSource._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 @@ -164,13 +165,13 @@ private static void IOCallback(uint errorCode, uint numBytes, NativeOverlapped* // Stow the result so that other threads can observe it // And, if no other thread is registering cancellation, continue - if (Interlocked.Exchange(ref provider._result, (long)packedResult) == FileStreamHelpers.NoResult) + if (Interlocked.Exchange(ref valueTaskSource._result, (long)packedResult) == FileStreamHelpers.NoResult) { // Successfully set the state, attempt to take back the callback - if (Interlocked.Exchange(ref provider._result, FileStreamHelpers.CompletedCallback) != FileStreamHelpers.NoResult) + if (Interlocked.Exchange(ref valueTaskSource._result, FileStreamHelpers.CompletedCallback) != FileStreamHelpers.NoResult) { // Successfully got the callback, finish the callback - provider.CompleteCallback(packedResult); + valueTaskSource.CompleteCallback(packedResult); } // else: Some other thread stole the result, so now it is responsible to finish the callback } @@ -226,11 +227,11 @@ private void Cancel(CancellationToken token) } /// - /// Extends with to support disposing of a + /// 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 MemoryAwaitableProvider : FileStreamAwaitableProvider + private sealed class MemoryAwaitableProvider : FileStreamValueTaskSource { private MemoryHandle _handle; // mutable struct; do not make this readonly From 877e28de8c0fd4d3f442c5d8adae5d50846110b9 Mon Sep 17 00:00:00 2001 From: carlossanlop Date: Tue, 6 Apr 2021 11:39:47 -0700 Subject: [PATCH 12/19] Bring back the raw pointer intOverlapped. --- .../AsyncWindowsFileStreamStrategy.cs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs index 0ac1813e45be96..2fe274c7f083da 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs @@ -134,7 +134,7 @@ private unsafe ValueTask ReadAsyncInternal(Memory destination, Cancel // Create and store async stream class library specific data in the async result FileStreamValueTaskSource valueTaskSource = FileStreamValueTaskSource.Create(this, _preallocatedOverlapped, 0, destination); - + NativeOverlapped* intOverlapped = valueTaskSource.Overlapped; // Calculate position in the file we should be at after the read is done long positionBefore = _filePosition; @@ -156,8 +156,8 @@ private unsafe ValueTask ReadAsyncInternal(Memory destination, Cancel // Now set the position to read from in the NativeOverlapped struct // For pipes, we should leave the offset fields set to 0. - valueTaskSource.Overlapped->OffsetLow = unchecked((int)positionBefore); - valueTaskSource.Overlapped->OffsetHigh = (int)(positionBefore >> 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 @@ -166,7 +166,7 @@ private unsafe ValueTask ReadAsyncInternal(Memory destination, Cancel } // queue an async ReadFile operation and pass in a packed overlapped - int r = FileStreamHelpers.ReadFileNative(_fileHandle, destination.Span, false, valueTaskSource.Overlapped, 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 @@ -188,7 +188,7 @@ private unsafe ValueTask ReadAsyncInternal(Memory destination, Cancel // We clear the overlapped status bit for this special case. // Failure to do so looks like we are freeing a pending overlapped later. - valueTaskSource.Overlapped->InternalLow = IntPtr.Zero; + intOverlapped->InternalLow = IntPtr.Zero; valueTaskSource.ReleaseNativeResource(); return new ValueTask(valueTaskSource.NumBufferedBytes); } @@ -250,14 +250,15 @@ private unsafe ValueTask WriteAsyncInternal(ReadOnlyMemory source, Cancell // Create and store async stream class library specific data in the async result FileStreamValueTaskSource valueTaskSource = FileStreamValueTaskSource.Create(this, _preallocatedOverlapped, 0, source); + NativeOverlapped* intOverlapped = valueTaskSource.Overlapped; long positionBefore = _filePosition; if (CanSeek) { // Now set the position to read from in the NativeOverlapped struct // For pipes, we should leave the offset fields set to 0. - valueTaskSource.Overlapped->OffsetLow = (int)positionBefore; - valueTaskSource.Overlapped->OffsetHigh = (int)(positionBefore >> 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 @@ -267,7 +268,7 @@ private unsafe ValueTask WriteAsyncInternal(ReadOnlyMemory source, Cancell } // queue an async WriteFile operation and pass in a packed overlapped - int r = FileStreamHelpers.WriteFileNative(_fileHandle, source.Span, false, valueTaskSource.Overlapped, 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 From 48a50b3a6c39d611da6f1ed40cb91d125e9aabe9 Mon Sep 17 00:00:00 2001 From: carlossanlop Date: Wed, 7 Apr 2021 08:52:20 -0700 Subject: [PATCH 13/19] Rename MemoryAwaitableProvider to MemoryFileStreamValueTaskSource. --- .../src/System/IO/Strategies/FileStreamValueTaskSource.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamValueTaskSource.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamValueTaskSource.cs index cbc225b85ac8e9..d701df63fa6884 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamValueTaskSource.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamValueTaskSource.cs @@ -44,7 +44,7 @@ public static FileStreamValueTaskSource Create( MemoryMarshal.TryGetArray(memory, out ArraySegment buffer) && preallocatedOverlapped.IsUserObject(buffer.Array) ? new FileStreamValueTaskSource(strategy, preallocatedOverlapped, numBufferedBytes, buffer.Array) : - new MemoryAwaitableProvider(strategy, numBufferedBytes, memory); + new MemoryFileStreamValueTaskSource(strategy, numBufferedBytes, memory); } protected FileStreamValueTaskSource( @@ -231,12 +231,12 @@ private void Cancel(CancellationToken token) /// when the operation has completed. This should only be used /// when memory doesn't wrap a byte[]. /// - private sealed class MemoryAwaitableProvider : FileStreamValueTaskSource + private sealed class MemoryFileStreamValueTaskSource : FileStreamValueTaskSource { private MemoryHandle _handle; // mutable struct; do not make this readonly // this type handles the pinning, so bytes are null - internal unsafe MemoryAwaitableProvider(AsyncWindowsFileStreamStrategy strategy, int numBufferedBytes, ReadOnlyMemory memory) + internal unsafe MemoryFileStreamValueTaskSource(AsyncWindowsFileStreamStrategy strategy, int numBufferedBytes, ReadOnlyMemory memory) : base(strategy, null, numBufferedBytes, null) // this type handles the pinning, so null is passed for bytes to the base { _handle = memory.Pin(); From e61732bdaae5b3bfad321fcca5e90cc04a8b2feb Mon Sep 17 00:00:00 2001 From: carlossanlop Date: Wed, 7 Apr 2021 09:11:08 -0700 Subject: [PATCH 14/19] Remove numBufferedBytes and avoid unnecessary allocation in Read(byte[],int,int). --- .../Strategies/AsyncWindowsFileStreamStrategy.cs | 15 ++++++++++----- .../IO/Strategies/FileStreamValueTaskSource.cs | 15 +++++---------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs index 2fe274c7f083da..88e4cf648670ff 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs @@ -115,9 +115,14 @@ internal override void OnBufferAllocated(byte[] buffer) => Interlocked.CompareExchange(ref _currentOverlappedOwner, newSource, existingSource); public override int Read(byte[] buffer, int offset, int count) - => ReadAsyncInternal(new Memory(buffer, offset, count)).AsTask().GetAwaiter().GetResult(); + { + ValueTask vt = ReadAsyncInternal(new Memory(buffer, offset, count)); + return vt.IsCompleted? + vt.Result : + vt.AsTask().GetAwaiter().GetResult(); + } - public override Task ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) + public override Task ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) => ReadAsyncInternal(new Memory(buffer, offset, count), cancellationToken).AsTask(); public override ValueTask ReadAsync(Memory destination, CancellationToken cancellationToken = default) @@ -133,7 +138,7 @@ private unsafe ValueTask ReadAsyncInternal(Memory destination, Cancel Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed"); // Create and store async stream class library specific data in the async result - FileStreamValueTaskSource valueTaskSource = FileStreamValueTaskSource.Create(this, _preallocatedOverlapped, 0, destination); + FileStreamValueTaskSource valueTaskSource = FileStreamValueTaskSource.Create(this, _preallocatedOverlapped, destination); NativeOverlapped* intOverlapped = valueTaskSource.Overlapped; // Calculate position in the file we should be at after the read is done @@ -190,7 +195,7 @@ private unsafe ValueTask ReadAsyncInternal(Memory destination, Cancel // Failure to do so looks like we are freeing a pending overlapped later. intOverlapped->InternalLow = IntPtr.Zero; valueTaskSource.ReleaseNativeResource(); - return new ValueTask(valueTaskSource.NumBufferedBytes); + return new ValueTask(0); } else if (errorCode != Interop.Errors.ERROR_IO_PENDING) { @@ -249,7 +254,7 @@ private unsafe ValueTask WriteAsyncInternal(ReadOnlyMemory source, Cancell Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed"); // Create and store async stream class library specific data in the async result - FileStreamValueTaskSource valueTaskSource = FileStreamValueTaskSource.Create(this, _preallocatedOverlapped, 0, source); + FileStreamValueTaskSource valueTaskSource = FileStreamValueTaskSource.Create(this, _preallocatedOverlapped, source); NativeOverlapped* intOverlapped = valueTaskSource.Overlapped; long positionBefore = _filePosition; diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamValueTaskSource.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamValueTaskSource.cs index d701df63fa6884..574aac52d46bf9 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamValueTaskSource.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamValueTaskSource.cs @@ -20,7 +20,6 @@ private unsafe class FileStreamValueTaskSource : IValueTaskSource, IValueTa internal static readonly IOCompletionCallback s_ioCallback = IOCallback; private readonly AsyncWindowsFileStreamStrategy _strategy; - private readonly int _numBufferedBytes; private ManualResetValueTaskSourceCore _source; // mutable struct; do not make this readonly private NativeOverlapped* _overlapped; @@ -33,7 +32,6 @@ private unsafe class FileStreamValueTaskSource : IValueTaskSource, IValueTa public static FileStreamValueTaskSource Create( AsyncWindowsFileStreamStrategy strategy, PreAllocatedOverlapped? preallocatedOverlapped, - int numBufferedBytes, ReadOnlyMemory memory) { // If the memory passed in is the strategy's internal buffer, we can use the base AwaitableProvider, @@ -43,18 +41,16 @@ public static FileStreamValueTaskSource Create( return preallocatedOverlapped != null && MemoryMarshal.TryGetArray(memory, out ArraySegment buffer) && preallocatedOverlapped.IsUserObject(buffer.Array) ? - new FileStreamValueTaskSource(strategy, preallocatedOverlapped, numBufferedBytes, buffer.Array) : - new MemoryFileStreamValueTaskSource(strategy, numBufferedBytes, memory); + new FileStreamValueTaskSource(strategy, preallocatedOverlapped, buffer.Array) : + new MemoryFileStreamValueTaskSource(strategy, memory); } protected FileStreamValueTaskSource( AsyncWindowsFileStreamStrategy strategy, PreAllocatedOverlapped? preallocatedOverlapped, - int numBufferedBytes, byte[]? bytes) { _strategy = strategy; - _numBufferedBytes = numBufferedBytes; _result = FileStreamHelpers.NoResult; @@ -71,7 +67,6 @@ protected FileStreamValueTaskSource( } internal NativeOverlapped* Overlapped => _overlapped; - internal int NumBufferedBytes => _numBufferedBytes; public ValueTaskSourceStatus GetStatus(short token) => _source.GetStatus(token); public void OnCompleted(Action continuation, object? state, short token, ValueTaskSourceOnCompletedFlags flags) => _source.OnCompleted(continuation, state, token, flags); void IValueTaskSource.GetResult(short token) => _source.GetResult(token); @@ -203,7 +198,7 @@ private void CompleteCallback(ulong packedResult) else { Debug.Assert(result == FileStreamHelpers.ResultSuccess, "Unknown result"); - _source.SetResult((int)(packedResult & uint.MaxValue) + _numBufferedBytes); + _source.SetResult((int)(packedResult & uint.MaxValue)); } } @@ -236,8 +231,8 @@ private sealed class MemoryFileStreamValueTaskSource : FileStreamValueTaskSource private MemoryHandle _handle; // mutable struct; do not make this readonly // this type handles the pinning, so bytes are null - internal unsafe MemoryFileStreamValueTaskSource(AsyncWindowsFileStreamStrategy strategy, int numBufferedBytes, ReadOnlyMemory memory) - : base(strategy, null, numBufferedBytes, null) // this type handles the pinning, so null is passed for bytes to the base + internal unsafe MemoryFileStreamValueTaskSource(AsyncWindowsFileStreamStrategy strategy, ReadOnlyMemory memory) + : base(strategy, null, null) // this type handles the pinning, so null is passed for bytes to the base { _handle = memory.Pin(); } From ba3e23e4d5b9ea794ba7a4402d9e910fe06b77eb Mon Sep 17 00:00:00 2001 From: carlossanlop Date: Wed, 7 Apr 2021 09:18:21 -0700 Subject: [PATCH 15/19] Rename files of nested types. --- .../System.Private.CoreLib.Shared.projitems | 4 ++-- ...dowsFileStreamStrategy.ValueTaskSource.cs} | 24 +++++++++---------- .../AsyncWindowsFileStreamStrategy.cs | 10 ++++---- ...treamStrategy.CompletionSource.Windows.cs} | 22 ++++++++--------- .../Net5CompatFileStreamStrategy.Windows.cs | 10 ++++---- 5 files changed, 35 insertions(+), 35 deletions(-) rename src/libraries/System.Private.CoreLib/src/System/IO/Strategies/{FileStreamValueTaskSource.cs => AsyncWindowsFileStreamStrategy.ValueTaskSource.cs} (92%) rename src/libraries/System.Private.CoreLib/src/System/IO/Strategies/{FileStreamCompletionSource.Win32.cs => Net5CompatFileStreamStrategy.CompletionSource.Windows.cs} (92%) 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 85dd8331ebf121..736e86c5bfa828 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 @@ -1652,9 +1652,9 @@ - + - + diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamValueTaskSource.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.ValueTaskSource.cs similarity index 92% rename from src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamValueTaskSource.cs rename to src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.ValueTaskSource.cs index 574aac52d46bf9..f0e11765dee4ad 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamValueTaskSource.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.ValueTaskSource.cs @@ -15,7 +15,7 @@ internal sealed partial class AsyncWindowsFileStreamStrategy : WindowsFileStream /// /// Type that helps reduce allocations for FileStream.ReadAsync and FileStream.WriteAsync. /// - private unsafe class FileStreamValueTaskSource : IValueTaskSource, IValueTaskSource + private unsafe class ValueTaskSource : IValueTaskSource, IValueTaskSource { internal static readonly IOCompletionCallback s_ioCallback = IOCallback; @@ -29,7 +29,7 @@ private unsafe class FileStreamValueTaskSource : IValueTaskSource, IValueTa private bool _cancellationHasBeenRegistered; #endif - public static FileStreamValueTaskSource Create( + public static ValueTaskSource Create( AsyncWindowsFileStreamStrategy strategy, PreAllocatedOverlapped? preallocatedOverlapped, ReadOnlyMemory memory) @@ -41,11 +41,11 @@ public static FileStreamValueTaskSource Create( return preallocatedOverlapped != null && MemoryMarshal.TryGetArray(memory, out ArraySegment buffer) && preallocatedOverlapped.IsUserObject(buffer.Array) ? - new FileStreamValueTaskSource(strategy, preallocatedOverlapped, buffer.Array) : - new MemoryFileStreamValueTaskSource(strategy, memory); + new ValueTaskSource(strategy, preallocatedOverlapped, buffer.Array) : + new MemoryValueTaskSource(strategy, memory); } - protected FileStreamValueTaskSource( + protected ValueTaskSource( AsyncWindowsFileStreamStrategy strategy, PreAllocatedOverlapped? preallocatedOverlapped, byte[]? bytes) @@ -88,7 +88,7 @@ internal void RegisterForCancellation(CancellationToken cancellationToken) long packedResult = Interlocked.CompareExchange(ref _result, FileStreamHelpers.RegisteringCancellation, FileStreamHelpers.NoResult); if (packedResult == FileStreamHelpers.NoResult) { - _cancellationRegistration = cancellationToken.UnsafeRegister(static (s, token) => ((FileStreamValueTaskSource)s!).Cancel(token), this); + _cancellationRegistration = cancellationToken.UnsafeRegister(static (s, token) => ((ValueTaskSource)s!).Cancel(token), this); // Switch the result, just in case IO completed while we were setting the registration packedResult = Interlocked.Exchange(ref _result, FileStreamHelpers.NoResult); @@ -135,11 +135,11 @@ private static void IOCallback(uint errorCode, uint numBytes, NativeOverlapped* // be directly the AwaitableProvider 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 (AsyncWindowsFileStreamStrategy or FileStreamValueTaskSource)); - FileStreamValueTaskSource valueTaskSource = state switch + Debug.Assert(state is (AsyncWindowsFileStreamStrategy or ValueTaskSource)); + ValueTaskSource valueTaskSource = state switch { AsyncWindowsFileStreamStrategy strategy => strategy._currentOverlappedOwner!, // must be owned - _ => (FileStreamValueTaskSource)state + _ => (ValueTaskSource)state }; Debug.Assert(valueTaskSource != null); Debug.Assert(valueTaskSource._overlapped == pOverlapped, "Overlaps don't match"); @@ -222,16 +222,16 @@ private void Cancel(CancellationToken token) } /// - /// Extends with to support disposing of a + /// 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 MemoryFileStreamValueTaskSource : FileStreamValueTaskSource + private sealed class MemoryValueTaskSource : ValueTaskSource { private MemoryHandle _handle; // mutable struct; do not make this readonly // this type handles the pinning, so bytes are null - internal unsafe MemoryFileStreamValueTaskSource(AsyncWindowsFileStreamStrategy strategy, ReadOnlyMemory memory) + internal unsafe MemoryValueTaskSource(AsyncWindowsFileStreamStrategy strategy, ReadOnlyMemory memory) : base(strategy, null, null) // this type handles the pinning, so null is passed for bytes to the base { _handle = memory.Pin(); diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs index 88e4cf648670ff..18cbb7455d38ba 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs @@ -11,7 +11,7 @@ namespace System.IO.Strategies internal sealed partial class AsyncWindowsFileStreamStrategy : WindowsFileStreamStrategy { private PreAllocatedOverlapped? _preallocatedOverlapped; // optimization for async ops to avoid per-op allocations - private FileStreamValueTaskSource? _currentOverlappedOwner; // async op currently using the preallocated overlapped + private ValueTaskSource? _currentOverlappedOwner; // async op currently using the preallocated overlapped internal AsyncWindowsFileStreamStrategy(SafeFileHandle handle, FileAccess access, FileShare share) : base(handle, access, share) @@ -108,10 +108,10 @@ internal override void OnBufferAllocated(byte[] buffer) { Debug.Assert(_preallocatedOverlapped == null); - _preallocatedOverlapped = new PreAllocatedOverlapped(FileStreamValueTaskSource.s_ioCallback, this, buffer); + _preallocatedOverlapped = new PreAllocatedOverlapped(ValueTaskSource.s_ioCallback, this, buffer); } - private FileStreamValueTaskSource? CompareExchangeCurrentOverlappedOwner(FileStreamValueTaskSource? newSource, FileStreamValueTaskSource? existingSource) + private ValueTaskSource? CompareExchangeCurrentOverlappedOwner(ValueTaskSource? newSource, ValueTaskSource? existingSource) => Interlocked.CompareExchange(ref _currentOverlappedOwner, newSource, existingSource); public override int Read(byte[] buffer, int offset, int count) @@ -138,7 +138,7 @@ private unsafe ValueTask ReadAsyncInternal(Memory destination, Cancel Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed"); // Create and store async stream class library specific data in the async result - FileStreamValueTaskSource valueTaskSource = FileStreamValueTaskSource.Create(this, _preallocatedOverlapped, destination); + ValueTaskSource valueTaskSource = ValueTaskSource.Create(this, _preallocatedOverlapped, destination); NativeOverlapped* intOverlapped = valueTaskSource.Overlapped; // Calculate position in the file we should be at after the read is done @@ -254,7 +254,7 @@ private unsafe ValueTask WriteAsyncInternal(ReadOnlyMemory source, Cancell Debug.Assert(!_fileHandle.IsClosed, "!_handle.IsClosed"); // Create and store async stream class library specific data in the async result - FileStreamValueTaskSource valueTaskSource = FileStreamValueTaskSource.Create(this, _preallocatedOverlapped, source); + ValueTaskSource valueTaskSource = ValueTaskSource.Create(this, _preallocatedOverlapped, source); NativeOverlapped* intOverlapped = valueTaskSource.Overlapped; long positionBefore = _filePosition; diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamCompletionSource.Win32.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.CompletionSource.Windows.cs similarity index 92% rename from src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamCompletionSource.Win32.cs rename to src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.CompletionSource.Windows.cs index bd60664682b9f0..b65adf5ae04f23 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamCompletionSource.Win32.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.CompletionSource.Windows.cs @@ -14,7 +14,7 @@ internal sealed partial class Net5CompatFileStreamStrategy : 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 unsafe class CompletionSource : TaskCompletionSource { internal static readonly unsafe IOCompletionCallback s_ioCallback = IOCallback; @@ -30,7 +30,7 @@ private unsafe class FileStreamCompletionSource : TaskCompletionSource 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(Net5CompatFileStreamStrategy strategy, PreAllocatedOverlapped? preallocatedOverlapped, + internal CompletionSource(Net5CompatFileStreamStrategy strategy, PreAllocatedOverlapped? preallocatedOverlapped, int numBufferedBytes, byte[]? bytes) : base(TaskCreationOptions.RunContinuationsAsynchronously) { _numBufferedBytes = numBufferedBytes; @@ -120,11 +120,11 @@ internal static void IOCallback(uint errorCode, uint numBytes, NativeOverlapped* // 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 Net5CompatFileStreamStrategy || state is FileStreamCompletionSource); - FileStreamCompletionSource completionSource = state switch + Debug.Assert(state is Net5CompatFileStreamStrategy || state is CompletionSource); + CompletionSource completionSource = state switch { Net5CompatFileStreamStrategy strategy => strategy._currentOverlappedOwner!, // must be owned - _ => (FileStreamCompletionSource)state + _ => (CompletionSource)state }; Debug.Assert(completionSource != null); Debug.Assert(completionSource._overlapped == pOverlapped, "Overlaps don't match"); @@ -191,8 +191,8 @@ 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(state is CompletionSource, "Unknown state passed to cancellation"); + CompletionSource completionSource = (CompletionSource)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 @@ -210,7 +210,7 @@ private static void Cancel(object? state) } } - public static FileStreamCompletionSource Create(Net5CompatFileStreamStrategy strategy, PreAllocatedOverlapped? preallocatedOverlapped, + public static CompletionSource Create(Net5CompatFileStreamStrategy strategy, PreAllocatedOverlapped? preallocatedOverlapped, int numBufferedBytesRead, ReadOnlyMemory memory) { // If the memory passed in is the strategy's internal buffer, we can use the base FileStreamCompletionSource, @@ -219,17 +219,17 @@ public static FileStreamCompletionSource Create(Net5CompatFileStreamStrategy str // where the underlying memory is backed by pre-pinned buffers. return preallocatedOverlapped != null && MemoryMarshal.TryGetArray(memory, out ArraySegment buffer) && preallocatedOverlapped.IsUserObject(buffer.Array) // preallocatedOverlapped is allocated when BufferedStream|Net5CompatFileStreamStrategy allocates the buffer - ? new FileStreamCompletionSource(strategy, preallocatedOverlapped, numBufferedBytesRead, buffer.Array) + ? new CompletionSource(strategy, preallocatedOverlapped, numBufferedBytesRead, buffer.Array) : new MemoryFileStreamCompletionSource(strategy, numBufferedBytesRead, memory); } } /// - /// Extends with to support disposing of a + /// 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 sealed class MemoryFileStreamCompletionSource : CompletionSource { private MemoryHandle _handle; // mutable struct; do not make this readonly diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.Windows.cs index 67b639a6829a02..fead01218fb0c3 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.Windows.cs @@ -44,7 +44,7 @@ internal sealed partial class Net5CompatFileStreamStrategy : FileStreamStrategy 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 + private CompletionSource? _currentOverlappedOwner; // async op currently using the preallocated overlapped private void Init(FileMode mode, FileShare share, string originalPath, FileOptions options) { @@ -544,10 +544,10 @@ partial void OnBufferAllocated() Debug.Assert(_preallocatedOverlapped == null); if (_useAsyncIO) - _preallocatedOverlapped = new PreAllocatedOverlapped(FileStreamCompletionSource.s_ioCallback, this, _buffer); + _preallocatedOverlapped = new PreAllocatedOverlapped(CompletionSource.s_ioCallback, this, _buffer); } - private FileStreamCompletionSource? CompareExchangeCurrentOverlappedOwner(FileStreamCompletionSource? newSource, FileStreamCompletionSource? existingSource) + private CompletionSource? CompareExchangeCurrentOverlappedOwner(CompletionSource? newSource, CompletionSource? existingSource) => Interlocked.CompareExchange(ref _currentOverlappedOwner, newSource, existingSource); private void WriteSpan(ReadOnlySpan source) @@ -757,7 +757,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, _preallocatedOverlapped, numBufferedBytesRead, destination); + CompletionSource completionSource = CompletionSource.Create(this, _preallocatedOverlapped, numBufferedBytesRead, destination); NativeOverlapped* intOverlapped = completionSource.Overlapped; // Calculate position in the file we should be at after the read is done @@ -975,7 +975,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, _preallocatedOverlapped, 0, source); + CompletionSource completionSource = CompletionSource.Create(this, _preallocatedOverlapped, 0, source); NativeOverlapped* intOverlapped = completionSource.Overlapped; if (CanSeek) From 6595f14038f0c3fc52ede967019928a89063898f Mon Sep 17 00:00:00 2001 From: carlossanlop Date: Thu, 8 Apr 2021 13:30:24 -0700 Subject: [PATCH 16/19] Nested async result codes in static class. --- ...ndowsFileStreamStrategy.ValueTaskSource.cs | 30 ++++++++++--------- .../Strategies/FileStreamHelpers.Windows.cs | 18 +++++++---- ...StreamStrategy.CompletionSource.Windows.cs | 30 ++++++++++--------- 3 files changed, 44 insertions(+), 34 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.ValueTaskSource.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.ValueTaskSource.cs index f0e11765dee4ad..b23c784be99607 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.ValueTaskSource.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.ValueTaskSource.cs @@ -10,6 +10,8 @@ namespace System.IO.Strategies { + using TaskSourceCodes = FileStreamHelpers.TaskSourceCodes; + internal sealed partial class AsyncWindowsFileStreamStrategy : WindowsFileStreamStrategy { /// @@ -52,7 +54,7 @@ protected ValueTaskSource( { _strategy = strategy; - _result = FileStreamHelpers.NoResult; + _result = TaskSourceCodes.NoResult; _source = default; // Using RunContinuationsAsynchronously for compat reasons (old API used Task.Factory.StartNew for continuations) @@ -85,23 +87,23 @@ internal void RegisterForCancellation(CancellationToken cancellationToken) if (_overlapped != null) { // Register the cancellation only if the IO hasn't completed - long packedResult = Interlocked.CompareExchange(ref _result, FileStreamHelpers.RegisteringCancellation, FileStreamHelpers.NoResult); - if (packedResult == FileStreamHelpers.NoResult) + long packedResult = Interlocked.CompareExchange(ref _result, TaskSourceCodes.RegisteringCancellation, TaskSourceCodes.NoResult); + if (packedResult == TaskSourceCodes.NoResult) { _cancellationRegistration = cancellationToken.UnsafeRegister(static (s, token) => ((ValueTaskSource)s!).Cancel(token), this); // Switch the result, just in case IO completed while we were setting the registration - packedResult = Interlocked.Exchange(ref _result, FileStreamHelpers.NoResult); + packedResult = Interlocked.Exchange(ref _result, TaskSourceCodes.NoResult); } - else if (packedResult != FileStreamHelpers.CompletedCallback) + else if (packedResult != TaskSourceCodes.CompletedCallback) { // Failed to set the result, IO is in the process of completing // Attempt to take the packed result - packedResult = Interlocked.Exchange(ref _result, FileStreamHelpers.NoResult); + packedResult = Interlocked.Exchange(ref _result, TaskSourceCodes.NoResult); } // If we have a callback that needs to be completed - if ((packedResult != FileStreamHelpers.NoResult) && (packedResult != FileStreamHelpers.CompletedCallback) && (packedResult != FileStreamHelpers.RegisteringCancellation)) + if ((packedResult != TaskSourceCodes.NoResult) && (packedResult != TaskSourceCodes.CompletedCallback) && (packedResult != TaskSourceCodes.RegisteringCancellation)) { CompleteCallback((ulong)packedResult); } @@ -151,19 +153,19 @@ private static void IOCallback(uint errorCode, uint numBytes, NativeOverlapped* ulong packedResult; if (errorCode != 0 && errorCode != Interop.Errors.ERROR_BROKEN_PIPE && errorCode != Interop.Errors.ERROR_NO_DATA) { - packedResult = ((ulong)FileStreamHelpers.ResultError | errorCode); + packedResult = ((ulong)TaskSourceCodes.ResultError | errorCode); } else { - packedResult = ((ulong)FileStreamHelpers.ResultSuccess | numBytes); + packedResult = ((ulong)TaskSourceCodes.ResultSuccess | numBytes); } // Stow the result so that other threads can observe it // And, if no other thread is registering cancellation, continue - if (Interlocked.Exchange(ref valueTaskSource._result, (long)packedResult) == FileStreamHelpers.NoResult) + if (Interlocked.Exchange(ref valueTaskSource._result, (long)packedResult) == TaskSourceCodes.NoResult) { // Successfully set the state, attempt to take back the callback - if (Interlocked.Exchange(ref valueTaskSource._result, FileStreamHelpers.CompletedCallback) != FileStreamHelpers.NoResult) + if (Interlocked.Exchange(ref valueTaskSource._result, TaskSourceCodes.CompletedCallback) != TaskSourceCodes.NoResult) { // Successfully got the callback, finish the callback valueTaskSource.CompleteCallback(packedResult); @@ -180,8 +182,8 @@ private void CompleteCallback(ulong packedResult) ReleaseNativeResource(); // Unpack the result and send it to the user - long result = (long)(packedResult & FileStreamHelpers.ResultMask); - if (result == FileStreamHelpers.ResultError) + long result = (long)(packedResult & TaskSourceCodes.ResultMask); + if (result == TaskSourceCodes.ResultError) { int errorCode = unchecked((int)(packedResult & uint.MaxValue)); if (errorCode == Interop.Errors.ERROR_OPERATION_ABORTED) @@ -197,7 +199,7 @@ private void CompleteCallback(ulong packedResult) } else { - Debug.Assert(result == FileStreamHelpers.ResultSuccess, "Unknown result"); + Debug.Assert(result == TaskSourceCodes.ResultSuccess, "Unknown result"); _source.SetResult((int)(packedResult & uint.MaxValue)); } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Windows.cs index 23d86c3b8c3209..b35b8d7b01cb78 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Windows.cs @@ -14,12 +14,18 @@ namespace System.IO.Strategies // this type defines a set of stateless FileStream/FileStreamStrategy helper methods internal static partial class FileStreamHelpers { - internal const long NoResult = 0; - internal const long ResultSuccess = (long)1 << 32; - internal const long ResultError = (long)2 << 32; - internal const long RegisteringCancellation = (long)4 << 32; - internal const long CompletedCallback = (long)8 << 32; - internal const ulong ResultMask = ((ulong)uint.MaxValue) << 32; + // Async completion/return codes shared by: + // - AsyncWindowsFileStreamStrategy.ValueTaskSource + // - Net5CompatFileStreamStrategy.CompletionSource + internal static class TaskSourceCodes + { + internal const long NoResult = 0; + internal const long ResultSuccess = (long)1 << 32; + internal const long ResultError = (long)2 << 32; + internal const long RegisteringCancellation = (long)4 << 32; + internal const long CompletedCallback = (long)8 << 32; + internal const ulong ResultMask = ((ulong)uint.MaxValue) << 32; + } private static FileStreamStrategy ChooseStrategyCore(SafeFileHandle handle, FileAccess access, FileShare share, int bufferSize, bool isAsync) { diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.CompletionSource.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.CompletionSource.Windows.cs index b65adf5ae04f23..41380156ca3bbe 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.CompletionSource.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.CompletionSource.Windows.cs @@ -9,6 +9,8 @@ namespace System.IO.Strategies { + using TaskSourceCodes = FileStreamHelpers.TaskSourceCodes; + internal sealed partial class Net5CompatFileStreamStrategy : FileStreamStrategy { // This is an internal object extending TaskCompletionSource with fields @@ -35,7 +37,7 @@ internal CompletionSource(Net5CompatFileStreamStrategy strategy, PreAllocatedOve { _numBufferedBytes = numBufferedBytes; _strategy = strategy; - _result = FileStreamHelpers.NoResult; + _result = TaskSourceCodes.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 @@ -67,23 +69,23 @@ public void RegisterForCancellation(CancellationToken cancellationToken) Action? cancelCallback = s_cancelCallback ??= Cancel; // Register the cancellation only if the IO hasn't completed - long packedResult = Interlocked.CompareExchange(ref _result, FileStreamHelpers.RegisteringCancellation, FileStreamHelpers.NoResult); - if (packedResult == FileStreamHelpers.NoResult) + long packedResult = Interlocked.CompareExchange(ref _result, TaskSourceCodes.RegisteringCancellation, TaskSourceCodes.NoResult); + if (packedResult == TaskSourceCodes.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, FileStreamHelpers.NoResult); + packedResult = Interlocked.Exchange(ref _result, TaskSourceCodes.NoResult); } - else if (packedResult != FileStreamHelpers.CompletedCallback) + else if (packedResult != TaskSourceCodes.CompletedCallback) { // Failed to set the result, IO is in the process of completing // Attempt to take the packed result - packedResult = Interlocked.Exchange(ref _result, FileStreamHelpers.NoResult); + packedResult = Interlocked.Exchange(ref _result, TaskSourceCodes.NoResult); } // If we have a callback that needs to be completed - if ((packedResult != FileStreamHelpers.NoResult) && (packedResult != FileStreamHelpers.CompletedCallback) && (packedResult != FileStreamHelpers.RegisteringCancellation)) + if ((packedResult != TaskSourceCodes.NoResult) && (packedResult != TaskSourceCodes.CompletedCallback) && (packedResult != TaskSourceCodes.RegisteringCancellation)) { CompleteCallback((ulong)packedResult); } @@ -136,19 +138,19 @@ internal static void IOCallback(uint errorCode, uint numBytes, NativeOverlapped* ulong packedResult; if (errorCode != 0 && errorCode != Interop.Errors.ERROR_BROKEN_PIPE && errorCode != Interop.Errors.ERROR_NO_DATA) { - packedResult = ((ulong)FileStreamHelpers.ResultError | errorCode); + packedResult = ((ulong)TaskSourceCodes.ResultError | errorCode); } else { - packedResult = ((ulong)FileStreamHelpers.ResultSuccess | numBytes); + packedResult = ((ulong)TaskSourceCodes.ResultSuccess | numBytes); } // Stow the result so that other threads can observe it // And, if no other thread is registering cancellation, continue - if (FileStreamHelpers.NoResult == Interlocked.Exchange(ref completionSource._result, (long)packedResult)) + if (TaskSourceCodes.NoResult == Interlocked.Exchange(ref completionSource._result, (long)packedResult)) { // Successfully set the state, attempt to take back the callback - if (Interlocked.Exchange(ref completionSource._result, FileStreamHelpers.CompletedCallback) != FileStreamHelpers.NoResult) + if (Interlocked.Exchange(ref completionSource._result, TaskSourceCodes.CompletedCallback) != TaskSourceCodes.NoResult) { // Successfully got the callback, finish the callback completionSource.CompleteCallback(packedResult); @@ -165,8 +167,8 @@ private void CompleteCallback(ulong packedResult) ReleaseNativeResource(); // Unpack the result and send it to the user - long result = (long)(packedResult & FileStreamHelpers.ResultMask); - if (result == FileStreamHelpers.ResultError) + long result = (long)(packedResult & TaskSourceCodes.ResultMask); + if (result == TaskSourceCodes.ResultError) { int errorCode = unchecked((int)(packedResult & uint.MaxValue)); if (errorCode == Interop.Errors.ERROR_OPERATION_ABORTED) @@ -182,7 +184,7 @@ private void CompleteCallback(ulong packedResult) } else { - Debug.Assert(result == FileStreamHelpers.ResultSuccess, "Unknown result"); + Debug.Assert(result == TaskSourceCodes.ResultSuccess, "Unknown result"); TrySetResult((int)(packedResult & uint.MaxValue) + _numBufferedBytes); } } From 32bdb7b5d0cd4ca1ab67ba79ffa7b0155b404c94 Mon Sep 17 00:00:00 2001 From: carlossanlop Date: Thu, 8 Apr 2021 14:08:23 -0700 Subject: [PATCH 17/19] Address feedback. --- ...ndowsFileStreamStrategy.ValueTaskSource.cs | 32 +++++++++---------- .../AsyncWindowsFileStreamStrategy.cs | 2 +- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.ValueTaskSource.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.ValueTaskSource.cs index b23c784be99607..c96ffd8ea79b6d 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.ValueTaskSource.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.ValueTaskSource.cs @@ -3,7 +3,6 @@ using System.Buffers; using System.Diagnostics; -using System.Runtime.ExceptionServices; using System.Runtime.InteropServices; using System.Threading; using System.Threading.Tasks.Sources; @@ -53,17 +52,12 @@ protected ValueTaskSource( byte[]? bytes) { _strategy = strategy; - _result = TaskSourceCodes.NoResult; - _source = default; - // Using RunContinuationsAsynchronously for compat reasons (old API used Task.Factory.StartNew for continuations) - _source.RunContinuationsAsynchronously = true; - _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); + _strategy._fileHandle.ThreadPoolBinding!.AllocateNativeOverlapped(s_ioCallback, this, bytes); Debug.Assert(_overlapped != null, "AllocateNativeOverlapped returned null"); } @@ -137,7 +131,7 @@ private static void IOCallback(uint errorCode, uint numBytes, NativeOverlapped* // be directly the AwaitableProvider 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 (AsyncWindowsFileStreamStrategy or ValueTaskSource)); + Debug.Assert(state is AsyncWindowsFileStreamStrategy or ValueTaskSource); ValueTaskSource valueTaskSource = state switch { AsyncWindowsFileStreamStrategy strategy => strategy._currentOverlappedOwner!, // must be owned @@ -177,8 +171,8 @@ private static void IOCallback(uint errorCode, uint numBytes, NativeOverlapped* private void CompleteCallback(ulong packedResult) { - // Free up the native resource and cancellation registration - CancellationToken cancellationToken = _cancellationRegistration.Token; // access before disposing registration + CancellationToken cancellationToken = _cancellationRegistration.Token; + ReleaseNativeResource(); // Unpack the result and send it to the user @@ -186,16 +180,18 @@ private void CompleteCallback(ulong packedResult) if (result == TaskSourceCodes.ResultError) { int errorCode = unchecked((int)(packedResult & uint.MaxValue)); + Exception e; if (errorCode == Interop.Errors.ERROR_OPERATION_ABORTED) { - _source.SetException(ExceptionDispatchInfo.SetCurrentStackTrace(new OperationCanceledException(cancellationToken.IsCancellationRequested ? cancellationToken : new CancellationToken(canceled: true)))); + CancellationToken ct = cancellationToken.IsCancellationRequested ? cancellationToken : new CancellationToken(canceled: true); + e = new OperationCanceledException(ct); } else { - Exception e = Win32Marshal.GetExceptionForWin32Error(errorCode); - e.SetCurrentStackTrace(); - _source.SetException(ExceptionDispatchInfo.SetCurrentStackTrace(e)); + e = Win32Marshal.GetExceptionForWin32Error(errorCode); } + e.SetCurrentStackTrace(); + _source.SetException(e); } else { @@ -206,6 +202,9 @@ private void CompleteCallback(ulong packedResult) private void Cancel(CancellationToken token) { + // WARNING: This may potentially be called under a lock (during cancellation registration) + Debug.Assert(_overlapped != null && GetStatus(Version) != ValueTaskSourceStatus.Succeeded, "IO should not have completed yet"); + // If the handle is still valid, attempt to cancel the IO if (!_strategy._fileHandle.IsInvalid && !Interop.Kernel32.CancelIoEx(_strategy._fileHandle, _overlapped)) @@ -216,8 +215,9 @@ private void Cancel(CancellationToken token) // This probably means that the IO operation has completed. if (errorCode != Interop.Errors.ERROR_NOT_FOUND) { - _source.SetException(ExceptionDispatchInfo.SetCurrentStackTrace( // TODO: Resource string for exception - new OperationCanceledException("IO operation cancelled.", Win32Marshal.GetExceptionForWin32Error(errorCode), token))); + Exception e = new OperationCanceledException(SR.OperationCanceled, Win32Marshal.GetExceptionForWin32Error(errorCode), token); + e.SetCurrentStackTrace(); + _source.SetException(e); } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs index 18cbb7455d38ba..30594f2000246f 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs @@ -122,7 +122,7 @@ public override int Read(byte[] buffer, int offset, int count) vt.AsTask().GetAwaiter().GetResult(); } - public override Task ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) + public override Task ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) => ReadAsyncInternal(new Memory(buffer, offset, count), cancellationToken).AsTask(); public override ValueTask ReadAsync(Memory destination, CancellationToken cancellationToken = default) From b05dab9e3c63180c1569dc114c766a9d3a823e4d Mon Sep 17 00:00:00 2001 From: carlossanlop Date: Fri, 9 Apr 2021 09:20:39 -0700 Subject: [PATCH 18/19] Address suggestions --- .../AsyncWindowsFileStreamStrategy.ValueTaskSource.cs | 5 ++--- .../IO/Strategies/AsyncWindowsFileStreamStrategy.cs | 10 +++++----- ...ompatFileStreamStrategy.CompletionSource.Windows.cs | 3 +-- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.ValueTaskSource.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.ValueTaskSource.cs index c96ffd8ea79b6d..b165ffa8715e6a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.ValueTaskSource.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.ValueTaskSource.cs @@ -6,11 +6,10 @@ using System.Runtime.InteropServices; using System.Threading; using System.Threading.Tasks.Sources; +using TaskSourceCodes = System.IO.Strategies.FileStreamHelpers.TaskSourceCodes; namespace System.IO.Strategies { - using TaskSourceCodes = FileStreamHelpers.TaskSourceCodes; - internal sealed partial class AsyncWindowsFileStreamStrategy : WindowsFileStreamStrategy { /// @@ -84,7 +83,7 @@ internal void RegisterForCancellation(CancellationToken cancellationToken) long packedResult = Interlocked.CompareExchange(ref _result, TaskSourceCodes.RegisteringCancellation, TaskSourceCodes.NoResult); if (packedResult == TaskSourceCodes.NoResult) { - _cancellationRegistration = cancellationToken.UnsafeRegister(static (s, token) => ((ValueTaskSource)s!).Cancel(token), this); + _cancellationRegistration = cancellationToken.UnsafeRegister((s, token) => Cancel(token), this); // Switch the result, just in case IO completed while we were setting the registration packedResult = Interlocked.Exchange(ref _result, TaskSourceCodes.NoResult); diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs index 30594f2000246f..d8b52fb01e0427 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.cs @@ -10,7 +10,7 @@ namespace System.IO.Strategies { internal sealed partial class AsyncWindowsFileStreamStrategy : WindowsFileStreamStrategy { - private PreAllocatedOverlapped? _preallocatedOverlapped; // optimization for async ops to avoid per-op allocations + private PreAllocatedOverlapped? _preallocatedOverlapped; // optimization for async ops to avoid per-op allocations private ValueTaskSource? _currentOverlappedOwner; // async op currently using the preallocated overlapped internal AsyncWindowsFileStreamStrategy(SafeFileHandle handle, FileAccess access, FileShare share) @@ -116,10 +116,10 @@ internal override void OnBufferAllocated(byte[] buffer) public override int Read(byte[] buffer, int offset, int count) { - ValueTask vt = ReadAsyncInternal(new Memory(buffer, offset, count)); - return vt.IsCompleted? - vt.Result : - vt.AsTask().GetAwaiter().GetResult(); + ValueTask vt = ReadAsyncInternal(new Memory(buffer, offset, count), CancellationToken.None); + return vt.IsCompleted ? + vt.Result : + vt.AsTask().GetAwaiter().GetResult(); } public override Task ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.CompletionSource.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.CompletionSource.Windows.cs index 41380156ca3bbe..de5af4778a198a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.CompletionSource.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/Net5CompatFileStreamStrategy.CompletionSource.Windows.cs @@ -6,11 +6,10 @@ using System.Runtime.InteropServices; using System.Threading; using System.Threading.Tasks; +using TaskSourceCodes = System.IO.Strategies.FileStreamHelpers.TaskSourceCodes; namespace System.IO.Strategies { - using TaskSourceCodes = FileStreamHelpers.TaskSourceCodes; - internal sealed partial class Net5CompatFileStreamStrategy : FileStreamStrategy { // This is an internal object extending TaskCompletionSource with fields From d5344be37eb5e99aa8e1206bc45d7671bd85f9dc Mon Sep 17 00:00:00 2001 From: carlossanlop Date: Thu, 15 Apr 2021 10:09:02 -0700 Subject: [PATCH 19/19] Bring back RunContinuationAsynchronously=true --- .../AsyncWindowsFileStreamStrategy.ValueTaskSource.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.ValueTaskSource.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.ValueTaskSource.cs index b165ffa8715e6a..35a9a2e51736b3 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.ValueTaskSource.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/AsyncWindowsFileStreamStrategy.ValueTaskSource.cs @@ -53,6 +53,9 @@ protected ValueTaskSource( _strategy = strategy; _result = TaskSourceCodes.NoResult; + _source = default; + _source.RunContinuationsAsynchronously = true; + _overlapped = bytes != null && _strategy.CompareExchangeCurrentOverlappedOwner(this, null) == null ? _strategy._fileHandle.ThreadPoolBinding!.AllocateNativeOverlapped(preallocatedOverlapped!) : // allocated when buffer was created, and buffer is non-null