Add QuicConnection.IsQuicSupported for runtime feature detection #31689

scalablecory · 2020-02-03T22:37:59Z

Add QuicConnection.IsQuicSupported for runtime feature detection, update tests to use it, and add a QuicNotSupportedException.

scalablecory · 2020-02-03T23:58:24Z

no merge: found an issue; correction incoming.

Wraith2 · 2020-02-04T00:10:17Z

QuicNotSupportedException doesn't really seem to offer anything that you couldn't get from NotSupportedException other than being based on QuicException, is there compelling reason not to use the existing NotSupportedException?

jkotalik · 2020-02-04T00:20:29Z

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

+            // TODO: check using MsQuicOpen return value; but there doesn't seem to be a consistent error code for this yet.
+            OperatingSystem ver = Environment.OSVersion;
+
+            if (ver.Platform != PlatformID.Win32NT || ver.Version >= new Version(10, 0, 19041, 0))


I don't think we should be checking this version of Windows here; technically, this OS version is for schannel support of TLS 1.3. There are other TLS implementation which would work on a lower windows version (I haven't tried but there are options for msquic).

I'd like to get rid of that version check all together. It is not something that MsQuicOpen currently checks, and will instead result in callbacks failing with unspecified platform-specific TLS errors.

The TODO is left to find a better way. It is unclear what exactly msquic requires from kernel to be successful. The recommendation I've got so far is that "latest is better". This version check will at least avoid loading MsQuic on versions of Windows we haven't tested it against.

My primary goal is to avoid needing to comment out source code to avoid breaking CI; that's fragile and we will have an embarrassing merge at some point.

jkotalik · 2020-02-04T00:25:25Z

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

+            {
+                try
+                {
+                    new MsQuicApi().Dispose();


Instead of newing up a MsQuicApi, can you just create a new property (ex: IsLoaded) that would be set in the private constructor and check Api.IsLoaded

… update tests to use it. Add QuicNotSupportedException.

jkotalik · 2020-02-04T00:48:26Z

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

+                uint status = Interop.MsQuic.MsQuicOpen(version: 1, out registration);
+                if (!MsQuicStatusHelper.SuccessfulStatusCode(status))
+                {
+                    throw new QuicNotSupportedException();


Yeah, if we can avoid this exception that would be nice. Just use NotSupportedException.

jkotalik

Pending some feedback and making it really clear that we should remove the OS check 😄

stephentoub · 2020-02-04T07:27:12Z

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

@@ -27,6 +27,7 @@ public sealed partial class QuicConnection : System.IDisposable
        public System.Net.Security.SslApplicationProtocol NegotiatedApplicationProtocol => throw null;
        public ValueTask CloseAsync(long errorCode, System.Threading.CancellationToken cancellationToken = default) => throw null;
        public void Dispose() => throw null;
+        public static bool IsQuicSupported => throw null;


We're still operating in a "none of this has been reviewed yet and we won't ship these APIs until we do and revise them accordingly" mode, right? Is there a must-do issue tracking that to make sure it doesn't slip through the cracks?

We're still operating in a "none of this has been reviewed yet and we won't ship these APIs until we do and revise them accordingly" mode, right?

Yes.

Is there a must-do issue tracking that to make sure it doesn't slip through the cracks?

It's been in active discussion during QUIC syncs; I'm aiming for us to have that ironed out in the next couple weeks. #31709

gfoidl · 2020-02-04T09:52:47Z

src/libraries/System.Net.Quic/src/Resources/Strings.resx

@@ -57,8 +117,10 @@
  <resheader name="writer">
    <value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
  </resheader>
-
+  <data name="net_quic_notsupported" xml:space="preserve">
+    <value>QUIC is not supported on this platform. See http://aka.ms/dotnetquic</value>


The link redirects to https://github.com/dotnet/runtime
Is this intentional or is the redirect not setup right now?

Intentional; Around release we'll have a docs page or blog post outlining how to play with HTTP/3 -- it'll get updated at that time.

maryamariyan · 2020-02-05T18:23:16Z

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

This PR is modifying a ref *.cs file. When adding/modifying a public API, 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.

scalablecory added the area-System.Net label Feb 3, 2020

scalablecory added this to the 5.0 milestone Feb 3, 2020

scalablecory requested a review from jkotalik February 3, 2020 22:37

scalablecory mentioned this pull request Feb 3, 2020

Add runtime platform support detection microsoft/msquic#92

Closed

scalablecory added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 3, 2020

jkotalik reviewed Feb 4, 2020

View reviewed changes

Add QuicConnection.IsQuicSupported for runtime feature detection, and…

f1c8a67

… update tests to use it. Add QuicNotSupportedException.

scalablecory force-pushed the quic-feature-detection branch from ae8173a to f1c8a67 Compare February 4, 2020 00:34

scalablecory removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 4, 2020

jkotalik reviewed Feb 4, 2020

View reviewed changes

jkotalik approved these changes Feb 4, 2020

View reviewed changes

stephentoub reviewed Feb 4, 2020

View reviewed changes

gfoidl reviewed Feb 4, 2020

View reviewed changes

maryamariyan added the new-api-needs-documentation label Feb 5, 2020

Cory Nelson added 3 commits February 6, 2020 16:06

WIP

851170b

WIP

03f0224

WIP

3f17420

scalablecory merged commit 796d36a into dotnet:master Feb 7, 2020

scalablecory self-assigned this Feb 12, 2020

scalablecory deleted the quic-feature-detection branch July 29, 2020 16:06

ghost locked as resolved and limited conversation to collaborators Dec 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add QuicConnection.IsQuicSupported for runtime feature detection #31689

Add QuicConnection.IsQuicSupported for runtime feature detection #31689

scalablecory commented Feb 3, 2020

scalablecory commented Feb 3, 2020

Wraith2 commented Feb 4, 2020 •

edited

Loading

jkotalik Feb 4, 2020

scalablecory Feb 4, 2020

jkotalik Feb 4, 2020

scalablecory Feb 4, 2020

jkotalik Feb 4, 2020

jkotalik left a comment

stephentoub Feb 4, 2020

scalablecory Feb 4, 2020

gfoidl Feb 4, 2020

scalablecory Feb 4, 2020

maryamariyan commented Feb 5, 2020

Add QuicConnection.IsQuicSupported for runtime feature detection #31689

Add QuicConnection.IsQuicSupported for runtime feature detection #31689

Conversation

scalablecory commented Feb 3, 2020

scalablecory commented Feb 3, 2020

Wraith2 commented Feb 4, 2020 • 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

jkotalik 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

maryamariyan commented Feb 5, 2020

Wraith2 commented Feb 4, 2020 •

edited

Loading