Skip to content
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

Abstracting QUIC #48685

Open
scalablecory opened this issue Feb 24, 2021 · 10 comments
Open

Abstracting QUIC #48685

scalablecory opened this issue Feb 24, 2021 · 10 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Quic
Milestone

Comments

@scalablecory
Copy link
Contributor

Similar to how Stream abstracts a Socket, we need some sort of abstraction for QuicStream, QuicListener, and QuicConnection.

This would allow for improved testing as well as providing the abstracts for #48617 to use.

Something I've wanted to try for this is to have these classes be the actual abstraction, and move their implementation/construction behind a QuicConnection.ConnectAsync and QuicConnection.CreateListener.

@scalablecory scalablecory added this to the 6.0.0 milestone Feb 24, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Feb 24, 2021
@ghost
Copy link

ghost commented Feb 24, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Similar to how Stream abstracts a Socket, we need some sort of abstraction for QuicStream, QuicListener, and QuicConnection.

This would allow for improved testing as well as providing the abstracts for #48617 to use.

Something I've wanted to try for this is to have these classes be the actual abstraction, and move their implementation/construction behind a QuicConnection.ConnectAsync and QuicConnection.CreateListener.

Author: scalablecory
Assignees: -
Labels:

area-System.Net.Quic

Milestone: 6.0.0

@scalablecory scalablecory removed the untriaged New issue has not been triaged by the area owner label Feb 24, 2021
@geoffkizer
Copy link
Contributor

I'm not sure we need an abstraction for QuicListener. And I think we already have an abstraction for QuicStream -- it's just Stream.

I agree re QuicConnection. We should consider adding a base class that abstracts the concept of a multiplexed connection.

@karelz karelz added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Feb 25, 2021
@scalablecory
Copy link
Contributor Author

Ultimately I want to have everything needed to support SocketsHttpHandler.QuicConnectCallback.

And I think we already have an abstraction for QuicStream -- it's just Stream.

There are some QUIC-specific APIs like waiting for stream shutdown ACK and aborting a stream with an error code.

We should consider adding a base class that abstracts the concept of a multiplexed connection.

Yeah. This bleeds into the connection abstraction stuff. At a minimum I'd like to have a thought experiment on how we could in the future insert some abstract MultiplexedConnection type.

I'm not sure we need an abstraction for QuicListener

Yeah, we don't have a socket listener abstraction currently, so we can probably do without it.

@scalablecory
Copy link
Contributor Author

CC @Tratcher @halter73 @jkotalik @JamesNK

Having an abstraction was previously a high-pri item for you; is this still the case? What do you plan to use this for?

We need to figure out abstraction model for 6.0.

@JamesNK
Copy link
Member

JamesNK commented Apr 22, 2021

@davidfowl

I think we've already solved this on our side. We introduced a multiplexed connection type in ASP.NET Core's transport abstractions.

See https://github.com/dotnet/aspnetcore/tree/9817a811c1fe6034b112e4fde84d2f4271492dc5/src/Servers/Connections.Abstractions/src

@halter73
Copy link
Member

halter73 commented Apr 23, 2021

We had the idea that Kestrel and HttpClient should consolidate on a transport abstraction in System.Net.Connections and Microsoft.AspNetCore.Connections would disappear. #1793.

We wanted to land the non-multiplexed version of this for .NET 5, but that didn't pan out. Sharing a transport abstraction between Kestrel and HttpClient without breaking behavior or regressing performance is no small task though. Trust me 😄.

Adding QUIC to the mix will only add to that, but at least there would be less legacy behavior to maintain.

@scalablecory
Copy link
Contributor Author

@halter73 if I understand correctly, nothing in Kestrel is depending directly on QuicStream, QuicConnection, beyond the abstraction implementation? You're not using the QuicProvider stuff at all?

@JamesNK
Copy link
Member

JamesNK commented Apr 27, 2021

Correct. The Microsoft.AspNetCore.Server.Kestrel.Transport.Quic assembly uses QUIC types and nothing else.

QuicProvider is not used: https://github.com/dotnet/aspnetcore/search?q=QuicProvider

@JamesNK
Copy link
Member

JamesNK commented Apr 28, 2021

If you want to see how ASP.NET Core is using QUIC types, the transport source is here: https://github.com/dotnet/aspnetcore/tree/main/src/Servers/Kestrel/Transport.Quic/src

@karelz karelz modified the milestones: 6.0.0, Future May 18, 2021
@karelz karelz modified the milestones: Future, 7.0.0 Sep 21, 2021
@ManickaP ManickaP self-assigned this Nov 2, 2021
@ManickaP
Copy link
Member

Punting to future, the reasoning is stated here: #64449. We don't plan to introduce abstractions for Quic classes in 7.0, but we're keeping our doors open to do it in the next release.

@ManickaP ManickaP modified the milestones: 7.0.0, Future Jan 28, 2022
@ManickaP ManickaP removed their assignment Jan 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Quic
Projects
None yet
Development

No branches or pull requests

6 participants