Make the HTTP.sys accept loop cheaper (#28345)

* Make the HTTP.sys accept loop cheaper
- Allocate a single AsyncAcceptContext per loop instead of one per request.
- The AsyncAcceptContext now implements IValueTaskSource instead of
Task and doesn't allocate a Task per request.
- Use a preallocated overlapped since we don't have overlapping calls to accept for a single AsyncAcceptContext.
- Remove some dead code from SafeNativeOverlapped

* Clear the nativeRequestContext on dispose

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

* Send the error response before settings the result

Author: davidfowl

src/Servers/HttpSys/HttpSysServer.slnf

@@ -1,29 +1,29 @@
-{
+{
   "solution": {
     "path": "..\\..\\..\\AspNetCore.sln",
-    "projects" : [
-      "src\\Servers\\HttpSys\\samples\\TestClient\\TestClient.csproj",
-      "src\\Servers\\HttpSys\\samples\\SelfHostServer\\SelfHostServer.csproj",
-      "src\\Servers\\HttpSys\\samples\\HotAddSample\\HotAddSample.csproj",
-      "src\\Servers\\HttpSys\\test\\FunctionalTests\\Microsoft.AspNetCore.Server.HttpSys.FunctionalTests.csproj",
-      "src\\Servers\\HttpSys\\test\\Tests\\Microsoft.AspNetCore.Server.HttpSys.Tests.csproj",
-      "src\\Servers\\HttpSys\\src\\Microsoft.AspNetCore.Server.HttpSys.csproj",
+    "projects": [
+      "src\\DefaultBuilder\\src\\Microsoft.AspNetCore.csproj",
+      "src\\Hosting\\Abstractions\\src\\Microsoft.AspNetCore.Hosting.Abstractions.csproj",
+      "src\\Hosting\\Hosting\\src\\Microsoft.AspNetCore.Hosting.csproj",
       "src\\Hosting\\Server.Abstractions\\src\\Microsoft.AspNetCore.Hosting.Server.Abstractions.csproj",
       "src\\Http\\Authentication.Abstractions\\src\\Microsoft.AspNetCore.Authentication.Abstractions.csproj",
       "src\\Http\\Authentication.Core\\src\\Microsoft.AspNetCore.Authentication.Core.csproj",
       "src\\Http\\Headers\\src\\Microsoft.Net.Http.Headers.csproj",
-      "src\\Http\\Http\\src\\Microsoft.AspNetCore.Http.csproj",
       "src\\Http\\Http.Abstractions\\src\\Microsoft.AspNetCore.Http.Abstractions.csproj",
       "src\\Http\\Http.Extensions\\src\\Microsoft.AspNetCore.Http.Extensions.csproj",
       "src\\Http\\Http.Features\\src\\Microsoft.AspNetCore.Http.Features.csproj",
+      "src\\Http\\Http\\src\\Microsoft.AspNetCore.Http.csproj",
       "src\\Http\\Metadata\\src\\Microsoft.AspNetCore.Metadata.csproj",
       "src\\Http\\WebUtilities\\src\\Microsoft.AspNetCore.WebUtilities.csproj",
-      "src\\Hosting\\Abstractions\\src\\Microsoft.AspNetCore.Hosting.Abstractions.csproj",
-      "src\\Hosting\\Hosting\\src\\Microsoft.AspNetCore.Hosting.csproj",
       "src\\Servers\\Connections.Abstractions\\src\\Microsoft.AspNetCore.Connections.Abstractions.csproj",
+      "src\\Servers\\HttpSys\\samples\\HotAddSample\\HotAddSample.csproj",
       "src\\Servers\\HttpSys\\samples\\QueueSharing\\QueueSharing.csproj",
-      "src\\Servers\\Kestrel\\Transport.Sockets\\src\\Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets.csproj",
-      "src\\DefaultBuilder\\src\\Microsoft.AspNetCore.csproj"
+      "src\\Servers\\HttpSys\\samples\\SelfHostServer\\SelfHostServer.csproj",
+      "src\\Servers\\HttpSys\\samples\\TestClient\\TestClient.csproj",
+      "src\\Servers\\HttpSys\\src\\Microsoft.AspNetCore.Server.HttpSys.csproj",
+      "src\\Servers\\HttpSys\\test\\FunctionalTests\\Microsoft.AspNetCore.Server.HttpSys.FunctionalTests.csproj",
+      "src\\Servers\\HttpSys\\test\\Tests\\Microsoft.AspNetCore.Server.HttpSys.Tests.csproj",
+      "src\\Servers\\Kestrel\\Transport.Sockets\\src\\Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets.csproj"
     ]
   }
-}
+}

src/Servers/HttpSys/src/AsyncAcceptContext.cs

@@ -2,121 +2,135 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
-using System.Diagnostics.CodeAnalysis;
 using System.Threading;
 using System.Threading.Tasks;
+using System.Threading.Tasks.Sources;
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.HttpSys.Internal;
 
 namespace Microsoft.AspNetCore.Server.HttpSys
 {
-    internal unsafe class AsyncAcceptContext : TaskCompletionSource<RequestContext>, IDisposable
+    internal unsafe class AsyncAcceptContext : IValueTaskSource<RequestContext>, IDisposable
     {
-        internal static readonly IOCompletionCallback IOCallback = new IOCompletionCallback(IOWaitCallback);
+        private static readonly IOCompletionCallback IOCallback = IOWaitCallback;
+        private readonly PreAllocatedOverlapped _preallocatedOverlapped;
+        private NativeOverlapped* _overlapped;
+
+        // mutable struct; do not make this readonly
+        private ManualResetValueTaskSourceCore<RequestContext> _mrvts = new()
+        {
+            // We want to run continuations on the IO threads
+            RunContinuationsAsynchronously = false
+        };
 
         private NativeRequestContext _nativeRequestContext;
 
         internal AsyncAcceptContext(HttpSysListener server)
         {
             Server = server;
-            AllocateNativeRequest();
+            _preallocatedOverlapped = new(IOCallback, state: this, pinData: null);
         }
 
         internal HttpSysListener Server { get; }
 
-        [SuppressMessage("Microsoft.Design", "CA1031:DoNotCatchGeneralExceptionTypes", Justification = "Redirecting to callback")]
-        [SuppressMessage("Microsoft.Reliability", "CA2000:Dispose objects before losing scope", Justification = "Disposed by callback")]
+        internal ValueTask<RequestContext> AcceptAsync()
+        {
+            _mrvts.Reset();
+
+            AllocateNativeRequest();
+
+            uint statusCode = QueueBeginGetContext();
+            if (statusCode != UnsafeNclNativeMethods.ErrorCodes.ERROR_SUCCESS &&
+                statusCode != UnsafeNclNativeMethods.ErrorCodes.ERROR_IO_PENDING)
+            {
+                // some other bad error, possible(?) return values are:
+                // ERROR_INVALID_HANDLE, ERROR_INSUFFICIENT_BUFFER, ERROR_OPERATION_ABORTED
+                return ValueTask.FromException<RequestContext>(new HttpSysException((int)statusCode));
+            }
+
+            return new ValueTask<RequestContext>(this, _mrvts.Version);
+        }
+
         private static void IOCompleted(AsyncAcceptContext asyncContext, uint errorCode, uint numBytes)
         {
             bool complete = false;
+
             try
             {
                 if (errorCode != UnsafeNclNativeMethods.ErrorCodes.ERROR_SUCCESS &&
                     errorCode != UnsafeNclNativeMethods.ErrorCodes.ERROR_MORE_DATA)
                 {
-                    asyncContext.TrySetException(new HttpSysException((int)errorCode));
-                    complete = true;
+                    asyncContext._mrvts.SetException(new HttpSysException((int)errorCode));
+                    return;
                 }
-                else
+
+                HttpSysListener server = asyncContext.Server;
+                if (errorCode == UnsafeNclNativeMethods.ErrorCodes.ERROR_SUCCESS)
                 {
-                    HttpSysListener server = asyncContext.Server;
-                    if (errorCode == UnsafeNclNativeMethods.ErrorCodes.ERROR_SUCCESS)
+                    // at this point we have received an unmanaged HTTP_REQUEST and memoryBlob
+                    // points to it we need to hook up our authentication handling code here.
+                    try
                     {
-                        // at this point we have received an unmanaged HTTP_REQUEST and memoryBlob
-                        // points to it we need to hook up our authentication handling code here.
-                        try
-                        {
-                            if (server.ValidateRequest(asyncContext._nativeRequestContext) && server.ValidateAuth(asyncContext._nativeRequestContext))
-                            {
-                                RequestContext requestContext = new RequestContext(server, asyncContext._nativeRequestContext);
-                                asyncContext.TrySetResult(requestContext);
-                                complete = true;
-                            }
-                        }
-                        catch (Exception)
-                        {
-                            server.SendError(asyncContext._nativeRequestContext.RequestId, StatusCodes.Status400BadRequest);
-                            throw;
-                        }
-                        finally
+                        var nativeContext = asyncContext._nativeRequestContext;
+
+                        if (server.ValidateRequest(nativeContext) && server.ValidateAuth(nativeContext))
                         {
-                            // The request has been handed to the user, which means this code can't reuse the blob.  Reset it here.
-                            if (complete)
-                            {
-                                asyncContext._nativeRequestContext = null;
-                            }
-                            else
-                            {
-                                asyncContext.AllocateNativeRequest(size: asyncContext._nativeRequestContext.Size);
-                            }
+                            // It's important that we clear the native request context before we set the result
+                            // we want to reuse this object for future accepts.
+                            asyncContext._nativeRequestContext = null;
+
+                            var requestContext = new RequestContext(server, nativeContext);
+                            asyncContext._mrvts.SetResult(requestContext);
+
+                            complete = true;
                         }
                     }
-                    else
+                    catch (Exception ex)
                     {
-                        //  (uint)backingBuffer.Length - AlignmentPadding
-                        asyncContext.AllocateNativeRequest(numBytes, asyncContext._nativeRequestContext.RequestId);
+                        server.SendError(asyncContext._nativeRequestContext.RequestId, StatusCodes.Status400BadRequest);
+                        asyncContext._mrvts.SetException(ex);
                     }
-
-                    // We need to issue a new request, either because auth failed, or because our buffer was too small the first time.
-                    if (!complete)
+                    finally
                     {
-                        uint statusCode = asyncContext.QueueBeginGetContext();
-                        if (statusCode != UnsafeNclNativeMethods.ErrorCodes.ERROR_SUCCESS &&
-                            statusCode != UnsafeNclNativeMethods.ErrorCodes.ERROR_IO_PENDING)
+                        if (!complete)
                         {
-                            // someother bad error, possible(?) return values are:
-                            // ERROR_INVALID_HANDLE, ERROR_INSUFFICIENT_BUFFER, ERROR_OPERATION_ABORTED
-                            asyncContext.TrySetException(new HttpSysException((int)statusCode));
-                            complete = true;
+                            asyncContext.AllocateNativeRequest(size: asyncContext._nativeRequestContext.Size);
                         }
                     }
-                    if (!complete)
-                    {
-                        return;
-                    }
+                }
+                else
+                {
+                    //  (uint)backingBuffer.Length - AlignmentPadding
+                    asyncContext.AllocateNativeRequest(numBytes, asyncContext._nativeRequestContext.RequestId);
                 }
 
-                if (complete)
+                // We need to issue a new request, either because auth failed, or because our buffer was too small the first time.
+                if (!complete)
                 {
-                    asyncContext.Dispose();
+                    uint statusCode = asyncContext.QueueBeginGetContext();
+
+                    if (statusCode != UnsafeNclNativeMethods.ErrorCodes.ERROR_SUCCESS &&
+                        statusCode != UnsafeNclNativeMethods.ErrorCodes.ERROR_IO_PENDING)
+                    {
+                        // someother bad error, possible(?) return values are:
+                        // ERROR_INVALID_HANDLE, ERROR_INSUFFICIENT_BUFFER, ERROR_OPERATION_ABORTED
+                        asyncContext._mrvts.SetException(new HttpSysException((int)statusCode));
+                    }
                 }
             }
             catch (Exception exception)
             {
-                // Logged by caller
-                asyncContext.TrySetException(exception);
-                asyncContext.Dispose();
+                asyncContext._mrvts.SetException(exception);
             }
         }
 
         private static unsafe void IOWaitCallback(uint errorCode, uint numBytes, NativeOverlapped* nativeOverlapped)
         {
-            // take the ListenerAsyncResult object from the state
             var asyncResult = (AsyncAcceptContext)ThreadPoolBoundHandle.GetNativeOverlappedState(nativeOverlapped);
             IOCompleted(asyncResult, errorCode, numBytes);
         }
 
-        internal uint QueueBeginGetContext()
+        private uint QueueBeginGetContext()
         {
             uint statusCode = UnsafeNclNativeMethods.ErrorCodes.ERROR_SUCCESS;
             bool retry;
@@ -133,7 +147,7 @@ internal uint QueueBeginGetContext()
                     _nativeRequestContext.NativeRequest,
                     _nativeRequestContext.Size,
                     &bytesTransferred,
-                    _nativeRequestContext.NativeOverlapped);
+                    _overlapped);
 
                 if (statusCode == UnsafeNclNativeMethods.ErrorCodes.ERROR_INVALID_PARAMETER && _nativeRequestContext.RequestId != 0)
                 {
@@ -162,18 +176,20 @@ internal uint QueueBeginGetContext()
             return statusCode;
         }
 
-        internal void AllocateNativeRequest(uint? size = null, ulong requestId = 0)
+        private void AllocateNativeRequest(uint? size = null, ulong requestId = 0)
         {
             _nativeRequestContext?.ReleasePins();
             _nativeRequestContext?.Dispose();
 
-            // We can't reuse overlapped objects
             var boundHandle = Server.RequestQueue.BoundHandle;
-            var nativeOverlapped = new SafeNativeOverlapped(boundHandle,
-                boundHandle.AllocateNativeOverlapped(IOCallback, this, pinData: null));
 
-            // nativeRequest
-            _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()
@@ -189,8 +205,32 @@ 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 _mrvts.GetResult(token);
+        }
+
+        public ValueTaskSourceStatus GetStatus(short token)
+        {
+            return _mrvts.GetStatus(token);
+        }
+
+        public void OnCompleted(Action<object> continuation, object state, short token, ValueTaskSourceOnCompletedFlags flags)
+        {
+            _mrvts.OnCompleted(continuation, state, token, flags);
+        }
     }
 }

src/Servers/HttpSys/src/HttpSysListener.cs

@@ -195,7 +195,7 @@ private void Stop()
                         return;
                     }
 
-                    Logger.LogTrace(LoggerEventIds.ListenerStopping,"Stopping the listener.");
+                    Logger.LogTrace(LoggerEventIds.ListenerStopping, "Stopping the listener.");
 
                     // If this instance created the queue then remove the URL prefixes before shutting down.
                     if (_requestQueue.Created)
@@ -277,34 +277,12 @@ private void DisposeInternal()
         /// <summary>
         /// Accept a request from the incoming request queue.
         /// </summary>
-        public Task<RequestContext> AcceptAsync()
+        internal ValueTask<RequestContext> AcceptAsync(AsyncAcceptContext acceptContext)
         {
-            AsyncAcceptContext acceptContext = null;
-            try
-            {
-                CheckDisposed();
-                Debug.Assert(_state != State.Stopped, "Listener has been stopped.");
-                // prepare the ListenerAsyncResult object (this will have it's own
-                // event that the user can wait on for IO completion - which means we
-                // need to signal it when IO completes)
-                acceptContext = new AsyncAcceptContext(this);
-                uint statusCode = acceptContext.QueueBeginGetContext();
-                if (statusCode != UnsafeNclNativeMethods.ErrorCodes.ERROR_SUCCESS &&
-                    statusCode != UnsafeNclNativeMethods.ErrorCodes.ERROR_IO_PENDING)
-                {
-                    // some other bad error, possible(?) return values are:
-                    // ERROR_INVALID_HANDLE, ERROR_INSUFFICIENT_BUFFER, ERROR_OPERATION_ABORTED
-                    acceptContext.Dispose();
-                    throw new HttpSysException((int)statusCode);
-                }
-            }
-            catch (Exception exception)
-            {
-                Logger.LogError(LoggerEventIds.AcceptError, exception, "AcceptAsync");
-                throw;
-            }
+            CheckDisposed();
+            Debug.Assert(_state != State.Stopped, "Listener has been stopped.");
 
-            return acceptContext.Task;
+            return acceptContext.AcceptAsync();
         }
 
         internal unsafe bool ValidateRequest(NativeRequestContext requestMemory)

src/Servers/HttpSys/src/MessagePump.cs

@@ -178,14 +178,17 @@ internal void SetShutdownSignal()
         // The awaits will manage stack depth for us.
         private async Task ProcessRequestsWorker()
         {
+            // Allocate and accept context per loop and reuse it for all accepts
+            using var acceptContext = new AsyncAcceptContext(Listener);
+
             int workerIndex = Interlocked.Increment(ref _acceptorCounts);
             while (!Stopping && workerIndex <= _maxAccepts)
             {
                 // Receive a request
                 RequestContext requestContext;
                 try
                 {
-                    requestContext = await Listener.AcceptAsync();
+                    requestContext = await Listener.AcceptAsync(acceptContext);
                     // Assign the message pump to this request context
                     requestContext.MessagePump = this;
                 }

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);