Fix Http3 connection close metric sometimes remaining Unset#65398
Fix Http3 connection close metric sometimes remaining Unset#65398BrennanConroy wants to merge 1 commit intomainfrom
Conversation
BrennanConroy
added
feature-kestrel
area-networking
There was a problem hiding this comment.
Pull request overview
Fixes an occasional debug-assert crash in Kestrel HTTP/3 metrics by ensuring an appropriate ConnectionEndReason is set when a ConnectionResetException occurs with no active requests (mirroring HTTP/2 behavior).
Changes:
- Set
ConnectionEndReason.TransportCompletedfor HTTP/3 connection resets when_activeRequestCount == 0. - Add an HTTP/3 in-memory functional test that reproduces the “reset with no active requests” scenario to prevent regressions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs | Ensures connection reset without active requests records TransportCompleted instead of leaving the reason Unset. |
| src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs | Adds a regression test that triggers a connection reset early and validates metrics don’t report an error. |
| public async Task ConnectionResetWithNoActiveRequests_ReportsTransportCompleted() | ||
| { | ||
| // When a ConnectionResetException occurs with no active requests (e.g., the client | ||
| // disconnects after all requests have completed), the connection end reason should be | ||
| // TransportCompleted, not Unset. | ||
|
|
||
| var abortTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); | ||
|
|
||
| // skipCount: 0 means the first AcceptAsync throws ConnectionResetException immediately. | ||
| // The outbound control stream is created via ConnectAsync (not AcceptAsync) so it still succeeds. | ||
| Http3Api.MultiplexedConnectionContext = new ConnectionResetMultiplexedConnectionContext(Http3Api, skipCount: 0, abortTcs); | ||
| await Http3Api.InitializeConnectionAsync(_noopApplication); | ||
|
|
||
| // Wait for the connection to be aborted (ProcessRequestsAsync calls Abort which sets metrics | ||
| // before calling _multiplexedContext.Abort, which signals abortTcs). | ||
| await abortTcs.Task.DefaultTimeout(); | ||
|
|
||
| MetricsAssert.NoError(Http3Api.ConnectionTags); | ||
| } |
There was a problem hiding this comment.
This test name/comment say it verifies the connection end reason is TransportCompleted, but the only assertion is MetricsAssert.NoError(...) (i.e., absence of error.type). That condition would also be true for other non-error end reasons, so the test isn’t actually validating the specific behavior it describes. Consider renaming the test/comment to match what’s asserted (e.g., that metrics don’t crash/aren’t left Unset), or add an assertion mechanism that can observe the exact ConnectionEndReason used.
| public override async ValueTask<ConnectionContext> AcceptAsync(CancellationToken cancellationToken = default) | ||
| { | ||
| if (_skipCount-- <= 0) | ||
| { | ||
| throw new ConnectionResetException("Connection reset by peer."); | ||
| } | ||
| return await base.AcceptAsync(cancellationToken); |
There was a problem hiding this comment.
AcceptAsync is marked async but it only forwards to base.AcceptAsync (or throws). This introduces an unnecessary async state machine in the hot path of the test harness. Consider removing async and returning the ValueTask from base.AcceptAsync(...) directly.
| public override async ValueTask<ConnectionContext> AcceptAsync(CancellationToken cancellationToken = default) | |
| { | |
| if (_skipCount-- <= 0) | |
| { | |
| throw new ConnectionResetException("Connection reset by peer."); | |
| } | |
| return await base.AcceptAsync(cancellationToken); | |
| public override ValueTask<ConnectionContext> AcceptAsync(CancellationToken cancellationToken = default) | |
| { | |
| if (_skipCount-- <= 0) | |
| { | |
| throw new ConnectionResetException("Connection reset by peer."); | |
| } | |
| return base.AcceptAsync(cancellationToken); |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
LGTM other than the copilot nits. |
2 participants
Closes #57579
This is the same as what Http2 does:
aspnetcore/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs
Lines 390 to 401 in 07ad803