Skip to content

Replat on System.Net.Quic#18689

Merged
jkotalik merged 9 commits into
masterfrom
jkotalik/quicReplat
Feb 7, 2020
Merged

Replat on System.Net.Quic#18689
jkotalik merged 9 commits into
masterfrom
jkotalik/quicReplat

Conversation

@jkotalik
Copy link
Copy Markdown
Contributor

@jkotalik jkotalik commented Jan 30, 2020

  • 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
Comment thread NuGet.config Outdated
<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
Copy Markdown
Member

@halter73 halter73 Jan 30, 2020

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
Copy Markdown
Contributor Author

@jkotalik jkotalik Jan 31, 2020

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.

Comment thread src/Servers/Kestrel/Transport.Quic/src/Internal/QuicStreamContext.cs Outdated
Comment thread src/Servers/Kestrel/Transport.Quic/src/Internal/QuicConnectionContext.cs Outdated
Comment thread src/Servers/Kestrel/Transport.Quic/src/Internal/QuicConnectionListener.cs Outdated
Comment thread src/Servers/Kestrel/Transport.Quic/src/Internal/QuicConnectionContext.cs Outdated
Comment thread src/Servers/Kestrel/Transport.Quic/src/Internal/QuicStreamContext.cs
Comment thread src/Servers/Kestrel/Transport.Quic/src/Internal/QuicStreamContext.cs
Comment thread src/Servers/Kestrel/Transport.Quic/src/Internal/QuicStreamContext.cs Outdated
Comment thread src/Servers/Kestrel/Transport.Quic/src/QuicTransportOptions.cs
Comment thread src/Servers/Kestrel/samples/QuicSampleApp/Program.cs
@halter73
Copy link
Copy Markdown
Member

halter73 commented Jan 30, 2020

Nit: Add readonly to some more fields.

Tratcher
Tratcher reviewed Jan 31, 2020
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

jkotalik commented Feb 6, 2020

🆙 📅

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

@halter73 halter73 Feb 6, 2020

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
Comment thread src/Servers/Kestrel/Transport.Quic/src/Internal/QuicTransportContext.cs Outdated
Options = options;
}

public IHostApplicationLifetime AppLifetime { get; }
Copy link
Copy Markdown
Member

@halter73 halter73 Feb 6, 2020

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
Comment thread 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
Copy Markdown
Member

@halter73 halter73 Feb 6, 2020

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
Comment thread 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
Copy Markdown
Member

@halter73 halter73 Feb 7, 2020

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
Copy Markdown
Contributor Author

@jkotalik jkotalik Feb 7, 2020

Choose a reason for hiding this comment

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

Yep.

halter73
halter73 reviewed Feb 7, 2020
View reviewed changes
Comment thread src/Servers/Kestrel/Transport.Quic/src/Internal/QuicConnectionListener.cs Outdated
@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