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

Sync shared code with aspnetcore for HTTP/3 #32779

Merged
merged 1 commit into from
Feb 27, 2020

Conversation

jkotalik
Copy link
Contributor

@scalablecory / @Tratcher I removed the OS check for IsQuicSupport for now; I was hitting issues where the version in aspnetcore didn't match the version in runtime. @Tratcher also had pushback on this check in general. We can discuss what we'd like to do here.

@jkotalik jkotalik requested review from Tratcher, scalablecory and a team February 25, 2020 02:14
@jkotalik jkotalik changed the title Sync shared code with runtime for HTTP/3 Sync shared code with aspnetcore for HTTP/3 Feb 25, 2020
@scalablecory
Copy link
Contributor

What is @Tratcher's concern? I would prefer to just fix it rather than remove.

@Tratcher
Copy link
Member

Cory
#32575

@Tratcher
Copy link
Member

Without this check, what's the error you get from any earlier version of Windows? Does it just fail on the next new MsQuicApi() with a NotSupportedException?

@scalablecory
Copy link
Contributor

Cory
#32575

Indeed, I'd rather fix that than get rid of it. @jkotas's suggestion is exactly what I had in mind and I bet there's some P/invoke signature for that somewhere in corefx already that we can use.

Without this check, what's the error you get from any earlier version of Windows? Does it just fail on the next new MsQuicApi() with a NotSupportedException?

No, and that's the problem. msquic fails with unspecified error codes at unspecified points (whenever it tries to use TLS for the first time, who knows when that is... probably when it connects) when platform support is not there, and the error codes are different depending on the platform it targets. Not very useful behavior from a library.

We know TLS 1.3 is required, and so we can check for TLS 1.3 availability (I'm sitting on a patch to do that), but it also requires additional APIs beyond just basic TLS 1.3 support, so we can't leave it there.

My goal is to fast-fail with a useful messages for users, and I'm hoping msquic is enhanced in the future so we can remove the check all together.

@Tratcher
Copy link
Member

As long as the fail-fast is accurate.

@stephentoub
Copy link
Member

stephentoub commented Feb 25, 2020

I bet there's some P/invoke signature for that somewhere in corefx already that we can use.

RuntimeInformation.OSDescription:


wraps a (wrapper to a) P/Invoke to it:
[DllImport(Libraries.NtDll, ExactSpelling = true)]
private static extern int RtlGetVersion(ref RTL_OSVERSIONINFOEX lpVersionInformation);

@Tratcher
Copy link
Member

Can we merge this as is to get the shared source back in sync and address #32575 separately?

@jkotalik
Copy link
Contributor Author

A few things:

  • If we are using msquic with stub tls, we wouldn't need an updated version of Windows. You really only need this when using schannel. I constantly needed to uncomment this check when using stub tls. Maybe we can fix this via an app context switch or something like that.
  • This check doesn't work in Kestrel which I need clarification on. https://github.com/dotnet/runtime/pull/32779/files#diff-0482602b7073d6fa3fa809fa90d9130dL144. Why does Environment.OSVersion return different values between AspNetCore and Runtime?
  • Right now, as the System.Net.Quic code is shared between Kestrel and Runtime, pinvoking is a bit more annoying.

@jkotalik
Copy link
Contributor Author

I'm with @Tratcher here, I'd rather unblock Kestrel from using msquic in general (stub or tls) and figure out a better fix in a follow up PR.

@scalablecory
Copy link
Contributor

If we are using msquic with stub tls

I don't think this is a scenario we should support. It is no longer "QUIC" in that case, and so QUIC is indeed "not supported".

Right now, as the System.Net.Quic code is shared between Kestrel and Runtime, pinvoking is a bit more annoying.

How so?

@Tratcher Tratcher merged commit 40e44e7 into dotnet:master Feb 27, 2020
@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants