Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 37 additions & 21 deletions src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -224,14 +224,21 @@ public async Task ProcessRequestsAsync<TContext>(IHttpApplication<TContext> appl
// the maximum capacity of the dynamic table to zero.

// Don't create Encoder and Decoder as they aren't used now.
Exception? error = null;

// Don't delay setting up the connection on the control stream being ready.
// Task is awaited when connection finishes.
var controlStreamTask = CreateControlStreamAsync(application);
Exception? error = null;
ValueTask outboundControlStreamTask = default;

try
{
var outboundControlStream = await CreateNewUnidirectionalStreamAsync(application);
lock (_sync)
{
OutboundControlStream = outboundControlStream;
}

// Don't delay on waiting to send outbound control stream settings.
outboundControlStreamTask = ProcessOutboundControlStreamAsync(outboundControlStream);

while (_isClosed == 0)
{
// Don't pass a cancellation token to AcceptAsync.
Expand Down Expand Up @@ -341,23 +348,26 @@ public async Task ProcessRequestsAsync<TContext>(IHttpApplication<TContext> appl
stream.Abort(connectionError, (Http3ErrorCode)_errorCodeFeature.Error);
}

lock (_sync)
{
OutboundControlStream?.Abort(connectionError, (Http3ErrorCode)_errorCodeFeature.Error);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the lock really necessary? If we see a stale null OutboundControlStream, is it wrong to treat it as if it hadn't been fully created? I know this lock was there before and it doesn't really hurt if it's generally uncontested. I'm just curious.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably not necessary. Can look at cleaning it up in the future.

}

while (_activeRequestCount > 0)
{
await _streamCompletionAwaitable;
}

await outboundControlStreamTask;

_context.TimeoutControl.CancelTimeout();
_context.TimeoutControl.StartDrainTimeout(Limits.MinResponseDataRate, Limits.MaxResponseBufferSize);
}
catch
{
Abort(connectionError, Http3ErrorCode.NoError);
Abort(connectionError, Http3ErrorCode.InternalError);
throw;
}

// Ensure control stream creation task finished. At this point the connection, including the control
// stream should be closed/aborted. Error handling inside method ensures await won't throw.
await controlStreamTask;
}
}

Expand Down Expand Up @@ -400,18 +410,12 @@ private void UpdateConnectionState()
}
}

private async ValueTask CreateControlStreamAsync<TContext>(IHttpApplication<TContext> application) where TContext : notnull
private async ValueTask ProcessOutboundControlStreamAsync(Http3ControlStream controlStream)
{
try
{
var stream = await CreateNewUnidirectionalStreamAsync(application);
lock (_sync)
{
OutboundControlStream = stream;
}

await stream.SendStreamIdAsync(id: 0);
await stream.SendSettingsFrameAsync();
await controlStream.SendStreamIdAsync(id: 0);
await controlStream.SendSettingsFrameAsync();
}
catch (Exception ex)
{
Expand Down Expand Up @@ -449,15 +453,27 @@ private async ValueTask<Http3ControlStream> CreateNewUnidirectionalStreamAsync<T
return new Http3ControlStream<TContext>(application, httpConnectionContext);
}

private ValueTask<FlushResult> SendGoAway(long id)
private async ValueTask<FlushResult> SendGoAway(long id)
{
Http3ControlStream? stream;
lock (_sync)
{
if (OutboundControlStream != null)
stream = OutboundControlStream;
}

if (stream != null)
{
try
{
return await stream.SendGoAway(id);
}
catch
{
return OutboundControlStream.SendGoAway(id);
// The control stream may not be healthy.
// Ignore error sending go away.
}
}

return default;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http3
internal class Http3ConnectionErrorException : Exception
{
public Http3ConnectionErrorException(string message, Http3ErrorCode errorCode)
: base($"HTTP/3 connection error ({errorCode}): {message}")
: base($"HTTP/3 connection error ({Http3Formatting.ToFormattedErrorCode(errorCode)}): {message}")
{
ErrorCode = errorCode;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,15 @@ await WaitForConnectionErrorAsync<Http3ConnectionErrorException>(
[Fact]
public async Task ControlStream_ServerToClient_ErrorInitializing_ConnectionError()
{
OnCreateServerControlStream = () => throw new Exception();
OnCreateServerControlStream = () =>
{
var controlStream = new Http3ControlStream(this, StreamInitiator.Server);

// Make server connection error when trying to write to control stream.
controlStream.StreamContext.Transport.Output.Complete();

return controlStream;
};

await InitializeConnectionAsync(_noopApplication);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -187,13 +187,16 @@ internal async ValueTask<Http3ControlStream> GetInboundControlStream()
if (_inboundControlStream == null)
{
var reader = MultiplexedConnectionContext.ToClientAcceptQueue.Reader;
while (await reader.WaitToReadAsync())
while (await reader.WaitToReadAsync().DefaultTimeout())
{
while (reader.TryRead(out var stream))
{
_inboundControlStream = stream;
var streamId = await stream.TryReadStreamIdAsync();
Debug.Assert(streamId == 0, "StreamId sent that was non-zero, which isn't handled by tests");

// -1 means stream was completed.
Debug.Assert(streamId == 0 || streamId == -1, "StreamId sent that was non-zero, which isn't handled by tests");

return _inboundControlStream;
}
}
Expand Down Expand Up @@ -231,6 +234,12 @@ internal async Task WaitForConnectionErrorAsync<TException>(bool ignoreNonGoAway
}

AssertConnectionError<TException>(expectedErrorCode, expectedErrorMessage);

// Verify HttpConnection.ProcessRequestsAsync has exited.
await _connectionTask.DefaultTimeout();

// Verify server-to-client control stream has completed.
await _inboundControlStream.ReceiveEndAsync();
}

internal void AssertConnectionError<TException>(Http3ErrorCode expectedErrorCode, params string[] expectedErrorMessage) where TException : Exception
Expand Down