-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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
Closed
Closed
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
73e022b
HTTP/3: Pool QuicStreamContext instances
JamesNK 27af4ff
Update
JamesNK 1d9a415
Tests
JamesNK f92fe86
Fix
JamesNK 75ef80d
Limit pool size and unit test
JamesNK 4fbe9be
Extract test stream context from test stream types
JamesNK 383876e
Reuse context in tests
JamesNK 7f2da6a
Reset Http3Stream
JamesNK 9047c89
Update
JamesNK 6eed9e8
Add PersistentState
JamesNK File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
20 changes: 20 additions & 0 deletions
20
src/Servers/Kestrel/Core/src/Internal/Http3/ICachedHttp3StreamFeature.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http3 | ||
{ | ||
internal interface ICachedHttp3StreamFeature<TContext> where TContext : notnull | ||
{ | ||
Http3Stream<TContext> CachedStream { get; } | ||
} | ||
|
||
internal class DefaultCachedHttp3StreamFeature<TContext> : ICachedHttp3StreamFeature<TContext> where TContext : notnull | ||
{ | ||
public Http3Stream<TContext> CachedStream { get; } | ||
|
||
public DefaultCachedHttp3StreamFeature(Http3Stream<TContext> cachedStream) | ||
{ | ||
CachedStream = cachedStream; | ||
} | ||
} | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
62 changes: 62 additions & 0 deletions
62
src/Servers/Kestrel/Transport.Quic/src/Internal/CompletionPipeReader.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
using System; | ||
using System.IO.Pipelines; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
|
||
namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Quic.Internal | ||
{ | ||
internal sealed class CompletionPipeReader : PipeReader | ||
{ | ||
private readonly PipeReader _inner; | ||
|
||
public bool IsComplete { get; private set; } | ||
public Exception? CompleteException { get; private set; } | ||
|
||
public CompletionPipeReader(PipeReader inner) | ||
{ | ||
_inner = inner; | ||
} | ||
|
||
public override void AdvanceTo(SequencePosition consumed) | ||
{ | ||
_inner.AdvanceTo(consumed); | ||
} | ||
|
||
public override void AdvanceTo(SequencePosition consumed, SequencePosition examined) | ||
{ | ||
_inner.AdvanceTo(consumed, examined); | ||
} | ||
|
||
public override ValueTask CompleteAsync(Exception? exception = null) | ||
{ | ||
IsComplete = true; | ||
CompleteException = exception; | ||
return _inner.CompleteAsync(exception); | ||
} | ||
|
||
public override void Complete(Exception? exception = null) | ||
{ | ||
IsComplete = true; | ||
CompleteException = exception; | ||
_inner.Complete(exception); | ||
} | ||
|
||
public override void CancelPendingRead() | ||
{ | ||
_inner.CancelPendingRead(); | ||
} | ||
|
||
public override ValueTask<ReadResult> ReadAsync(CancellationToken cancellationToken = default) | ||
{ | ||
return _inner.ReadAsync(cancellationToken); | ||
} | ||
|
||
public override bool TryRead(out ReadResult result) | ||
{ | ||
return _inner.TryRead(out result); | ||
} | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Uh oh!
There was an error while loading. Please reload this page.
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:
Or a new explicit dictionary on transport context? Note that BaseConnectionContext already has
Features
andItems
properties - https://github.com/dotnet/aspnetcore/blob/main/src/Servers/Connections.Abstractions/src/BaseConnectionContext.csI 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.
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.