Skip to content

Commit 670cfd0

Browse files
authored
HTTP/3: Fix incorrectly pooling aborted streams (#35441)
1 parent 4655c3c commit 670cfd0

File tree

4 files changed

+115
-3
lines changed

4 files changed

+115
-3
lines changed

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

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -449,11 +449,15 @@ private void ShutdownWrite(Exception? shutdownReason)
449449

450450
public override async ValueTask DisposeAsync()
451451
{
452+
// Be conservative about what can be pooled.
453+
// Only pool bidirectional streams whose pipes have completed successfully and haven't been aborted.
452454
CanReuse = _stream.CanRead && _stream.CanWrite
453455
&& _transportPipeReader.IsCompletedSuccessfully
454456
&& _transportPipeWriter.IsCompletedSuccessfully
455457
&& !_clientAbort
456-
&& !_serverAborted;
458+
&& !_serverAborted
459+
&& _shutdownReadReason == null
460+
&& _shutdownWriteReason == null;
457461

458462
_originalTransport.Input.Complete();
459463
_originalTransport.Output.Complete();

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -115,8 +115,7 @@ public async Task ClientCertificate_Required_NotSent_ConnectionAborted()
115115

116116
// https://github.com/dotnet/runtime/issues/57246 The accept still completes even though the connection was rejected, but it's already failed.
117117
var serverContext = await connectionListener.AcceptAndAddFeatureAsync().DefaultTimeout();
118-
qex = await Assert.ThrowsAsync<QuicException>(() => serverContext.ConnectAsync().DefaultTimeout());
119-
Assert.Equal("Failed to open stream to peer. Error Code: INVALID_STATE", qex.Message);
118+
await Assert.ThrowsAsync<QuicException>(() => serverContext.ConnectAsync().DefaultTimeout());
120119
}
121120
}
122121
}

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

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,52 @@ public async Task BidirectionalStream_ServerReadsDataAndCompletes_GracefullyClos
4949
Assert.Contains(TestSink.Writes, m => m.Message.Contains(@"shutting down writes because: ""The QUIC transport's send loop completed gracefully.""."));
5050
}
5151

52+
[ConditionalFact]
53+
[MsQuicSupported]
54+
public async Task BidirectionalStream_ReadAborted_NotPooled()
55+
{
56+
// Arrange
57+
await using var connectionListener = await QuicTestHelpers.CreateConnectionListenerFactory(LoggerFactory);
58+
59+
var options = QuicTestHelpers.CreateClientConnectionOptions(connectionListener.EndPoint);
60+
using var clientConnection = new QuicConnection(QuicImplementationProviders.MsQuic, options);
61+
await clientConnection.ConnectAsync().DefaultTimeout();
62+
63+
await using var serverConnection = await connectionListener.AcceptAndAddFeatureAsync().DefaultTimeout();
64+
65+
// Act
66+
var clientStream = clientConnection.OpenBidirectionalStream();
67+
await clientStream.WriteAsync(TestData).DefaultTimeout();
68+
var serverStream = await serverConnection.AcceptAsync().DefaultTimeout();
69+
var readResult = await serverStream.Transport.Input.ReadAtLeastAsync(TestData.Length).DefaultTimeout();
70+
serverStream.Transport.Input.AdvanceTo(readResult.Buffer.End);
71+
72+
await clientStream.WriteAsync(TestData).DefaultTimeout();
73+
74+
// Complete writing.
75+
await serverStream.Transport.Output.CompleteAsync();
76+
77+
// Abort read-side of the stream and then complete pipe.
78+
// This simulates what Kestrel does when a request finishes without
79+
// reading the request body to the end.
80+
serverStream.Features.Get<IStreamAbortFeature>().AbortRead((long)Http3ErrorCode.NoError, new ConnectionAbortedException("Test message."));
81+
await serverStream.Transport.Input.CompleteAsync();
82+
83+
var quicStreamContext = Assert.IsType<QuicStreamContext>(serverStream);
84+
85+
// Both send and receive loops have exited.
86+
await quicStreamContext._processingTask.DefaultTimeout();
87+
Assert.True(quicStreamContext.CanWrite);
88+
Assert.True(quicStreamContext.CanRead);
89+
90+
await quicStreamContext.DisposeAsync();
91+
92+
var quicConnectionContext = Assert.IsType<QuicConnectionContext>(serverConnection);
93+
94+
// Assert
95+
Assert.Equal(0, quicConnectionContext.StreamPool.Count);
96+
}
97+
5298
[ConditionalTheory]
5399
[MsQuicSupported]
54100
[InlineData(1024)]

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

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -672,6 +672,69 @@ public async Task GET_ClientCancellationAfterResponseHeaders_RequestAbortRaised(
672672
}
673673
}
674674

675+
[ConditionalFact]
676+
[MsQuicSupported]
677+
public async Task StreamResponseContent_DelayAndTrailers_ClientSuccess()
678+
{
679+
// Arrange
680+
var builder = CreateHostBuilder(async context =>
681+
{
682+
var feature = context.Features.Get<IHttpResponseTrailersFeature>();
683+
684+
for (var i = 1; i < 200; i++)
685+
{
686+
feature.Trailers.Append($"trailer-{i}", new string('!', i));
687+
}
688+
689+
Logger.LogInformation($"Server trailer count: {feature.Trailers.Count}");
690+
691+
await context.Request.BodyReader.ReadAtLeastAsync(TestData.Length);
692+
693+
for (var i = 0; i < 3; i++)
694+
{
695+
await context.Response.BodyWriter.WriteAsync(TestData);
696+
697+
await Task.Delay(TimeSpan.FromMilliseconds(10));
698+
}
699+
});
700+
701+
using (var host = builder.Build())
702+
using (var client = Http3Helpers.CreateClient())
703+
{
704+
await host.StartAsync();
705+
706+
// Act
707+
var request = new HttpRequestMessage(HttpMethod.Post, $"https://127.0.0.1:{host.GetPort()}/");
708+
request.Content = new ByteArrayContent(TestData);
709+
request.Version = HttpVersion.Version30;
710+
request.VersionPolicy = HttpVersionPolicy.RequestVersionExact;
711+
712+
var response = await client.SendAsync(request, CancellationToken.None);
713+
response.EnsureSuccessStatusCode();
714+
715+
var responseStream = await response.Content.ReadAsStreamAsync();
716+
717+
await responseStream.ReadUntilEndAsync();
718+
719+
Logger.LogInformation($"Client trailer count: {response.TrailingHeaders.Count()}");
720+
721+
for (var i = 1; i < 200; i++)
722+
{
723+
try
724+
{
725+
var value = response.TrailingHeaders.GetValues($"trailer-{i}").Single();
726+
Assert.Equal(new string('!', i), value);
727+
}
728+
catch (Exception ex)
729+
{
730+
throw new Exception($"Error checking trailer {i}", ex);
731+
}
732+
}
733+
734+
await host.StopAsync();
735+
}
736+
}
737+
675738
[ConditionalFact]
676739
[MsQuicSupported]
677740
public async Task GET_MultipleRequests_ConnectionAndTraceIdsUpdated()

0 commit comments

Comments
 (0)