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

Enable HTTP/3 support by default #73153

Merged
merged 1 commit into from
Aug 2, 2022
Merged

Conversation

stephentoub
Copy link
Member

I'm surprised this wasn't done a while ago. Is there a reason for it?
Fixes #73140

@ghost
Copy link

ghost commented Aug 1, 2022

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

Issue Details

I'm surprised this wasn't done a while ago. Is there a reason for it?
Fixes #73140

Author: stephentoub
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@stephentoub
Copy link
Member Author

ps We appear to lack tests for both HTTP/2 and HTTP/3 that test toggling the switch and validating it behaves as expected. We should add those separately.

@stephentoub stephentoub added this to the 7.0.0 milestone Aug 1, 2022
@jkotas
Copy link
Member

jkotas commented Aug 1, 2022

What is it going to do to single file deployments (/p:PubliishSingleFile=true and /p:PublishAot=true)? Are they going to require the extra msquic.dll to be deployed to work?

@stephentoub
Copy link
Member Author

stephentoub commented Aug 1, 2022

Are they going to require the extra msquic.dll to be deployed to work?

Even with HTTP/3 support enabled by default, it still then checks whether QUIC is supported:

if (IsHttp3Supported())
{
_http3Enabled = _poolManager.Settings._maxHttpVersion >= HttpVersion.Version30 && QuicConnection.IsSupported;
}

which is only set to true after successfully loading msquic:

I read that as meaning if msquic.dll isn't available, HTTP/3 simply won't be available even with this toggled. Does that address your concerns? Or are you asking whether there's additional deployment work that needs to be done to ensure msquic.dll is deployed?

@jkotas
Copy link
Member

jkotas commented Aug 1, 2022

if msquic.dll isn't available, HTTP/3 simply won't be available even with this toggled.

It addressed my top concern.

The silent behavior changes when going from F5 to single file publish is still a problem. In general, we try to avoid behavior changes like this, and warn you via analyzers when it is possible to avoid them.

We can wait and see whether this one will be a problem in practice. The proper solution for the single file issue would be to start statically linking msquic copy into the single file targets.

cc @dotnet/ilc-contrib

@wfurt
Copy link
Member

wfurt commented Aug 1, 2022

AFAIK we will still ship it is preview feature... and I think we want explicit enabling. It may be different than HTTP3SUPPORT.

@stephentoub
Copy link
Member Author

AFAIK we will still ship it is preview feature

We do?? QUIC is in preview in .NET 7, but I thought we'd all agreed HTTP/3 was fully supported. @karelz?

@vitek-karas
Copy link
Member

Honestly the integration of msquic.dll seems somewhat broken. If it's a native dependency, it should be left next to the executable (for normal single-file, not sure what NativeAOT does, but it might do the same). The problem is that we don't want to to this by default - each app would get it and it sort of break single file. Since in .NET 6 simple hello world produces exactly one file, now it wouldn't.

What is pretty weird is that we include the .dll in self-contained, but it effectively disappears for single-file. @VSadov - who might know what's going on there.

For example the native dependencies of WPF behave the right way - they are left next to the exe.

If we ship this way - if there's an API which enables QUIC or which requires QUIC it should be marked it with RequiresAssemblyFiles - /FYI @tlakollo

@jkotas
Copy link
Member

jkotas commented Aug 1, 2022

If we ship this way - if there's an API which enables QUIC or which requires QUIC it should be marked it with

Quic is used for http (HttpClient) and everything built on top of it. It is not practical to mark this whole surface like that.

@jkotas
Copy link
Member

jkotas commented Aug 1, 2022

What is pretty weird is that we include the .dll in self-contained, but it effectively disappears for single-file.

We assume that all inbox netcoreapp native dependencies have been linked into single file host and thus it is safe to exclude them during publish. msquic violates this assumption.

@wfurt
Copy link
Member

wfurt commented Aug 1, 2022

I think you may be right @stephentoub. I'll check with @karelz

more on the single-file: This gets more tricky as we don't (for security reasons) ship libmsquic.[so|dylib] on Unix platforms. HTTP/3 will light-up if given system has compatible version of msquic available.

@jkotas
Copy link
Member

jkotas commented Aug 1, 2022

HTTP/3 will light-up if given system has compatible version of msquic available.

Light-up based on the OS features is ok. We have number of places where we do that, e.g. for globalization.

We are concerned about behavior differences between dotnet run and dotnet publish (running on the same OS in both cases).

@vitek-karas
Copy link
Member

If we ship this way - if there's an API which enables QUIC or which requires QUIC it should be marked it with

Quic is used for http (HttpClient) and everything built on top of it. It is not practical to mark this whole surface like that.

I agree, what I meant is if there's an API which is Quic specific (like EnableQuic or something along those lines), then that should be annotated.

@stephentoub
Copy link
Member Author

if there's an API which is Quic specific (like EnableQuic or something along those lines), then that should be annotated.

There isn't. There's an IsSupported get-only property that reflects whether an underlying QUIC implementation is available.

@karelz
Copy link
Member

karelz commented Aug 2, 2022

Triage:

  • HTTP/3 should be enabled by default
  • For single-file we need separate discussion - let's move it to new issue (TBD to be linked)
    • On Windows we include msquic in .NET Runtime -- we should decide how and where to make it part of single-file publish.
    • On Linux we depend on OS package (globally installed), so it is entirely external component to .NET Runtime

@karelz karelz requested a review from rzikm August 2, 2022 16:50
@ManickaP
Copy link
Member

ManickaP commented Aug 2, 2022

Just a note to all this, with H/3 enabled by default, you might get an upgrade via Alt-Svc and switch to H/3 behind the scenes. That was the main reason we put it behind app switch. If we're fine with this, let's proceed, I have no qualms about this change.

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@stephentoub
Copy link
Member Author

with H/3 enabled by default, you might get an upgrade via Alt-Svc and switch to H/3 behind the scenes. That was the main reason we put it behind app switch

If we leave it disabled via that switch by default, we're effectively saying HTTP/3 isn't supported and very few folks are going to use it. Presumably that's not our intent?

If we're concerned about the Alt-Svc upgrade auto-upgrading 1.1 or 2.0 requests to 3.0, then that should presumably be considered as a separate mechanism to opt-in/out that specifically.

@ManickaP
Copy link
Member

ManickaP commented Aug 2, 2022

If we're concerned about the Alt-Svc upgrade auto-upgrading 1.1 or 2.0 requests to 3.0, then that should presumably be considered as a separate mechanism to opt-in/out that specifically.

We do have one, you have to either request 3.0 version or enable upgrade via RequestVersionOrHigher.

EDIT: and default is RequestVersionOrLower so nobody should get any surprises if using the defaults.

@stephentoub
Copy link
Member Author

stephentoub commented Aug 2, 2022

Then I don't understand the stated concern "with H/3 enabled by default, you might get an upgrade via Alt-Svc and switch to H/3 behind the scenes"...? This PR isn't changing the default 1.1 in HttpRequestMessage, it's not changing the default RequestVersionOrLower on HttpClient, etc.; it's just changing whether HTTP/3 can be used at all by default.

@ManickaP
Copy link
Member

ManickaP commented Aug 2, 2022

The only combination I can think of, that this would change behavior, is using RequestVersionOrHigher and specifying H/1, but expecting only upgrade to H/2. With this change, you can upgrade to H/3 as well.

As I said before, I'm not against this change, I just wanted to add one of the reasons we discussed when adding this switch originally.

@stephentoub
Copy link
Member Author

I see. If you've said you're ok with the request version or higher, congratulations, we're honoring your request :) This is the nature of HttpVersionPolicy, and if memory serves, this was all discussed at the time that unbounded upwards policy option was added.

@stephentoub stephentoub merged commit 2642c54 into dotnet:main Aug 2, 2022
@stephentoub stephentoub deleted the http3enabled branch August 2, 2022 18:54
@karelz
Copy link
Member

karelz commented Aug 3, 2022

FYI: As promised above, I filed follow up work items for 8.0 for Single-file #73290 and NativeAOT #73291
We can continue technical discussion there.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 2, 2022
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.

HTTP/3: Enable by default?
7 participants