H/3 and Quic AppContext switch #55332
Conversation
|
Note regarding the 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. |
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsH/3 and Quic are considered preview for .NET 6.0. We'll guard the usage with app context switch and potentially with Changes:
This change is also replacement for renaming S.N.Quic to private (#54720), which we decided not to bother with. Fixes #55024
|
| @@ -6,13 +6,15 @@ | |||
|
|
|||
There was a problem hiding this comment.
This is marked as <IsNETCoreAppRef>false</IsNETCoreAppRef> in .csproj and there is no nuget package either. I think it means there is no way for people out there to call these APIs even if they opt-in in to preview features.
Do we need to remove <IsNETCoreAppRef>false</IsNETCoreAppRef> ?
There was a problem hiding this comment.
<IsNETCoreAppRef>false</IsNETCoreAppRef> is on purpose. The thinking here is to discourage people from using S.N.Quic, which AFAIU is still possible with direct reference to the implementation lib.
However, I do think we'll get rid of all the attributes in this PR, since it's just redundancy to the app context switch.
cc: @jeffhandley
There was a problem hiding this comment.
Our current general thinking is to encourage people to try preview features if they are interested. More details is in https://github.com/dotnet/designs/blob/main/accepted/2021/preview-features/preview-features.md
What makes S.N.Quic preview different from the generic numeric API preview that we would want to discourage one, and encourage the other?
cc @terrajobst
There was a problem hiding this comment.
The shape of the API will change in 7.0, that's why it's private and that's why we don't want anyone to depend on it. And if so, to jump through some hoops to be aware of limited "liability" (e.g. backward non-compatibility post 6.0). Maybe I'm not expressing myself well.
But as I said before, this is to showcase it and kick off a discussion (which it did) and then decide what to leave in.
There was a problem hiding this comment.
Preview feature is HTTP/3, not quic. So we want to encourage users to try quic indirectly - it is certainly not ready for direct consumption.
There was a problem hiding this comment.
I understand the concern about compatibility. It is why we have invented RequiresPreviewFeatures and the associated analyzer.
RequiresPreviewFeatures attribute and analyzer is the only hoop we have for other preview features. The APIs for those other preview features can and will change in .NET 7 too.
My question is whether there is something fundamentally different about Quic to deserve more hoops or different hoops.
There was a problem hiding this comment.
So we want to encourage users to try quic indirectly - it is certainly not ready for direct consumption.
I read it as that it is not worth it for anybody to look at and try the APIs in System.Net.Quic and provide feedback, we know that these APIs are not good. Is that correct?
If it is the case, I agree that it should stay private, and System.Net.Quic does not need any RequiresPreviewFeaturesAttribute annotations.
There was a problem hiding this comment.
I read it as that it is not worth it for anybody to look at and try the APIs in System.Net.Quic and provide feedback, we know that these APIs are not good. Is that correct?
Yes. We basically de-prioritized the QUIC API in order to focus on delivering HTTP3.
There was a problem hiding this comment.
My take from this discussion is to get rid of the attributes in S.N.Quic, but keep the one for H/3 version in (even though you still can create the version manually without any warnings).
And the AppContext switch will stay as it is.
ManickaP
force-pushed
the
mapichov/55024_h3_preview
branch
from
1381924 to
1475914
Compare
| @@ -208,6 +208,7 @@ public static partial class HttpVersion | |||
| public static readonly System.Version Version10; | |||
| public static readonly System.Version Version11; | |||
| public static readonly System.Version Version20; | |||
| [System.Runtime.Versioning.RequiresPreviewFeaturesAttribute] | |||
There was a problem hiding this comment.
This is still debatable since we dropped the preview feature from S.N.Quic.
There was a problem hiding this comment.
Removed as discussed offline.
ManickaP
force-pushed
the
mapichov/55024_h3_preview
branch
2 times, most recently
from
17b2109 to
4a90439
Compare
| // by an AppContext switch, or by an environment variable being set to true/1. | ||
| public static bool AllowHttp3AndQuic { get; } = RuntimeSettingParser.QueryRuntimeSettingSwitch( | ||
| "System.Net.SocketsHttpHandler.Http3AndQuicSupport", | ||
| "DOTNET_SYSTEM_NET_HTTP_SOCKETSHTTPHANDLER_HTTP3ANDQUICSUPPORT", |
There was a problem hiding this comment.
Was the name of the switch discussed/approved somewhere? I just wonder why it was changed
There was a problem hiding this comment.
I just wonder why it was changed
To express that it's used for S.N.Quic as well, which the original switch wasn't. Since S.N.Quic is "hidden" and shouldn't be used, it's not strictly needed. Not even I myself am 100% sure about the naming here 😄
About discussing it, it's happening here. AFAIK, switches are not strictly considered public API (e.g. #47583) so no need to go through API Review.
There was a problem hiding this comment.
Renamed as discussed offline.
ManickaP
force-pushed
the
mapichov/55024_h3_preview
branch
from
4a90439 to
c9f0a8c
Compare
|
Hello @ManickaP! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
|
Failing test are preexisting. |
…debugger_custom_views * 'main' of github.com:thaystg/runtime: (125 commits) [wasm] [debugger] Support method calls (dotnet#55458) [debugger] Fix debugging after hot reloading (dotnet#55599) Inliner: Extend IL limit for profiled call-sites, allow inlining for switches. (dotnet#55478) DiagnosticSourceEventSource supports base class properties (dotnet#55613) [mono] Fix race during mono_image_storage_open (dotnet#55201) [mono] Add wrapper info for native func wrappers. (dotnet#55602) H/3 and Quic AppContext switch (dotnet#55332) Compression.ZipFile support for Unix Permissions (dotnet#55531) [mono] Fix skipping of static methods during IMT table construction. (dotnet#55610) Combine System.Private.Xml TrimmingTests projects (dotnet#55606) fix name conflict with Configuration class (dotnet#55597) Finish migrating RSAOpenSsl from RSA* to EVP_PKEY* Disable generic math (dotnet#55540) Obsolete CryptoConfig.EncodeOID (dotnet#55592) Address System.Net.Http.WinHttpHandler's nullable warnings targeting .NETCoreApp (dotnet#54995) Enable Http2_MultipleConnectionsEnabled_ConnectionLimitNotReached_ConcurrentRequestsSuccessfullyHandled (dotnet#55572) Fix Task.WhenAny failure mode when passed ICollection of zero tasks (dotnet#55580) Consume DistributedContextPropagator in DiagnosticsHandler (dotnet#55392) Add property ordering feature (dotnet#55586) Reduce subtest count in Reflection (dotnet#55537) ...
ghost
locked as resolved and limited conversation to collaborators
H/3 and Quic are considered preview for .NET 6.0. We'll guard the usage with app context switch and potentially with
[RequiresPreviewFeatures]attributes.This PR does both to showcase it, but it's not a problem to decide to forgo one of them (probably the preview feature attribute).
Changes:
Http3DraftSupporttoHttp3AndQuicSupportto express it turns off/on S.N.Quic as wellHttpVersion.Http3and S.N.Quic public types with[RequiresPreviewFeatures]This change is also replacement for renaming S.N.Quic to private (#54720), which we decided not to bother with.
Fixes #55024