PR feedback

- Move NativeOverlapped* to the AsyncAcceptContext. This removes the SafeNativeOverlapped allocation as the lifetime is managed by the AsyncAcceptContext
- Rename tcs

Author: davidfowl

src/Servers/HttpSys/src/AsyncAcceptContext.cs

@@ -13,9 +13,10 @@ internal unsafe class AsyncAcceptContext : IValueTaskSource<RequestContext>, IDi
     {
         private static readonly IOCompletionCallback IOCallback = IOWaitCallback;
         private readonly PreAllocatedOverlapped _preallocatedOverlapped;
+        private NativeOverlapped* _overlapped;
 
         // mutable struct; do not make this readonly
-        private ManualResetValueTaskSourceCore<RequestContext> _tcs = new()
+        private ManualResetValueTaskSourceCore<RequestContext> _mrvts = new()
         {
             // We want to run continuations on the IO threads
             RunContinuationsAsynchronously = false
@@ -33,7 +34,7 @@ internal AsyncAcceptContext(HttpSysListener server)
 
         internal ValueTask<RequestContext> AcceptAsync()
         {
-            _tcs.Reset();
+            _mrvts.Reset();
 
             AllocateNativeRequest();
 
@@ -46,7 +47,7 @@ internal ValueTask<RequestContext> AcceptAsync()
                 return ValueTask.FromException<RequestContext>(new HttpSysException((int)statusCode));
             }
 
-            return new ValueTask<RequestContext>(this, _tcs.Version);
+            return new ValueTask<RequestContext>(this, _mrvts.Version);
         }
 
         private static void IOCompleted(AsyncAcceptContext asyncContext, uint errorCode, uint numBytes)
@@ -58,7 +59,7 @@ private static void IOCompleted(AsyncAcceptContext asyncContext, uint errorCode,
                 if (errorCode != UnsafeNclNativeMethods.ErrorCodes.ERROR_SUCCESS &&
                     errorCode != UnsafeNclNativeMethods.ErrorCodes.ERROR_MORE_DATA)
                 {
-                    asyncContext._tcs.SetException(new HttpSysException((int)errorCode));
+                    asyncContext._mrvts.SetException(new HttpSysException((int)errorCode));
                     return;
                 }
 
@@ -78,14 +79,14 @@ private static void IOCompleted(AsyncAcceptContext asyncContext, uint errorCode,
                             asyncContext._nativeRequestContext = null;
 
                             var requestContext = new RequestContext(server, nativeContext);
-                            asyncContext._tcs.SetResult(requestContext);
+                            asyncContext._mrvts.SetResult(requestContext);
 
                             complete = true;
                         }
                     }
                     catch (Exception ex)
                     {
-                        asyncContext._tcs.SetException(ex);
+                        asyncContext._mrvts.SetException(ex);
                     }
                     finally
                     {
@@ -111,13 +112,13 @@ private static void IOCompleted(AsyncAcceptContext asyncContext, uint errorCode,
                     {
                         // someother bad error, possible(?) return values are:
                         // ERROR_INVALID_HANDLE, ERROR_INSUFFICIENT_BUFFER, ERROR_OPERATION_ABORTED
-                        asyncContext._tcs.SetException(new HttpSysException((int)statusCode));
+                        asyncContext._mrvts.SetException(new HttpSysException((int)statusCode));
                     }
                 }
             }
             catch (Exception exception)
             {
-                asyncContext._tcs.SetException(exception);
+                asyncContext._mrvts.SetException(exception);
             }
         }
 
@@ -144,7 +145,7 @@ private uint QueueBeginGetContext()
                     _nativeRequestContext.NativeRequest,
                     _nativeRequestContext.Size,
                     &bytesTransferred,
-                    _nativeRequestContext.NativeOverlapped);
+                    _overlapped);
 
                 if (statusCode == UnsafeNclNativeMethods.ErrorCodes.ERROR_INVALID_PARAMETER && _nativeRequestContext.RequestId != 0)
                 {
@@ -179,10 +180,14 @@ private void AllocateNativeRequest(uint? size = null, ulong requestId = 0)
             _nativeRequestContext?.Dispose();
 
             var boundHandle = Server.RequestQueue.BoundHandle;
-            var nativeOverlapped = new SafeNativeOverlapped(boundHandle,
-                boundHandle.AllocateNativeOverlapped(_preallocatedOverlapped));
 
-            _nativeRequestContext = new NativeRequestContext(nativeOverlapped, Server.MemoryPool, size, requestId);
+            if (_overlapped != null)
+            {
+                boundHandle.FreeNativeOverlapped(_overlapped);
+            }
+
+            _nativeRequestContext = new NativeRequestContext(Server.MemoryPool, size, requestId);
+            _overlapped = boundHandle.AllocateNativeOverlapped(_preallocatedOverlapped);
         }
 
         public void Dispose()
@@ -199,23 +204,31 @@ protected virtual void Dispose(bool disposing)
                     _nativeRequestContext.ReleasePins();
                     _nativeRequestContext.Dispose();
                     _nativeRequestContext = null;
+
+                    var boundHandle = Server.RequestQueue.BoundHandle;
+
+                    if (_overlapped != null)
+                    {
+                        boundHandle.FreeNativeOverlapped(_overlapped);
+                        _overlapped = null;
+                    }
                 }
             }
         }
 
         public RequestContext GetResult(short token)
         {
-            return _tcs.GetResult(token);
+            return _mrvts.GetResult(token);
         }
 
         public ValueTaskSourceStatus GetStatus(short token)
         {
-            return _tcs.GetStatus(token);
+            return _mrvts.GetStatus(token);
         }
 
         public void OnCompleted(Action<object> continuation, object state, short token, ValueTaskSourceOnCompletedFlags flags)
         {
-            _tcs.OnCompleted(continuation, state, token, flags);
+            _mrvts.OnCompleted(continuation, state, token, flags);
         }
     }
 }

src/Servers/HttpSys/src/NativeInterop/HttpApi.cs

@@ -3,6 +3,7 @@
 
 using System;
 using System.Runtime.InteropServices;
+using System.Threading;
 using Microsoft.AspNetCore.HttpSys.Internal;
 using static Microsoft.AspNetCore.HttpSys.Internal.HttpApiTypes;
 
@@ -25,7 +26,7 @@ internal static unsafe class HttpApi
         internal static extern uint HttpReceiveClientCertificate(SafeHandle requestQueueHandle, ulong connectionId, uint flags, byte* pSslClientCertInfo, uint sslClientCertInfoSize, uint* pBytesReceived, SafeNativeOverlapped pOverlapped);
 
         [DllImport(HTTPAPI, ExactSpelling = true, CallingConvention = CallingConvention.StdCall, SetLastError = true)]
-        internal static extern uint HttpReceiveHttpRequest(SafeHandle requestQueueHandle, ulong requestId, uint flags, HTTP_REQUEST* pRequestBuffer, uint requestBufferLength, uint* pBytesReturned, SafeNativeOverlapped pOverlapped);
+        internal static extern uint HttpReceiveHttpRequest(SafeHandle requestQueueHandle, ulong requestId, uint flags, HTTP_REQUEST* pRequestBuffer, uint requestBufferLength, uint* pBytesReturned, NativeOverlapped* pOverlapped);
 
         [DllImport(HTTPAPI, ExactSpelling = true, CallingConvention = CallingConvention.StdCall, SetLastError = true)]
         internal static extern uint HttpSendHttpResponse(SafeHandle requestQueueHandle, ulong requestId, uint flags, HTTP_RESPONSE_V2* pHttpResponse, HTTP_CACHE_POLICY* pCachePolicy, uint* pBytesSent, IntPtr pReserved1, uint Reserved2, SafeNativeOverlapped pOverlapped, IntPtr pLogData);

src/Shared/HttpSys/RequestProcessing/NativeRequestContext.cs

@@ -25,15 +25,12 @@ internal unsafe class NativeRequestContext : IDisposable
         private IMemoryOwner<byte> _backingBuffer;
         private MemoryHandle _memoryHandle;
         private int _bufferAlignment;
-        private SafeNativeOverlapped _nativeOverlapped;
         private bool _permanentlyPinned;
         private bool _disposed;
 
         // To be used by HttpSys
-        internal NativeRequestContext(SafeNativeOverlapped nativeOverlapped, MemoryPool<Byte> memoryPool, uint? bufferSize, ulong requestId)
+        internal NativeRequestContext(MemoryPool<byte> memoryPool, uint? bufferSize, ulong requestId)
         {
-            _nativeOverlapped = nativeOverlapped;
-
             // TODO:
             // Apparently the HttpReceiveHttpRequest memory alignment requirements for non - ARM processors
             // are different than for ARM processors. We have seen 4 - byte - aligned buffers allocated on
@@ -70,8 +67,6 @@ internal NativeRequestContext(HttpApiTypes.HTTP_REQUEST* request, bool useLatin1
             _permanentlyPinned = true;
         }
 
-        internal SafeNativeOverlapped NativeOverlapped => _nativeOverlapped;
-
         internal HttpApiTypes.HTTP_REQUEST* NativeRequest
         {
             get
@@ -130,8 +125,6 @@ internal void ReleasePins()
             _memoryHandle.Dispose();
             _memoryHandle = default;
             _nativeRequest = null;
-            _nativeOverlapped?.Dispose();
-            _nativeOverlapped = null;
         }
 
         public virtual void Dispose()
@@ -140,7 +133,6 @@ public virtual void Dispose()
             {
                 _disposed = true;
                 Debug.Assert(_nativeRequest == null, "RequestContextBase::Dispose()|Dispose() called before ReleasePins().");
-                _nativeOverlapped?.Dispose();
                 _memoryHandle.Dispose();
                 _backingBuffer.Dispose();
             }