-
Notifications
You must be signed in to change notification settings - Fork 10.3k
HTTP/3: Http3Stream pooling (builds on top of QUIC stream pooling) #34222
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
Conversation
if (!_streamContextPool.TryDequeue(out var testStreamContext)) | ||
{ | ||
testStreamContext = new TestStreamContext(canRead: true, canWrite: true, this); | ||
} | ||
testStreamContext.Initialize(GetStreamId(0x00)); |
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.
TestStreamContext
is now reused to simulate how QUIC stream contexts are reused.
It is a bit of extra complexity to our tests, but I want to be able to test pooling Http3Streams without having to use Quic transport.
|
||
namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests | ||
{ | ||
internal sealed class CompletionPipeReader : PipeReader |
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.
Copy and paste from QUIC transport. Will move into shared code files.
if (stream == null) | ||
{ | ||
stream = new Http3Stream<TContext>(application, CreateHttpStreamContext(streamContext)); | ||
streamContext.Features.Set<ICachedHttp3StreamFeature<TContext>>(new DefaultCachedHttp3StreamFeature<TContext>(stream)); |
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.
The feature collection doesn't make sense as a caching mechanism since the collection itself shouldn't preserve state between uses. You'd want the lower layer to provide an explicate caching feature.
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.
Do you have a suggestion about how that might work? Would it be a new feature with a dictionary that gets preserved between requests like:
public interface IStateFeature
{
IDictionary<string, object> State { get; }
}
Or a new explicit dictionary on transport context? Note that BaseConnectionContext already has Features
and Items
properties - https://github.com/dotnet/aspnetcore/blob/main/src/Servers/Connections.Abstractions/src/BaseConnectionContext.cs
I created an issue a while back about using features to preserve the state between requests - #6895. There is a lot of crossover between that and this, just at a different layer.
I don't want to turn this into a big thing so another option is to have a hack in .NET 6 where the stream is added to the transport context's Items
with a specific key and that specific key isn't cleared between calls by the QUIC transport. We could then improve it in .NET 7.
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.
The state feature and the dictionary on the transport context accomplish the same end, I don't have a preference between them.
I don't want to turn this into a big thing so another option is to have a hack in .NET 6 where the stream is added to the transport context's
Items
with a specific key and that specific key isn't cleared between calls by the QUIC transport. We could then improve it in .NET 7.
That is hackier than I want to go. Given our http/3 goals for 6.0 emphasize functionality over perf, I'd rather not cache if we can't come up with a clear mechanic for it.
@@ -23,5 +23,9 @@ Microsoft.AspNetCore.Connections.MultiplexedConnectionBuilder.Use(System.Func<Mi | |||
Microsoft.AspNetCore.Connections.MultiplexedConnectionContext | |||
Microsoft.AspNetCore.Connections.MultiplexedConnectionContext.MultiplexedConnectionContext() -> void | |||
Microsoft.AspNetCore.Connections.MultiplexedConnectionDelegate | |||
abstract Microsoft.AspNetCore.Connections.MultiplexedConnectionContext.AcceptAsync(System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) -> System.Threading.Tasks.ValueTask<Microsoft.AspNetCore.Connections.ConnectionContext?> | |||
abstract Microsoft.AspNetCore.Connections.MultiplexedConnectionContext.ConnectAsync(Microsoft.AspNetCore.Http.Features.IFeatureCollection? features = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) -> System.Threading.Tasks.ValueTask<Microsoft.AspNetCore.Connections.ConnectionContext!> | |||
Microsoft.AspNetCore.Connections.MultiplexedStreamContext |
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.
Maybe put it directly on the ConnectionContext
0ee59b3
to
fa889da
Compare
fa889da
to
6eed9e8
Compare
Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:
|
45dfcaf
to
df76007
Compare
The suggestion is to have add this to Microsoft.Extensions.Features as a new feature type. The connection pooling would be aware of this feature type and re-set it. (not reset it, that's the opposite). + namespace Microsoft.AspNetCore.Http.Features
+ {
+ public interface IPersistentStateFeature { }
+ } |
+ namespace Microsoft.AspNetCore.Http.Features
+ {
+ public interface IPersistentStateFeature
+ {
+ IDictionary<object, object?> State { get; }
+ }
+ } |
Replaced by #34576 |
Note, this PR is built on top of #34075
Making them separate for now to make these changes easier to review.
This PR stashes
Http3Stream
and friends on transport stream content with a feature. It isn't complete, currently types like the framewriter, output producer and header decoder are recreated each time. A bit more work is required to reuse them as well.Proposed API
Add
PersistentState
property to connection context in ASP.NET Core's connection abstraction layer:This collection is different from the existing
Items
dictionary because it is not cleared when aConnectionContext
is pooled and reused for a new request.Usage Examples
This collection is used to persist a
Http3Stream
on a connection's QUIC streams (represented byConnectionContext
) so they are shared between requests:The goal is to reduce per-request allocations with HTTP/3. I haven't measured the difference yet (will measure before merging) but allocations saved per request will be significant (multiple KBs).
Alternative Designs
ConnectionContext
has existing property bags:Items
andFeatures
. These are both reset when a connection is reused and can't be used to persist state between connection instance reuses.TContext
to ConnectionContext for preserving state (like hosting's IHostContextContainer). Generic spreads everywhere.