Skip to content

Replat on System.Net.Quic #18689

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Feb 7, 2020
Merged

Replat on System.Net.Quic #18689

merged 9 commits into from
Feb 7, 2020

Conversation

jkotalik
Copy link
Contributor

  • Rename project from MsQuic to Quic.
  • Delete all code that was part of the original MsQuic prototype
  • Add package reference to make QuicSampleApp and QuicClient work together with a stub msquic

Still to do:

  • I initially thought that I could use the same tests as Libuv and Sockets, however there are some differences between connections and streams, the test client side using TCP rather than the transport as well, and various other things. I think I need to rewrite most of the tests anyways for now. I'm going to start working on that.
  • Do a bit more fine tuning on the ConnectionContext/StreamContext. I think we need another API on the connection which does an abort with an error code (there is only close now).

This work is required to continue working on HTTP/3

@analogrelay analogrelay added this to the 5.0.0-preview1 milestone Jan 30, 2020
halter73
halter73 requested changes Jan 30, 2020
View reviewed changes
NuGet.config Outdated
@@ -14,5 +14,6 @@
<add key="aspnetcore-dev" value="https://dotnet.myget.org/F/aspnetcore-dev/api/v3/index.json" />
<add key="aspnetcore-tools" value="https://dotnet.myget.org/F/aspnetcore-tools/api/v3/index.json" />
<add key="roslyn-tools" value="https://dotnet.myget.org/F/roslyn-tools/api/v3/index.json" />
<add key="general-testing" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/general-testing/nuget/v3/index.json" />
Copy link
Member

Choose a reason for hiding this comment

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

What are we getting from this feed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The MsQuicPackage. I can make a separate nuget config inside of the kestrel folder, but it's used in runtime.

@halter73
Copy link
Member

Nit: Add readonly to some more fields.

Tratcher
Tratcher reviewed Jan 31, 2020
View reviewed changes
Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

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

The Transport.Quic/README.md is outdated.

@jkotalik
Copy link
Contributor Author

jkotalik commented Feb 6, 2020

🆙 📅

halter73
halter73 reviewed Feb 6, 2020
View reviewed changes
src/Servers/Kestrel/Transport.Quic/src/Internal/QuicConnectionContext.cs Outdated
{
if (_closeTask != default)
{
await _connection.CloseAsync(errorCode: 0);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: It's probably worth storing this in _closeTask to protect against multiple calls to DisposeAsync.

halter73
halter73 reviewed Feb 6, 2020
View reviewed changes
src/Servers/Kestrel/Transport.Quic/src/Internal/QuicTransportContext.cs Outdated
{
public MsQuicTransportContext(IHostApplicationLifetime appLifetime, IMsQuicTrace log, MsQuicTransportOptions options)
public QuicTransportContext(IHostApplicationLifetime appLifetime, IQuicTrace log, QuicTransportOptions options)
{
AppLifetime = appLifetime;
Log = log;
Options = options;
}

public IHostApplicationLifetime AppLifetime { get; }
Copy link
Member

Choose a reason for hiding this comment

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

Is the IHostApplicationLifetime still being used anywhere? It seems like a pretty weird requirement for stuff like QuicConnectionFactory.

halter73
halter73 reviewed Feb 6, 2020
View reviewed changes
src/Servers/Kestrel/Transport.Quic/src/QuicConnectionFactory.cs
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;

namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Quic
Copy link
Member

Choose a reason for hiding this comment

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

I think we should either remove ".Client" from Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets.Client.SocketConnectionFactory or add it here. Fortunately SocketConnectionFactory hasn't shipped yet, so we have a choice.

halter73
halter73 reviewed Feb 7, 2020
View reviewed changes
src/Servers/Kestrel/Transport.Quic/src/Internal/QuicStreamContext.cs

public override void Abort(ConnectionAbortedException abortReason)
{
// Don't call _stream.Shutdown and _stream.Abort at the same time.
Copy link
Member

Choose a reason for hiding this comment

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

Is it OK to call _stream.Abort after _stream.Shutdown?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep.

@jkotalik jkotalik merged commit 1daebd1 into master Feb 7, 2020
@jkotalik jkotalik deleted the jkotalik/quicReplat branch February 7, 2020 18:43
@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
Reviewers

@Tratcher Tratcher Tratcher left review comments

@halter73 halter73 halter73 approved these changes

@analogrelay analogrelay Awaiting requested review from analogrelay

Assignees
No one assigned
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Milestone
5.0.0-preview1
Development

Successfully merging this pull request may close these issues.
6 participants
@jkotalik @halter73 @Tratcher @analogrelay @Pilchie @amcasey