-
Notifications
You must be signed in to change notification settings - Fork 4.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[HTTP/3] Support for HTTP/3 multiple connections #101531
base: main
Are you sure you want to change the base?
Conversation
Note regarding the
|
No noticeable regression on perf from main: main
crank --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/build/ci.profile.yml --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/scenarios/httpclient.benchmarks.yml --client.framework net9.0 --server.framework net9.0 --scenario httpclient-kestrel-get --profile intel-lin-app --profile amd-lin2-load --variable useHttpMessageInvoker=true --variable httpVersion=3.0 --variable useHttps=true --variable responseSize=256 --variable numberOfHttpClients=1 --variable concurrencyPerHttpClient=10000
crank --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/build/ci.profile.yml --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/scenarios/httpclient.benchmarks.yml --client.framework net9.0 --server.framework net9.0 --scenario httpclient-kestrel-get --profile intel-lin-app --profile amd-lin2-load --variable useHttpMessageInvoker=true --variable httpVersion=3.0 --variable useHttps=true --variable responseSize=256 --variable numberOfHttpClients=1 --variable concurrencyPerHttpClient=100
crank --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/build/ci.profile.yml --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/scenarios/httpclient.benchmarks.yml --client.framework net9.0 --server.framework net9.0 --scenario httpclient-kestrel-get --profile intel-lin-app --profile amd-lin2-load --variable useHttpMessageInvoker=true --variable httpVersion=3.0 --variable useHttps=true --variable responseSize=256 --variable numberOfHttpClients=20 --variable concurrencyPerHttpClient=500
PR, multiple H3 connections OFF
crank --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/build/ci.profile.yml --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/scenarios/httpclient.benchmarks.yml --client.framework net9.0 --server.framework net9.0 --scenario httpclient-kestrel-get --profile intel-lin-app --profile amd-lin2-load --variable useHttpMessageInvoker=true --variable httpVersion=3.0 --variable useHttps=true --variable responseSize=256 --variable numberOfHttpClients=1 --variable concurrencyPerHttpClient=10000 --client.options.outputFiles artifacts/bin/testhost/net9.0-linux-Release-x64/shared/Microsoft.NETCore.App/9.0.0/System.Net.Http.dll --client.options.outputFiles artifacts/bin/testhost/net9.0-linux-Release-x64/shared/Microsoft.NETCore.App/9.0.0/System.Net.Quic.dll
crank --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/build/ci.profile.yml --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/scenarios/httpclient.benchmarks.yml --client.framework net9.0 --server.framework net9.0 --scenario httpclient-kestrel-get --profile intel-lin-app --profile amd-lin2-load --variable useHttpMessageInvoker=true --variable httpVersion=3.0 --variable useHttps=true --variable responseSize=256 --variable numberOfHttpClients=1 --variable concurrencyPerHttpClient=100 --client.options.outputFiles artifacts/bin/testhost/net9.0-linux-Release-x64/shared/Microsoft.NETCore.App/9.0.0/System.Net.Http.dll --client.options.outputFiles artifacts/bin/testhost/net9.0-linux-Release-x64/shared/Microsoft.NETCore.App/9.0.0/System.Net.Quic.dl
rank --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/build/ci.profile.yml --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/scenarios/httpclient.benchmarks.yml --client.framework net9.0 --server.framework net9.0 --scenario httpclient-kestrel-get --profile intel-lin-app --profile amd-lin2-load --variable useHttpMessageInvoker=true --variable httpVersion=3.0 --variable useHttps=true --variable responseSize=256 --variable numberOfHttpClients=20 --variable concurrencyPerHttpClient=500 --client.options.outputFiles artifacts/bin/testhost/net9.0-linux-Release-x64/shared/Microsoft.NETCore.App/9.0.0/System.Net.Http.dll --client.options.outputFiles artifacts/bin/testhost/net9.0-linux-Release-x64/shared/Microsoft.NETCore.App/9.0.0/System.Net.Quic.dll
PR, multiple H3 connections ON
crank --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/build/ci.profile.yml --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/scenarios/httpclient.benchmarks.yml --client.framework net9.0 --server.framework net9.0 --scenario httpclient-kestrel-get --profile intel-lin-app --profile amd-lin2-load --variable useHttpMessageInvoker=true --variable httpVersion=3.0 --variable useHttps=true --variable responseSize=256 --variable numberOfHttpClients=1 --variable concurrencyPerHttpClient=10000 --client.options.outputFiles artifacts/bin/testhost/net9.0-linux-Release-x64/shared/Microsoft.NETCore.App/9.0.0/System.Net.Http.dll --client.options.outputFiles artifacts/bin/testhost/net9.0-linux-Release-x64/shared/Microsoft.NETCore.App/9.0.0/System.Net.Quic.dll
|
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3Connection.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs
Outdated
Show resolved
Hide resolved
.../System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.cs
Outdated
Show resolved
Hide resolved
.../System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.cs
Outdated
Show resolved
Hide resolved
/// Occurres when an additional stream capacity has been released by the peer. Corresponds to receiving MAX_STREAMS frame. | ||
/// </summary> | ||
public event QuicConnectionStreamsAvailableEventHandler? StreamsAvailable; | ||
private async void OnStreamsAvailable(int bidirectionalStreamsCountIncrement, int unidirectionalStreamsCountIncrement) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private async void OnStreamsAvailable(int bidirectionalStreamsCountIncrement, int unidirectionalStreamsCountIncrement) | |
private async Task OnStreamsAvailable(int bidirectionalStreamsCountIncrement, int unidirectionalStreamsCountIncrement) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gag reflex whenever I see async void
😃
E.g. it could be a regular async Task
with the caller doing _ = OnStreamsAvailable()
, like we do for InjectNewHttp3ConnectionAsync
.
It's probably fine in this case though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually prefer async void
to _ = BlaBlaAsync()
😄 And we have a precedent for it in S.N.Quic (which I authored 😃 )
src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs
Outdated
Show resolved
Hide resolved
...m.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.Http3.cs
Outdated
Show resolved
Hide resolved
…andler/Http3Connection.cs Co-authored-by: Radek Zikmund <32671551+rzikm@users.noreply.github.com>
Change of the logic: |
...m.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.Http3.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few more comments but otherwise this LGTM.
It's good that it substantially mimics the H2 logic, makes it much easier to follow.
@@ -210,6 +284,7 @@ public async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, lon | |||
|
|||
if (quicStream == null) | |||
{ | |||
ReleaseStream(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a 100% reliable way to detect whether the underlying QuicConnection
counted the stream as consumed or not?
E.g. even when OpenOutboundStreamAsync
is cancelled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, this is not actually right, in both cases we should not re-increment the count (only if we didn't even try to open the stream). I did some testing and added some comments to QuicStream
to make sure we have consistent behavior wrt stream counts regardless whether the call gets cancelled or not.
Debug.Assert(_availableRequestStreamsCount >= 0); | ||
|
||
if (NetEventSource.Log.IsEnabled()) Trace($"ReleaseStream: _availableRequestStreamsCount = {_availableRequestStreamsCount}"); | ||
++_availableRequestStreamsCount; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll have to signal the _availableStreamsWaiter
from here as well because StreamsAvailableCallback
may not be called for a while if a bunch of streams got consumed by requests that were immediately canceled and released their streams.
} | ||
public delegate void QuicConnectionStreamsAvailableCallback(System.Net.Quic.QuicConnection connection, int bidirectionalStreamsCountIncrement, int unidirectionalStreamsCountIncrement); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More of an API review question, but is there any chance we'll want to expose more info here such that we'd want the arguments to be a struct instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have only stream count related info, technically the transport sends MAX_STREAM_ID and separately for Uni/Bi directional streams.
The only think we could expose are different ways to express the new stream limit, which is part of alternative design in #101534
/// Provided via <see cref="StartAsync(Action{QuicStreamType}, CancellationToken)" /> from <see cref="QuicConnection" /> so that <see cref="QuicStream"/> can decrement its available stream count field. | ||
/// When <see cref="HandleEventStartComplete(ref START_COMPLETE_DATA)">START_COMPLETE</see> arrives it gets invoked and unset back to <c>null</c> to not to hold any unintended reference to <see cref="QuicConnection"/>. | ||
/// </summary> | ||
private Action<QuicStreamType>? _decrementAvailableStreamCount; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this functionally the same as if we instead temporarily stored a QuicConnection
and exposed the decrement function there as internal?
@@ -603,6 +674,15 @@ private unsafe int HandleEventPeerStreamStarted(ref PEER_STREAM_STARTED_DATA dat | |||
data.Flags |= QUIC_STREAM_OPEN_FLAGS.DELAY_ID_FC_UPDATES; | |||
return QUIC_STATUS_SUCCESS; | |||
} | |||
private unsafe int HandleEventStreamsAvailable(ref STREAMS_AVAILABLE_DATA data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get MsQuic to expose increments instead of current counts?
(presumably, that's the data that they're reacting to that triggered these events in the first place?)
That'd let us drop the whole DecrementAvailableStreamCount
dance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this idea. I can try that. It would also resolve the issue with the _decrementAvailableStreamCount
callback. However, I'll not push on adding this to MsQuic, until we have the API approved, we might end up with MAX_STREAM_ID or different value being reported by the callback...
@@ -427,7 +498,7 @@ public async ValueTask<QuicStream> OpenOutboundStreamAsync(QuicStreamType type, | |||
NetEventSource.Info(this, $"{this} New outbound {type} stream {stream}."); | |||
} | |||
|
|||
await stream.StartAsync(cancellationToken).ConfigureAwait(false); | |||
await stream.StartAsync(DecrementAvailableStreamCount, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using DecrementAvailableStreamCount
directly here will allocate a new object for every stream.
We could cache a single Action<QuicStreamType>
instance on the connection.
// The new connection could not handle even one request, either because it shut down before we could use it for any requests, | ||
// or because it immediately set the max concurrent streams limit to 0. | ||
// We don't want to get stuck in a loop where we keep trying to create new connections for the same request. | ||
// So, treat this as a connection failure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is right for HTTP/3.
With HTTP/2, this would only happen if the connection was already closed as it otherwise always starts with a non-zero amount of available streams.
With HTTP/3, _availableRequestStreamsCount
may still be 0 at this point, especially given that StreamsAvailableCallback
is queued to the thread pool and may therefore signal the initial available streams after we hand the connection to the pool and reach this block.
In which case we'd be throwing away a good connection and failing a request.
connectionException = e is OperationCanceledException oce && oce.CancellationToken == cts.Token && !waiter.CancelledByOriginatingRequestCompletion ? | ||
CreateConnectTimeoutException(oce) : | ||
e; | ||
// If the connection hasn't been initialized with QuicConnection there's no need to dispose it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assumes that InitQuicConnection
may never throw, right?
/// Occurres when an additional stream capacity has been released by the peer. Corresponds to receiving MAX_STREAMS frame. | ||
/// </summary> | ||
public event QuicConnectionStreamsAvailableEventHandler? StreamsAvailable; | ||
private async void OnStreamsAvailable(int bidirectionalStreamsCountIncrement, int unidirectionalStreamsCountIncrement) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gag reflex whenever I see async void
😃
E.g. it could be a regular async Task
with the caller doing _ = OnStreamsAvailable()
, like we do for InjectNewHttp3ConnectionAsync
.
It's probably fine in this case though.
Feel free to review and discuss the API changes. This is NO MERGE until we have the APIs approved and all the TODOs resolved
The implementation takes H/2 connection pooling and slightly adjusts it for H/3 specific behaviors (see a diff between HttpConnectionPool.Http2.cs and HttpConnectionPool.Http3.cs).
The PR depends on 2 API reviews:
EnableMultipleHttp3Connections
QuicConnection
#101534: forQuicConnection
changesTODO: Add numbers from benchmarks when it runs
TODO: Stress run depends on dotnet/aspnetcore#55282
Fixes #51775
Fixes #54968
Fixes #68380
Resolves #101535
Resolves #101534