[QUIC] API Update #49823

ManickaP · 2021-03-18T14:15:48Z

Updated API to microsoft/msquic@cc104e8, branched in my fork: https://github.com/ManickaP/msquic/commits/mapichov/net6_2020_03_18

The change mostly taken from https://github.com/scalablecory/runtime/tree/msquic-update. Plus: updated the msquic version to the newest main, adjusted the API, made compilable, fixed obvious test errors.

Fixes #44580, #31695

It still needs more work, but I don't want to hold msquic API update on refactorings / clean ups.

dotnet-issue-labeler · 2021-03-18T14:15:53Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2021-03-18T14:15:59Z

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

Issue Details

Updated API to microsoft/msquic@cc104e8, branched in my fork: https://github.com/ManickaP/msquic/commits/mapichov/net6_2020_03_18

The change mostly taken from https://github.com/scalablecory/runtime/tree/msquic-update

Fixes #44580, #31695

Author:	ManickaP
Assignees:	-
Labels:	`area-System.Net.Quic`, `new-api-needs-documentation`
Milestone:	-

jkotalik · 2021-03-18T17:36:31Z

src/libraries/System.Net.Quic/src/System.Net.Quic.csproj

    <Content Include="libmsquic.so" Condition="Exists('libmsquic.so')">
      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
      <CopyToPublishDirectory>PreserveNewest</CopyToPublishDirectory>
    </Content>
-    <Content Include="msquic.pdb" Condition="Exists('msquic.pdb')">
+    <Content Include="libmsquic.lttng.so" Condition="Exists('libmsquic.lttng.so')">


@scalablecory could we reference the System.Net.Experimental.Quic here instead now? https://github.com/dotnet/runtimelab/tree/feature/System.Net.Experimental.MsQuic

You won't need that package for .NET 6.

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicApi.cs

...aries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/MsQuicAlpnHelper.cs

jkotalik · 2021-03-18T17:44:25Z

...es/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/MsQuicNativeMethods.cs

@@ -299,58 +446,68 @@ internal struct ConnectionEvent
            IntPtr context,
            ref ConnectionEvent connectionEvent);

+        // TODO: order is Open, Close, Shutdown, Start, SetConfiguration, SendResumptionTicket


Can this be removed now?

Nope, that's my comment. The order (in the comment) is taken from msquic.h, we do not follow that in here. I didn't want to generate more diffs in this PR. I'll plan to do follow up that does not change the behavior at all, just shuffles things around to be more like msquic.h.

JamesNK · 2021-03-19T00:28:50Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicListener.cs

-                    default:
-                        return MsQuicStatusCodes.InternalError;
-                }
+                return MsQuicStatusCodes.InternalError;


What is the plan for logging inside System.Net.Quic?

For example, here there should be a log message that an unknown callback was received, including the unexpected evt.Type value. A generic InternalError status code isn't enough to understand what went wrong.

(logging can be added in the future, not blocking for this PR)

Found this - #32072

JamesNK · 2021-03-19T00:32:50Z

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicOptions.cs

+        /// Default is 100.
+        /// </summary>
+        // TODO consider constraining these limits to 0 to whatever the max of the QUIC library we are using.
+        public long MaxBidirectionalStreams { get; set; } = 100;


Need validation to stop invalid values (e.g. 0 or -50). Applies to all options.

Create an issue if you would prefer to do it in a follow up PR.

It's part of this #32079:

How should we be validating these limits?

JamesNK · 2021-03-19T00:34:08Z

src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTestBase.cs

    public class MsQuicTestBase
    {
        public SslServerAuthenticationOptions GetSslServerAuthenticationOptions()
        {
            return new SslServerAuthenticationOptions()
            {
-                ApplicationProtocols = new List<SslApplicationProtocol>() { new SslApplicationProtocol("quictest") }
+                ApplicationProtocols = new List<SslApplicationProtocol>() { new SslApplicationProtocol("quictest") },
+                // TODO: use a cert. MsQuic currently only allows certs that are trusted.


What is the cause of this trusted limitation? Is it a requirement of QUIC, or because we haven't added configuration to ignore invalid certificates yet?

we may need to plumb custom validation callback to managed code. I'm not sure if that was already done.

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs

wfurt · 2021-03-24T04:45:17Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs

+            _state.Connection = this;
+            try
+            {
+                uint status = MsQuicApi.Api.ConnectionStartDelegate(


Would it make sense to have QuicStatus enum here? While the list of errors may grow, there are really only three interesting values IMHO. We do that in some other places and it make debugging much easier as you can see the enum instead of number.

We already have something: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Quic/src/System/Net/Quic/Interop/MsQuicStatusCodes.cs
The codes differ on different platforms, that's why it's separate. Could we define an enum per platform and use it? I guess we could. Is it worth it? Depends on what we decide to do about the generated interop.
Otherwise, better messages are covered by #32066.

wfurt · 2021-03-24T04:51:24Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/MsQuicEnums.cs

+
+namespace System.Net.Quic.Implementations.MsQuic.Internal
+{
+    internal enum QUIC_EXECUTION_PROFILE : uint


I'm really NOT the style guy but in some other places we made the names C# friendly.

runtime/src/libraries/Common/src/System/Net/SecurityStatusPal.cs

Lines 25 to 30 in 79ae74f

internal enum SecurityStatusPalErrorCode

{

NotSet = 0,

OK,

ContinueNeeded,

CompleteNeeded,

and we use int unless uint is really needed (even if underlying C is uint)

I won't change the naming scheme in this PR, that's too revolutionary 😃.
We might change it in a separate one or we might go a different direction if we chose to generate the interop.

...es/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/MsQuicNativeMethods.cs

wfurt · 2021-03-24T06:23:18Z

...Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/SafeMsQuicConfigurationHandle.cs

+            QuicBuffer[]? buffers = null;
+            try
+            {
+                MsQuicAlpnHelper.Prepare(alpnProtocols, out handles, out buffers);


I don't think we need safe handles here. Since this only needs preserve through the call and never be exposed outside of this function I'm wondering if there is easier way. I'm sure the handles will work but feel heavy.

I see:
https://github.com/microsoft/msquic/blob/55c15aeb4f82335959fefa86005e69d1e7a81c58/src/core/configuration.c#L96-L105

But is this something we can safely assume will be true even in the future? Docs don't say anything about it: https://github.com/microsoft/msquic/blob/main/docs/api/ConfigurationOpen.md

cc: @nibanks

Yes, you can rely on MsQuic to copy what it needs in the function call. The memory doesn't have to exist beyond that,

Ultimately I'd love to have a configuration API -- something like SocketsHttpHandler really doesn't need more than a single configuration, while the current code will allocate one for every new connection it opens.

Ultimately I'd love to have a configuration API -- something like SocketsHttpHandler really doesn't need more than a single configuration, while the current code will allocate one for every new connection it opens.

Creating a configuration is a very heavy weight operation. I'd highly recommend one of the next things you do is to refactor this like @scalablecory mentions.

Does that mean that ConfigurationLoadCredential might be called multiple times on the same configuration with different certificate?
Or how should I go about sharing the configuration?

Am I wrong in my train of thought: instance of SocketsHttpHandler corresponds to one connection pool which corresponds to a single QuicConnection which holds one configuration.

Generally, you create one configuration for each unique security configuration. On server that usually maps 1:1 with each server certificate. On client most times there is only one, unless they're creating clients with different configurations.

(This is captured in #42642)

wfurt

While I think there is opportunity for improvements I feel there is value in getting something in and working in improvements over time. This is not cast in stone and underlying code may be fluid.

scalablecory

lgtm

scalablecory · 2021-03-25T13:53:32Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/Mock/MockConnection.cs

@@ -91,7 +91,9 @@ internal override bool Connected
        }

        // TODO: Should clone the endpoint since it is mutable
-        internal override IPEndPoint LocalEndPoint => _localEndPoint;
+        // TODO: could this be made back to non-nullable?


If we get rid of instance method ConnectAsync and make a static ValueTask<QuicConnection> QuicConnection.ConnectAsync(), then we can always have LocalEndPoint be non-null.

I consider the TODO as work that needs to be done/resolved. But I don't want to hold the PR on those issues. The work will be done in follow up PRs.

scalablecory · 2021-03-25T13:57:14Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicApi.cs


-        private readonly IntPtr _registrationContext;
+            SetParamDelegate =
+                Marshal.GetDelegateForFunctionPointer<SetParamDelegate>(


We should consider using function pointers instead of delegates.

It's been pointed out by Stephen and is captured here:

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicApi.cs

Line 124 in 5d3884f

// TODO: Consider updating all of these delegates to instead use function pointers.

It's on the plate.

scalablecory · 2021-03-25T14:47:54Z

...Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/SafeMsQuicConfigurationHandle.cs

+            QuicBuffer[]? buffers = null;
+            try
+            {
+                MsQuicAlpnHelper.Prepare(alpnProtocols, out handles, out buffers);


Ultimately I'd love to have a configuration API -- something like SocketsHttpHandler really doesn't need more than a single configuration, while the current code will allocate one for every new connection it opens.

scalablecory · 2021-03-25T14:50:45Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs

-        private GCHandle _handle;
+        // TODO: remove this.
+        // This is only used for client-initiated connections, and isn't needed even then once Connect() has been called.
+        private readonly SafeMsQuicConfigurationHandle? _configuration;


This is another thing that'd be removable if we switched to a static Connect as in a former comment.

I consider the TODO as work that needs to be done/resolved. But I don't want to hold the PR on those issues. The work will be done in follow up PRs.

jkotalik

💯

ManickaP · 2021-03-26T16:46:47Z

Only waiting for the build to finish, then I'll merge.

ManickaP added 9 commits March 12, 2021 17:12

Cherry picked change from scalablecory/msquic-update.

6bf36e4

Enum rename _FLAG --> _FLAGS to follow msquic naming.

e96522e

Enums updated

b92bd1d

Updated interop, compilable project.

7aa745c

Somewhat working update of msquic API

6a132c5

Merge main

1d0d98c

Post-merge compilation fixes.

68a5c48

Fixed most of the tests, one turned into ActiveIssue

851dc1b

Comments.

78e3981

dotnet-issue-labeler bot added area-System.Net.Quic new-api-needs-documentation labels Mar 18, 2021

Regenerated ref sources.

7ad5163

ManickaP marked this pull request as ready for review March 18, 2021 14:26

ManickaP requested review from scalablecory, CarnaViire, JamesNK, geoffkizer and jkotalik March 18, 2021 14:29

jkotalik reviewed Mar 18, 2021

View reviewed changes

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicApi.cs Outdated Show resolved Hide resolved

jkotalik reviewed Mar 18, 2021

View reviewed changes

JamesNK reviewed Mar 19, 2021

View reviewed changes

Addressed some feedback.

f67ed0d

ManickaP mentioned this pull request Mar 22, 2021

Avoid multiple exceptions at startup from MsQuic support tests #49973

Merged

Added readme.

204f4fa

wfurt reviewed Mar 24, 2021

View reviewed changes

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs Outdated Show resolved Hide resolved

wfurt reviewed Mar 24, 2021

View reviewed changes

...es/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/MsQuicNativeMethods.cs Show resolved Hide resolved

wfurt reviewed Mar 24, 2021

View reviewed changes

wfurt approved these changes Mar 24, 2021

View reviewed changes

ManickaP added 2 commits March 25, 2021 11:32

Merge branch 'main' into mapichov/44580_quic_update

72ea176

QUIC_ADDRESS_FAMILY

5d3884f

scalablecory approved these changes Mar 25, 2021

View reviewed changes

runfoapp bot mentioned this pull request Mar 25, 2021

Builds legs getting abandoned due to agent notification issues #35223

Closed

This comment has been minimized.

Sign in to view

Merge branch 'main' into mapichov/44580_quic_update

c0f896f

jkotalik approved these changes Mar 26, 2021

View reviewed changes

ManickaP merged commit 484a807 into dotnet:main Mar 26, 2021

ManickaP deleted the mapichov/44580_quic_update branch March 26, 2021 17:21

ManickaP mentioned this pull request Apr 6, 2021

[QUIC] Update MsQuic GCHandles #31695

Closed

ManickaP mentioned this pull request Apr 15, 2021

[QUIC] Don't leak QUIC session upon QuicListener disposal #32073

Closed

ghost locked as resolved and limited conversation to collaborators Apr 25, 2021

karelz added this to the 6.0.0 milestone May 20, 2021

	internal enum SecurityStatusPalErrorCode
	{
	NotSet = 0,
	OK,
	ContinueNeeded,
	CompleteNeeded,

[QUIC] API Update #49823

[QUIC] API Update #49823

Conversation

ManickaP commented Mar 18, 2021 • edited Loading

dotnet-issue-labeler bot commented Mar 18, 2021

ghost commented Mar 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesNK Mar 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wfurt left a comment

Choose a reason for hiding this comment

scalablecory left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

jkotalik left a comment

Choose a reason for hiding this comment

ManickaP commented Mar 26, 2021

ManickaP commented Mar 18, 2021 •

edited

Loading

JamesNK Mar 19, 2021 •

edited

Loading