Skip to content

Conversation

@JamesNK
Copy link
Member

@JamesNK JamesNK commented Mar 2, 2021

Quic transport is no longer experimental in .NET 6

This PR is a YOLO find and replace to remove Experimental from the Quic transport project and namespaces. I'd apprietate guidance on any ritutuals that are required when removing and adding packages from aspnetcore.

Two things:

  1. The runtime package we depend on still has Experimental in the name: System.Net.Experimental.MsQuic.
  2. I think we should consider updating the project name and namespace to Microsoft.AspNetCore.Server.Kestrel.Transport.MsQuic (note: Quic -> MsQuic). That's consistent with runtime's name. And in the future the Quic implementation might change to be written in managed code so we should be more specific.

PR

  • Renames and removes Experimental from Quic transport project
  • Removes Experimental from multiplex connection abstractions types

APIs promoted from experimental status need API review.

@JamesNK JamesNK requested review from a team, Tratcher, halter73 and jkotalik as code owners March 2, 2021 02:52
@JamesNK JamesNK added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Mar 2, 2021
@ghost
Copy link

ghost commented Mar 2, 2021

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@JamesNK
Copy link
Member Author

JamesNK commented Mar 8, 2021

Review notes:

IMultiplexedConnectionListenerFactory:

  • BindAsync IFeatureCollection used?
  • Indented for per endpoint cert?

IMultiplexedConnectionListener:

  • AcceptAsync IFeatureCollection used?

MultiplexedConnectionContext:

  • ValueTask<ConnectionContext> ConnectAsync, change return to nullable
  • Debate about ConnectAsync name. Options:
    • OpenAsync
    • CreateAsync

QuicTransportFactory:

  • Make internal. Likely was made public to match sockets transport

WebHostBuilderMsQuicExtensions:

  • Rename to WebHostBuilderQuicExtensions

QuicTransportOptions:

  • Revist everything

Will remove experimental now and make changes through previews. Re-review later in .NET 6.

@jkotalik @Tratcher @halter73 @davidfowl @wtgodbe

@halter73
Copy link
Member

halter73 commented Mar 9, 2021

The IFeatureCollection arguments are intended for future extensibility. Since TLS becomes a transport concern with QUIC using it for certs sounds like a viable option.

Why does ConnectAsync need to return a nullable?

@JamesNK
Copy link
Member Author

JamesNK commented Mar 9, 2021

Why does ConnectAsync need to return a nullable?

@jkotalik added that feedback. Is it to make ConnectAsync return consistent with AcceptAsync?

@JamesNK
Copy link
Member Author

JamesNK commented Mar 9, 2021

@dougbu The CI build is failing with this message:

eng\Baseline.Designer.props(,): error : Generated code is not up to date in eng/Baseline.Designer.props. You might need to regenerate the reference assemblies or project list (see docs/ReferenceAssemblies.md and docs/ReferenceResolution.md)

What changes do I need to make to pass CI build?

@JamesNK JamesNK added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Mar 9, 2021
@dougbu
Copy link
Contributor

dougbu commented Mar 9, 2021

What changes do I need to make to pass CI build?

It's mostly not you @JamesNK (and this likely applies to every PR that targets 'main' and not-yet-verified):

  1. I need to get [release/5.0] Merge, update baselines, SDK and runtimes #30781 in
  2. Bot needs to create a merge PR into 'main'
  3. I need to get that PR in
  4. You need to rebase on latest 'main'

@JamesNK
Copy link
Member Author

JamesNK commented Mar 10, 2021

Ok! No urgency to get this PR in.

@jkotalik
Copy link
Contributor

@jkotalik added that feedback. Is it to make ConnectAsync return consistent with AcceptAsync?

Yeah that was the idea behind it. Though maybe throwing for either if they fail to connect or accept makes more sense?

@JamesNK JamesNK force-pushed the jamesnk/quic-experimental branch from 5a510b8 to 13619ee Compare April 18, 2021 21:02
@JamesNK
Copy link
Member Author

JamesNK commented Apr 19, 2021

@dougbu @JunTaoLuo @wtgodbe Good to merge from a build POV?

Copy link
Member

@wtgodbe wtgodbe left a comment

Choose a reason for hiding this comment

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

LGTM build-wise

@JamesNK JamesNK added this to the 6.0-preview5 milestone Apr 19, 2021
@JamesNK JamesNK merged commit ea2f559 into main Apr 19, 2021
@JamesNK JamesNK deleted the jamesnk/quic-experimental branch April 19, 2021 20:07
@dougbu
Copy link
Contributor

dougbu commented Apr 19, 2021

Side question: Why do the Baseline files appear in the final commit❔ They should have been updated as part of a merge from release/5.0.

@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api-approved API was approved in API review, it can be implemented area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants