Log exceptions from asynchronously failing Expect: 100-continue sends (#37391)

* Log exceptions from asynchronously failing Expect: 100-continue sends

When Expect: 100-continue is used, the sending of the request body content is allowed to run concurrently with the receipt of the response headers.  If the processing of the response headers encounters an exception, we currently fail to ever join with that send task, which results in a TaskScheduler.UnobservedTaskException event.  This PR changes to observe the exception in such cases and log it.

(In doing so, I also came across an async void method, and changed all of the remaining ones in the assembly to be async Task instead.  The current implementation of async void isn't any cheaper than async Task, and is actually more expensive in certain ways, plus it unnecessarily interacts with any SynchronizationContext that may exist.)

* Address PR feedback

(cherry picked from commit 0bc3ee4a4f1debaa0dd3c957d5201ae2a47757a1)

3 files changed, 36 insertions, 8 deletions

diff --git a/src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs b/src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs
index 78c1201faf..861f2951e3 100644
--- a/src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs
+++ b/src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs
@@ -554,7 +554,7 @@ namespace System.Net.Http
             Task.Factory.StartNew(
                 s => {
                     var whrs = (WinHttpRequestState)s;
-                    whrs.Handler.StartRequest(whrs);
+                    _ = whrs.Handler.StartRequestAsync(whrs);
                 },
                 state,
                 CancellationToken.None,
@@ -764,7 +764,7 @@ namespace System.Net.Http
             }
         }
 
-        private async void StartRequest(WinHttpRequestState state)
+        private async Task StartRequestAsync(WinHttpRequestState state)
         {
             if (state.CancellationToken.IsCancellationRequested)
             {
diff --git a/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs b/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs
index 730e4f9a22..9701716f1a 100644
--- a/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs
+++ b/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs
@@ -123,18 +123,31 @@ namespace System.Net.Http
                     ValueTask<int>? readAheadTask = ConsumeReadAheadTask();
                     if (readAheadTask != null)
                     {
-                        IgnoreExceptionsAsync(readAheadTask.GetValueOrDefault());
+                        _ = IgnoreExceptionsAsync(readAheadTask.GetValueOrDefault());
                     }
                 }
             }
         }
 
         /// <summary>Awaits a task, ignoring any resulting exceptions.</summary>
-        private static async void IgnoreExceptionsAsync(ValueTask<int> task)
+        private static async Task IgnoreExceptionsAsync(ValueTask<int> task)
         {
             try { await task.ConfigureAwait(false); } catch { }
         }
 
+        /// <summary>Awaits a task, logging any resulting exceptions (which are otherwise ignored).</summary>
+        private async Task LogExceptionsAsync(Task task)
+        {
+            try
+            {
+                await task.ConfigureAwait(false);
+            }
+            catch (Exception e)
+            {
+                if (NetEventSource.IsEnabled) Trace($"Exception from asynchronous processing: {e}");
+            }
+        }
+
         /// <summary>Do a non-blocking poll to see whether the connection has data available or has been closed.</summary>
         /// <remarks>If we don't have direct access to the underlying socket, we instead use a read-ahead task.</remarks>
         public bool PollRead()
@@ -356,6 +369,7 @@ namespace System.Net.Http
         public async Task<HttpResponseMessage> SendAsyncCore(HttpRequestMessage request, CancellationToken cancellationToken)
         {
             TaskCompletionSource<bool> allowExpect100ToContinue = null;
+            Task sendRequestContentTask = null;
             Debug.Assert(_currentRequest == null, $"Expected null {nameof(_currentRequest)}.");
             Debug.Assert(RemainingBuffer.Length == 0, "Unexpected data in read buffer");
 
@@ -462,7 +476,6 @@ namespace System.Net.Http
                 // CRLF for end of headers.
                 await WriteTwoBytesAsync((byte)'\r', (byte)'\n').ConfigureAwait(false);
 
-                Task sendRequestContentTask = null;
                 if (request.Content == null)
                 {
                     // We have nothing more to send, so flush out any headers we haven't yet sent.
@@ -574,8 +587,9 @@ namespace System.Net.Http
                 // content has been received, so this task should generally already be complete.
                 if (sendRequestContentTask != null)
                 {
-                    await sendRequestContentTask.ConfigureAwait(false);
+                    Task sendTask = sendRequestContentTask;
                     sendRequestContentTask = null;
+                    await sendTask.ConfigureAwait(false);
                 }
 
                 // Parse the response headers.
@@ -665,6 +679,20 @@ namespace System.Net.Http
                 allowExpect100ToContinue?.TrySetResult(false);
 
                 if (NetEventSource.IsEnabled) Trace($"Error sending request: {error}");
+
+                // In the rare case where Expect: 100-continue was used and then processing
+                // of the response headers encountered an error such that we weren't able to
+                // wait for the sending to complete, it's possible the sending also encountered
+                // an exception or potentially is still going and will encounter an exception
+                // (we're about to Dispose for the connection). In such cases, we don't want any
+                // exception in that sending task to become unobserved and raise alarm bells, so we
+                // hook up a continuation that will log it.
+                if (sendRequestContentTask != null && !sendRequestContentTask.IsCompletedSuccessfully)
+                {
+                    _ = LogExceptionsAsync(sendRequestContentTask);
+                }
+
+                // Now clean up the connection.
                 Dispose();
 
                 // At this point, we're going to throw an exception; we just need to
diff --git a/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpContentReadStream.cs b/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpContentReadStream.cs
index d1c44340d8..cc50a457ac 100644
--- a/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpContentReadStream.cs
+++ b/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpContentReadStream.cs
@@ -74,14 +74,14 @@ namespace System.Net.Http
                     // Start the asynchronous drain.
                     // It may complete synchronously, in which case the connection will be put back in the pool synchronously.
                     // Skip the call to base.Dispose -- it will be deferred until DrainOnDisposeAsync finishes.
-                    DrainOnDisposeAsync();
+                    _ = DrainOnDisposeAsync();
                     return;
                 }
 
                 base.Dispose(disposing);
             }
 
-            private async void DrainOnDisposeAsync()
+            private async Task DrainOnDisposeAsync()
             {
                 HttpConnection connection = _connection;        // Will be null after drain succeeds