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

API review for QUIC APIs #48686

Closed
scalablecory opened this issue Feb 24, 2021 · 18 comments
Closed

API review for QUIC APIs #48686

scalablecory opened this issue Feb 24, 2021 · 18 comments

Comments

@scalablecory
Copy link
Contributor

scalablecory commented Feb 24, 2021

Placeholder for API review of QUIC APIs.

@ManickaP @CarnaViire @karelz we should try to get all the APIs designed soon. The API review will take some time (probably 4-8hr). Also mindful of any time @halter73 @jkotalik need to prove the APIs out in Kestrel.


Tracking API changes:

class QuicListener
{
-   public void Start(); // merged into constructor
}

class QuicStream
{
// this may be changed later after discussions; just want to be sure this change is not lost
+   public ValueTask ShutdownCompleted(CancellationToken cancellationToken = default);
}
@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

Placeholder for API review of QUIC APIs.

@ManickaP @CarnaViire @karelz we should try to get all the APIs designed soon. The API review will take some time (probably 4-8hr). Also mindful of any time @halter73 @jkotalik need to prove the APIs out in Kestrel.

Author: scalablecory
Assignees: -
Labels:

area-System.Net.Quic

Milestone: 6.0.0

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

I created #44786 a while back to try to capture the existing open issues. It may be missing some issues.

That said, we need to do a review of the entire API surface.

@wegylexy
Copy link
Contributor

I wish there's a way to use the underlying QUIC connection of HTTP/3 to send/receive custom datagrams, such that unreliable data may be multiplexed along the same SignalR connection. See also wegylexy#1

@mclayton7
Copy link
Contributor

@wegylexy I really like the idea of adding datagram support and hope this gets in! Would it be possible to use an async method instead of an event for the datagram receive? I think this would be more in line with the async socket apis.

@mclayton7
Copy link
Contributor

Looking through some of the existing QUIC implementation in Kestrel, and the work @scalablecory has done to support msquic 1.0, it would be great if the hooks were available to support a 3rd party WebTransport/QuicTransport implementation in Kestrel. I think it would just require exposing the Kestrel QUIC classes and letting someone write a thin layer to set the correct apln.

@wegylexy
Copy link
Contributor

@wegylexy I really like the idea of adding datagram support and hope this gets in! Would it be possible to use an async method instead of an event for the datagram receive? I think this would be more in line with the async socket apis.

Since msquic doesn't transfer ownership of the received datagram buffer, it must be consumed synchronously (e.g. through a ReadOnlySpan<byte>) to avoid copying the buffer. As such, raising an event / callback seems to be the only option for now. A typical use case would parse the buffer quickly into another class/struct, and then that new object could be queued asynchronously.
There is also an issue with the current socket APIs for streams as the shared resettable completion source leads to undefined behavior on overlapped async operations.

@scalablecory
Copy link
Contributor Author

@wegylexy please create a new issue with proposed API.

Note the datagram RFC has not been finished yet, so we will likely not support this in .NET 6.

@wegylexy
Copy link
Contributor

msquic has changed a lot too? Session is now Configuration and the .NET 6 preview API doesn't work with v1.0.0 of msquic.dll unless I missed something.

@ManickaP
Copy link
Member

We have a dependency on a certain msquic revision, see the git submodule here: https://github.com/dotnet/runtimelab/tree/feature/System.Net.Experimental.MsQuic/src
We do plan to update to the newer msquic version, but that's work in progress.

@wegylexy
Copy link
Contributor

Significant changes since microsoft/msquic#777 but this is the first stable supported release version.

@mclayton7
Copy link
Contributor

@wegylexy @scalablecory has done some work here (https://github.com/scalablecory/runtime/tree/msquic-update) on updating System.Net.Quic to work with the 1.0.0 release of msquic.

@wegylexy
Copy link
Contributor

wegylexy commented Feb 26, 2021

Great, it works. And I put back support for certificate file for OpenSSL, hoping it to work on Linux but haven't got the time to test yet. scalablecory#1

@karelz karelz modified the milestones: 6.0.0, Future May 18, 2021
@wegylexy
Copy link
Contributor

wegylexy commented May 27, 2021

Played around with msquic 1.3.0 openssl on Linux and Windows, and it works. Datagram API in my fork also works.

The only thing is that it always wastes time to verify the chain, even for self-signed certs without a chain, even when the validation callback is simply (_, _, _, _) => true.

@wegylexy
Copy link
Contributor

@mclayton7 async receipt of datagrams would hurt performance, see microsoft/msquic#1654
@scalablecory QUIC is finally RFCed

@scalablecory
Copy link
Contributor Author

scalablecory commented Jun 1, 2021

@scalablecory QUIC is finally RFCed

The datagram spec is still not RFCed. We'll likely be ready to consider a datagram API for .NET 7.

I'm aware you've prototyped it, and would be really happy to see you involved in API discussion and a PR from you once we're ready for it. If you can create a new issue with the proposed datagram APIs, that's where we'll want to start.

@wegylexy
Copy link
Contributor

wegylexy commented Jun 1, 2021

created #53533 for datagram API suggestion just now

@wegylexy
Copy link
Contributor

wegylexy commented Jun 1, 2021

oh, I see. it's still draft-ietf-quic-datagram-02.

@karelz karelz modified the milestones: Future, 7.0.0 Oct 26, 2021
@karelz
Copy link
Member

karelz commented Oct 26, 2021

Triage: We will create new issues for formal API reviews based on work in #44786. Closing

@karelz karelz closed this as completed Oct 26, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Nov 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants