HTTP/3: Improve QUIC stream and HTTP/3 conn completion errors (#35210)

Author: JamesNK

src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs

@@ -357,11 +357,6 @@ public async Task ProcessRequestsAsync<TContext>(IHttpApplication<TContext> appl
             }
             finally
             {
-                // TODO should this message say the connection is shutting down or closing rather than faulted?
-                // Faulted has a negative meaning and we can get here by the host stopping or ConnectionLifeTimeNotification.RequestClose.
-                var connectionError = error as ConnectionAbortedException
-                    ?? new ConnectionAbortedException(CoreStrings.Http3ConnectionFaulted, error!);
-
                 try
                 {
                     // Don't try to send GOAWAY if the client has already closed the connection.
@@ -378,19 +373,21 @@ public async Task ProcessRequestsAsync<TContext>(IHttpApplication<TContext> appl
                     {
                         foreach (var stream in _streams.Values)
                         {
-                            stream.Abort(connectionError, (Http3ErrorCode)_errorCodeFeature.Error);
+                            stream.Abort(CreateConnectionAbortError(error, clientAbort), (Http3ErrorCode)_errorCodeFeature.Error);
                         }
                     }
 
-                    // Complete outbound control stream.
                     if (outboundControlStream != null)
                     {
-                        await outboundControlStream.CompleteAsync();
+                        // Don't gracefully close the outbound control stream. If the peer detects
+                        // the control stream closes it will close with a procotol error.
+                        // Instead, allow control stream to be automatically aborted when the
+                        // connection is aborted.
                         await outboundControlStreamTask;
                     }
 
                     // Complete
-                    Abort(connectionError, (Http3ErrorCode)_errorCodeFeature.Error);
+                    Abort(CreateConnectionAbortError(error, clientAbort), (Http3ErrorCode)_errorCodeFeature.Error);
 
                     // Wait for active requests to complete.
                     while (_activeRequestCount > 0)
@@ -403,7 +400,7 @@ public async Task ProcessRequestsAsync<TContext>(IHttpApplication<TContext> appl
                 }
                 catch
                 {
-                    Abort(connectionError, Http3ErrorCode.InternalError);
+                    Abort(CreateConnectionAbortError(error, clientAbort), Http3ErrorCode.InternalError);
                     throw;
                 }
                 finally
@@ -418,6 +415,21 @@ public async Task ProcessRequestsAsync<TContext>(IHttpApplication<TContext> appl
             }
         }
 
+        private ConnectionAbortedException CreateConnectionAbortError(Exception? error, bool clientAbort)
+        {
+            if (error is ConnectionAbortedException abortedException)
+            {
+                return abortedException;
+            }
+
+            if (clientAbort)
+            {
+                return new ConnectionAbortedException("The client closed the HTTP/3 connection.", error!);
+            }
+
+            return new ConnectionAbortedException(CoreStrings.Http3ConnectionFaulted, error!);
+        }
+
         private Http3StreamContext CreateHttpStreamContext(ConnectionContext streamContext)
         {
             var httpConnectionContext = new Http3StreamContext(
@@ -657,8 +669,7 @@ public void OnInputOrOutputCompleted()
         {
             TryStopAcceptingStreams();
 
-            Debug.Assert(Enum.IsDefined((Http3ErrorCode)_errorCodeFeature.Error), "Known HTTP/3 error code should be set.");
-
+            // Abort the connection using the error code the client used. For a graceful close, this should be H3_NO_ERROR.
             Abort(new ConnectionAbortedException(CoreStrings.ConnectionAbortedByClient), (Http3ErrorCode)_errorCodeFeature.Error);
         }
 

src/Servers/Kestrel/Core/src/Internal/Http3/Http3ControlStream.cs

@@ -344,11 +344,6 @@ private void ProcessSetting(long id, long value)
             }
         }
 
-        internal ValueTask CompleteAsync()
-        {
-            return _frameWriter.CompleteAsync();
-        }
-
         private ValueTask ProcessGoAwayFrameAsync()
         {
             EnsureSettingsFrame(Http3FrameType.GoAway);

src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs

@@ -463,12 +463,12 @@ public async Task ProcessRequestAsync<TContext>(IHttpApplication<TContext> appli
 
                 _context.StreamLifetimeHandler.OnStreamConnectionError(ex);
             }
+            catch (ConnectionAbortedException ex)
+            {
+                error = ex;
+            }
             catch (ConnectionResetException ex)
             {
-                // TODO: This is temporary. Don't want to tie HTTP/3 layer to one transport.
-                // This is here to check what other exceptions can cause ConnectionResetException.
-                Debug.Assert(ex.InnerException is QuicStreamAbortedException);
-
                 error = ex;
                 Abort(new ConnectionAbortedException(ex.Message, ex), (Http3ErrorCode)_errorCodeFeature.Error);
             }

src/Servers/Kestrel/Transport.Quic/src/Internal/QuicStreamContext.cs

@@ -353,7 +353,7 @@ private async Task DoSend()
             }
             finally
             {
-                ShutdownWrite(shutdownReason);
+                ShutdownWrite(_shutdownWriteReason ?? _shutdownReason ?? shutdownReason);
 
                 // Complete the output after disposing the stream
                 Output.Complete(unexpectedError ?? shutdownReason);
@@ -373,6 +373,7 @@ public override void Abort(ConnectionAbortedException abortReason)
             }
 
             _serverAborted = true;
+            _shutdownReason = abortReason;
 
             _log.StreamAbort(this, Error, abortReason.Message);
 

src/Servers/Kestrel/Transport.Quic/test/QuicStreamContextTests.cs

@@ -46,6 +46,8 @@ public async Task BidirectionalStream_ServerReadsDataAndCompletes_GracefullyClos
             var quicConnectionContext = Assert.IsType<QuicConnectionContext>(serverConnection);
 
             Assert.Equal(1, quicConnectionContext.StreamPool.Count);
+
+            Assert.Contains(TestSink.Writes, m => m.Message.Contains(@"shutting down writes because: ""The QUIC transport's send loop completed gracefully.""."));
         }
 
         [ConditionalTheory]
@@ -292,6 +294,8 @@ public async Task ServerToClientUnidirectionalStream_ServerWritesDataAndComplete
 
             // Both send and receive loops have exited.
             await quicStreamContext._processingTask.DefaultTimeout();
+
+            Assert.Contains(TestSink.Writes, m => m.Message.Contains(@"shutting down writes because: ""The QUIC transport's send loop completed gracefully.""."));
         }
 
         [ConditionalFact]
@@ -333,6 +337,8 @@ public async Task ServerToClientUnidirectionalStream_ServerAborts_ClientGetsAbor
 
             // Both send and receive loops have exited.
             await quicStreamContext._processingTask.DefaultTimeout();
+
+            Assert.Contains(TestSink.Writes, m => m.Message.Contains(@"shutting down writes because: ""Test message""."));
         }
 
         [ConditionalFact]

src/Servers/Kestrel/shared/test/Http3/Http3InMemory.cs

@@ -150,9 +150,6 @@ internal async Task WaitForConnectionErrorAsync<TException>(bool ignoreNonGoAway
 #else
             await _connectionTask;
 #endif
-
-            // Verify server-to-client control stream has completed.
-            await _inboundControlStream.ReceiveEndAsync();
         }
 
         internal async Task WaitForGoAwayAsync(bool ignoreNonGoAwayFrames, long? expectedLastStreamId)

src/Servers/Kestrel/test/Interop.FunctionalTests/Http3/Http3RequestTests.cs

@@ -880,8 +880,18 @@ await WaitForLogAsync(logs =>
                 await WaitForLogAsync(logs =>
                 {
                     const int applicationAbortedConnectionId = 6;
-                    return logs.Any(w => w.LoggerName == "Microsoft.AspNetCore.Server.Kestrel.Transport.Quic" &&
-                                         w.EventId == applicationAbortedConnectionId);
+                    var connectionAbortLog = logs.FirstOrDefault(
+                        w => w.LoggerName == "Microsoft.AspNetCore.Server.Kestrel.Transport.Quic" &&
+                            w.EventId == applicationAbortedConnectionId);
+                    if (connectionAbortLog == null)
+                    {
+                        return false;
+                    }
+
+                    // This message says the client closed the connection because the server
+                    // sends a GOAWAY and the client then closes the connection once all requests are finished.
+                    Assert.Contains("The client closed the connection.", connectionAbortLog.Message);
+                    return true;
                 }, "Wait for connection abort.");
 
                 Logger.LogInformation("Sending request after connection abort.");