Clean up some http.sys code (#28308)

* Clean up some http.sys code
- Remove SuppressContext and ToIAsyncResult helpers.
Use TaskToApm instead.
- Dispatch off the current thread to start the accept loop
- Use logging pattern in ResponseBody
- Replace Contract.Assert with Debug.Assert
- Use new ValidateBufferArguments to verify WriteAsync overloads

* Update src/Servers/HttpSys/src/RequestProcessing/ResponseBody.cs

Co-authored-by: James Newton-King <[email protected]>

* Update src/Servers/HttpSys/src/RequestProcessing/ResponseBody.cs

Co-authored-by: James Newton-King <[email protected]>

* Fix one of the async voids

Co-authored-by: James Newton-King <[email protected]>

Author: davidfowl

src/Servers/HttpSys/src/Helpers.cs

@@ -3,10 +3,7 @@
 
 using System;
 using System.Globalization;
-using System.Runtime.CompilerServices;
 using System.Text;
-using System.Threading;
-using System.Threading.Tasks;
 
 namespace Microsoft.AspNetCore.Server.HttpSys
 {
@@ -15,42 +12,6 @@ internal static class Helpers
         internal static readonly byte[] ChunkTerminator = new byte[] { (byte)'0', (byte)'\r', (byte)'\n', (byte)'\r', (byte)'\n' };
         internal static readonly byte[] CRLF = new byte[] { (byte)'\r', (byte)'\n' };
 
-        internal static ConfiguredTaskAwaitable SupressContext(this Task task)
-        {
-            return task.ConfigureAwait(continueOnCapturedContext: false);
-        }
-
-        internal static ConfiguredTaskAwaitable<T> SupressContext<T>(this Task<T> task)
-        {
-            return task.ConfigureAwait(continueOnCapturedContext: false);
-        }
-
-        internal static IAsyncResult ToIAsyncResult(this Task task, AsyncCallback callback, object state)
-        {
-            var tcs = new TaskCompletionSource<int>(state);
-            task.ContinueWith(t =>
-            {
-                if (t.IsFaulted)
-                {
-                    tcs.TrySetException(t.Exception.InnerExceptions);
-                }
-                else if (t.IsCanceled)
-                {
-                    tcs.TrySetCanceled();
-                }
-                else
-                {
-                    tcs.TrySetResult(0);
-                }
-
-                if (callback != null)
-                {
-                    callback(tcs.Task);
-                }
-            }, CancellationToken.None, TaskContinuationOptions.None, TaskScheduler.Default);
-            return tcs.Task;
-        }
-
         internal static ArraySegment<byte> GetChunkHeader(long size)
         {
             if (size < int.MaxValue)

src/Servers/HttpSys/src/LoggerEventIds.cs

@@ -50,9 +50,5 @@ internal static class LoggerEventIds
         public static EventId DisconnectTriggered = new EventId(44, "DisconnectTriggered");
         public static EventId ListenerStopError = new EventId(45, "ListenerStopError");
         public static EventId ListenerDisposing = new EventId(46, "ListenerDisposing");
-    
-   
-
-      
     }
 }

src/Servers/HttpSys/src/MessagePump.cs

@@ -3,7 +3,7 @@
 
 using System;
 using System.Collections.Generic;
-using System.Diagnostics.Contracts;
+using System.Diagnostics;
 using System.Linq;
 using System.Threading;
 using System.Threading.Tasks;
@@ -119,9 +119,9 @@ public Task StartAsync<TContext>(IHttpApplication<TContext> application, Cancell
             // else // Attaching to an existing queue, don't add a default.
 
             // Can't call Start twice
-            Contract.Assert(Application == null);
+            Debug.Assert(Application == null);
 
-            Contract.Assert(application != null);
+            Debug.Assert(application != null);
 
             Application = new ApplicationWrapper<TContext>(application);
 
@@ -134,7 +134,8 @@ public Task StartAsync<TContext>(IHttpApplication<TContext> application, Cancell
                 _serverAddresses.Addresses.Add(prefix.FullPrefix);
             }
 
-            ActivateRequestProcessingLimits();
+            // Dispatch to get off the SynchronizationContext and use UnsafeQueueUserWorkItem to avoid capturing the ExecutionContext
+            ThreadPool.UnsafeQueueUserWorkItem(state => state.ActivateRequestProcessingLimits(), this, preferLocal: false);
 
             return Task.CompletedTask;
         }
@@ -143,7 +144,8 @@ private void ActivateRequestProcessingLimits()
         {
             for (int i = _acceptorCounts; i < _maxAccepts; i++)
             {
-                ProcessRequestsWorker();
+                // Ignore the result
+                _ = ProcessRequestsWorker();
             }
         }
 
@@ -174,7 +176,7 @@ internal void SetShutdownSignal()
         // When we start listening for the next request on one thread, we may need to be sure that the
         // completion continues on another thread as to not block the current request processing.
         // The awaits will manage stack depth for us.
-        private async void ProcessRequestsWorker()
+        private async Task ProcessRequestsWorker()
         {
             int workerIndex = Interlocked.Increment(ref _acceptorCounts);
             while (!Stopping && workerIndex <= _maxAccepts)
@@ -183,13 +185,13 @@ private async void ProcessRequestsWorker()
                 RequestContext requestContext;
                 try
                 {
-                    requestContext = await Listener.AcceptAsync().SupressContext();
+                    requestContext = await Listener.AcceptAsync();
                     // Assign the message pump to this request context
                     requestContext.MessagePump = this;
                 }
                 catch (Exception exception)
                 {
-                    Contract.Assert(Stopping);
+                    Debug.Assert(Stopping);
                     if (Stopping)
                     {
                         _logger.LogDebug(LoggerEventIds.AcceptErrorStopping, exception, "Failed to accept a request, the server is stopping.");

src/Servers/HttpSys/src/RequestProcessing/Request.cs

@@ -370,7 +370,7 @@ public X509Certificate2 ClientCertificate
             var certLoader = new ClientCertLoader(RequestContext, cancellationToken);
             try
             {
-                await certLoader.LoadClientCertificateAsync().SupressContext();
+                await certLoader.LoadClientCertificateAsync();
                 // Populate the environment.
                 if (certLoader.ClientCert != null)
                 {

src/Servers/HttpSys/src/RequestProcessing/RequestContext.cs

@@ -262,7 +262,7 @@ public async void Execute()
                     context = application.CreateContext(featureContext.Features);
                     try
                     {
-                        await application.ProcessRequestAsync(context).SupressContext();
+                        await application.ProcessRequestAsync(context);
                         await featureContext.CompleteAsync();
                     }
                     finally

src/Servers/HttpSys/src/RequestProcessing/ResponseBody.cs

@@ -4,9 +4,7 @@
 using System;
 using System.Collections.Generic;
 using System.Diagnostics;
-using System.Diagnostics.CodeAnalysis;
 using System.IO;
-using System.Net;
 using System.Runtime.InteropServices;
 using System.Threading;
 using System.Threading.Tasks;
@@ -135,7 +133,7 @@ public void MarkDelegated()
                 if (!RequestContext.DisconnectToken.IsCancellationRequested)
                 {
                     // This is logged rather than thrown because it is too late for an exception to be visible in user code.
-                    Logger.LogError(LoggerEventIds.FewerBytesThanExpected, "ResponseStream::Dispose; Fewer bytes were written than were specified in the Content-Length.");
+                    Log.FewerBytesThanExpected(Logger);
                 }
                 _requestContext.Abort();
                 return;
@@ -180,14 +178,14 @@ public void MarkDelegated()
                 if (ThrowWriteExceptions)
                 {
                     var exception = new IOException(string.Empty, new HttpSysException((int)statusCode));
-                    Logger.LogError(LoggerEventIds.WriteError, exception, "Flush");
+                    Log.WriteError(Logger, exception);
                     Abort();
                     throw exception;
                 }
                 else
                 {
                     // Abort the request but do not close the stream, let future writes complete silently
-                    Logger.LogDebug(LoggerEventIds.WriteErrorIgnored, $"Flush; Ignored write exception: {statusCode}");
+                    Log.WriteErrorIgnored(Logger, statusCode);
                     Abort(dispose: false);
                 }
             }
@@ -350,7 +348,7 @@ private unsafe Task FlushInternalAsync(ArraySegment<byte> data, CancellationToke
             }
             catch (Exception e)
             {
-                Logger.LogError(LoggerEventIds.ErrorWhenFlushAsync, e, "FlushAsync");
+                Log.ErrorWhenFlushAsync(Logger, e);
                 asyncResult.Dispose();
                 Abort();
                 throw;
@@ -360,21 +358,21 @@ private unsafe Task FlushInternalAsync(ArraySegment<byte> data, CancellationToke
             {
                 if (cancellationToken.IsCancellationRequested)
                 {
-                    Logger.LogDebug(LoggerEventIds.WriteFlushCancelled,$"FlushAsync; Write cancelled with error code: {statusCode}");
+                    Log.WriteFlushCancelled(Logger, statusCode);
                     asyncResult.Cancel(ThrowWriteExceptions);
                 }
                 else if (ThrowWriteExceptions)
                 {
                     asyncResult.Dispose();
                     Exception exception = new IOException(string.Empty, new HttpSysException((int)statusCode));
-                    Logger.LogError(LoggerEventIds.ErrorWhenFlushAsync, exception, "FlushAsync");
+                    Log.ErrorWhenFlushAsync(Logger, exception);
                     Abort();
                     throw exception;
                 }
                 else
                 {
                     // Abort the request but do not close the stream, let future writes complete silently
-                    Logger.LogDebug(LoggerEventIds.WriteErrorIgnored,$"FlushAsync; Ignored write exception: {statusCode}");
+                    Log.WriteErrorIgnored(Logger, statusCode);
                     asyncResult.FailSilently();
                 }
             }
@@ -488,6 +486,8 @@ private HttpApiTypes.HTTP_FLAGS ComputeLeftToWrite(long writeCount, bool endOfRe
 
         public override void Write(byte[] buffer, int offset, int count)
         {
+            ValidateBufferArguments(buffer, offset, count);
+
             if (!RequestContext.AllowSynchronousIO)
             {
                 throw new InvalidOperationException("Synchronous IO APIs are disabled, see AllowSynchronousIO.");
@@ -522,7 +522,7 @@ private void CheckWriteCount(long? count)
 
         public override IAsyncResult BeginWrite(byte[] buffer, int offset, int count, AsyncCallback callback, object state)
         {
-            return WriteAsync(buffer, offset, count).ToIAsyncResult(callback, state);
+            return TaskToApm.Begin(WriteAsync(buffer, offset, count), callback, state);
         }
 
         public override void EndWrite(IAsyncResult asyncResult)
@@ -531,11 +531,14 @@ public override void EndWrite(IAsyncResult asyncResult)
             {
                 throw new ArgumentNullException(nameof(asyncResult));
             }
-            ((Task)asyncResult).GetAwaiter().GetResult();
+
+            TaskToApm.End(asyncResult);
         }
 
         public override Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken)
         {
+            ValidateBufferArguments(buffer, offset, count);
+
             // Validates for null and bounds. Allows count == 0.
             // TODO: Verbose log parameters
             var data = new ArraySegment<byte>(buffer, offset, count);
@@ -644,7 +647,7 @@ internal unsafe Task SendFileAsyncCore(string fileName, long offset, long? count
             }
             catch (Exception e)
             {
-                Logger.LogError(LoggerEventIds.FileSendAsyncError, e, "SendFileAsync");
+                Log.FileSendAsyncError(Logger, e);
                 asyncResult.Dispose();
                 Abort();
                 throw;
@@ -654,21 +657,21 @@ internal unsafe Task SendFileAsyncCore(string fileName, long offset, long? count
             {
                 if (cancellationToken.IsCancellationRequested)
                 {
-                    Logger.LogDebug(LoggerEventIds.FileSendAsyncCancelled,$"SendFileAsync; Write cancelled with error code: {statusCode}");
+                    Log.FileSendAsyncCancelled(Logger, statusCode);
                     asyncResult.Cancel(ThrowWriteExceptions);
                 }
                 else if (ThrowWriteExceptions)
                 {
                     asyncResult.Dispose();
                     var exception = new IOException(string.Empty, new HttpSysException((int)statusCode));
-                    Logger.LogError(LoggerEventIds.FileSendAsyncError, exception, "SendFileAsync");
+                    Log.FileSendAsyncError(Logger, exception);
                     Abort();
                     throw exception;
                 }
                 else
                 {
-                    // Abort the request but do not close the stream, let future writes complete silently
-                    Logger.LogDebug(LoggerEventIds.FileSendAsyncErrorIgnored,$"SendFileAsync; Ignored write exception: {statusCode}");
+                    // Abort the request but do not close the stream, let future writes complete 
+                    Log.FileSendAsyncErrorIgnored(Logger, statusCode);
                     asyncResult.FailSilently();
                 }
             }
@@ -715,8 +718,6 @@ internal void SwitchToOpaqueMode()
 
         // The final Content-Length async write can only be Canceled by CancelIoEx.
         // Sync can only be Canceled by CancelSynchronousIo, but we don't attempt this right now.
-        [SuppressMessage("Microsoft.Usage", "CA1806:DoNotIgnoreMethodResults", Justification =
-            "It is safe to ignore the return value on a cancel operation because the connection is being closed")]
         internal unsafe void CancelLastWrite()
         {
             ResponseStreamAsyncResult asyncState = _lastWrite;
@@ -733,5 +734,72 @@ private void CheckDisposed()
                 throw new ObjectDisposedException(GetType().FullName);
             }
         }
+
+        private static class Log
+        {
+            private static readonly Action<ILogger, Exception> _fewerBytesThanExpected =
+                LoggerMessage.Define(LogLevel.Error, LoggerEventIds.FewerBytesThanExpected, "ResponseStream::Dispose; Fewer bytes were written than were specified in the Content-Length.");
+
+            private static readonly Action<ILogger, Exception> _writeError =
+                LoggerMessage.Define(LogLevel.Error, LoggerEventIds.WriteError, "Flush");
+
+            private static readonly Action<ILogger, uint, Exception> _writeErrorIgnored =
+                LoggerMessage.Define<uint>(LogLevel.Debug, LoggerEventIds.WriteErrorIgnored, "Flush; Ignored write exception: {StatusCode}");
+
+            private static readonly Action<ILogger, Exception> _errorWhenFlushAsync =
+                LoggerMessage.Define(LogLevel.Debug, LoggerEventIds.ErrorWhenFlushAsync, "FlushAsync");
+
+            private static readonly Action<ILogger, uint, Exception> _writeFlushCancelled =
+                LoggerMessage.Define<uint>(LogLevel.Debug, LoggerEventIds.WriteFlushCancelled, "FlushAsync; Write cancelled with error code: {StatusCode}");
+
+            private static readonly Action<ILogger, Exception> _fileSendAsyncError =
+                LoggerMessage.Define(LogLevel.Error, LoggerEventIds.FileSendAsyncError, "SendFileAsync");
+
+            private static readonly Action<ILogger, uint, Exception> _fileSendAsyncCancelled =
+                LoggerMessage.Define<uint>(LogLevel.Debug, LoggerEventIds.FileSendAsyncCancelled, "SendFileAsync; Write cancelled with error code: {StatusCode}");
+
+            private static readonly Action<ILogger, uint, Exception> _fileSendAsyncErrorIgnored =
+                LoggerMessage.Define<uint>(LogLevel.Debug, LoggerEventIds.FileSendAsyncErrorIgnored, "SendFileAsync; Ignored write exception: {StatusCode}");
+
+            public static void FewerBytesThanExpected(ILogger logger)
+            {
+                _fewerBytesThanExpected(logger, null);
+            }
+
+            public static void WriteError(ILogger logger, IOException exception)
+            {
+                _writeError(logger, exception);
+            }
+
+            public static void WriteErrorIgnored(ILogger logger, uint statusCode)
+            {
+                _writeErrorIgnored(logger, statusCode, null);
+            }
+
+            public static void ErrorWhenFlushAsync(ILogger logger, Exception exception)
+            {
+                _errorWhenFlushAsync(logger, exception);
+            }
+
+            public static void WriteFlushCancelled(ILogger logger, uint statusCode)
+            {
+                _writeFlushCancelled(logger, statusCode, null);
+            }
+
+            public static void FileSendAsyncError(ILogger logger, Exception exception)
+            {
+                _fileSendAsyncError(logger, exception);
+            }
+
+            public static void FileSendAsyncCancelled(ILogger logger, uint statusCode)
+            {
+                _fileSendAsyncCancelled(logger, statusCode, null);
+            }
+
+            public static void FileSendAsyncErrorIgnored(ILogger logger, uint statusCode)
+            {
+                _fileSendAsyncErrorIgnored(logger, statusCode, null);
+            }
+        }
     }
 }