Skip to content

Add IPersistentStateFeature #34360

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 20, 2021
Merged

Add IPersistentStateFeature #34360

merged 1 commit into from
Jul 20, 2021

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Jul 15, 2021

Fixes #6895

Also unblocks HTTP/3 stream caching on the QUIC transport.

@JamesNK JamesNK changed the title Add PersistentStateFeature Add IPersistentStateFeature Jul 15, 2021
@JamesNK JamesNK requested a review from davidfowl July 15, 2021 01:26
Tratcher
Tratcher reviewed Jul 15, 2021
View reviewed changes
Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

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

I don't have any feedback beyond stephens.

@JamesNK JamesNK mentioned this pull request Jul 16, 2021
Closed
Base automatically changed from jamesnk/http3-quicstreampooling to main July 16, 2021 03:58
halter73
halter73 reviewed Jul 16, 2021
View reviewed changes
src/Servers/Kestrel/tools/CodeGenerator/TransportConnectionFeatureCollection.cs Outdated
@@ -27,6 +28,7 @@ public static string GenerateFile()
"IConnectionIdFeature",
"IConnectionTransportFeature",
"IConnectionItemsFeature",
"IPersistentStateFeature",
Copy link
Member

Choose a reason for hiding this comment

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

I think we should handle this more like the IConnectionSocketFeature, and implement this for QuicStreamContext only, since we don't pool SocketConnections.

Copy link
Member Author

@JamesNK JamesNK Jul 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We don't pool SocketConnections, but because SocketConnections implements this feature it is consistently available from HTTP requests.

HTTP/2 - Implemented on Http2Stream
HTTP/3 - Implemented on QuicStreamContext
HTTP/1.1 - Implemented on SocketConnection

Copy link
Member Author

Choose a reason for hiding this comment

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

ping @halter73

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather this not be implemented by transports it's not supported on. We should implement this on Http1Connection, not SocketConnection, similarly to how you implement it in Http2Stream.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@JamesNK JamesNK force-pushed the jamesnk/persistentstate branch from 1560b51 to 7834169 Compare July 17, 2021 00:08
@adityamandaleeka
Copy link
Member

@Tratcher

@JamesNK JamesNK force-pushed the jamesnk/persistentstate branch from f97debe to 3de7460 Compare July 20, 2021 01:24
halter73
halter73 reviewed Jul 20, 2021
View reviewed changes
src/Servers/Kestrel/Transport.Quic/src/Internal/QuicStreamContext.FeatureCollection.cs Outdated
}
}

private void InitiaizeFeatures()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private void InitiaizeFeatures()
private void InitializeFeatures()

Copy link
Member Author

Choose a reason for hiding this comment

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

I blame someone else 🐴

aspnetcore/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketConnection.FeatureCollection.cs

Line 13 in 52eff90

private void InitiaizeFeatures()

@JamesNK JamesNK enabled auto-merge (squash) July 20, 2021 01:42
@JamesNK JamesNK force-pushed the jamesnk/persistentstate branch from 3c0bb12 to f129cd6 Compare July 20, 2021 04:41
@JamesNK JamesNK merged commit 093dd61 into main Jul 20, 2021
@JamesNK JamesNK deleted the jamesnk/persistentstate branch July 20, 2021 05:56
@ghost ghost added this to the 6.0-rc1 milestone Jul 20, 2021
@ladeak ladeak mentioned this pull request Oct 17, 2021
Merged
@amcasey amcasey added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tratcher Tratcher Tratcher left review comments

@ladeak ladeak ladeak left review comments

@halter73 halter73 halter73 approved these changes

@BrennanConroy BrennanConroy Awaiting requested review from BrennanConroy BrennanConroy is a code owner

@davidfowl davidfowl Awaiting requested review from davidfowl

Assignees
No one assigned
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Milestone
6.0-rc1
Development

Successfully merging this pull request may close these issues.

Caching per-request objects on reused HttpContexts
7 participants
@JamesNK @adityamandaleeka @halter73 @davidfowl @Tratcher @ladeak @amcasey