[QUIC] API QuicListener #71579

ManickaP · 2022-07-02T11:39:04Z

Fixes #67560
Contributes to #68902

dotnet-issue-labeler · 2022-07-02T11:39:11Z

Note regarding the new-api-needs-documentation label:

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.

ghost · 2022-07-02T11:39:20Z

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

Issue Details

Fixes #67560
Contributes to #68902

Author:	ManickaP
Assignees:	-
Labels:	`new-api-needs-documentation`, `area-System.Net.Quic`
Milestone:	-

src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ValueTaskSource.cs

src/libraries/System.Net.Quic/src/System/Net/Quic/Interop/MsQuicSafeHandle.cs

gfoidl · 2022-07-02T16:56:13Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Interop/MsQuicSafeHandle.cs

+    // This is used for our logging that uses the same format of object identification as MsQuic to easily correlate log events.
+    private static readonly string[] TypeName = new string[]
+    {
+        " reg",


Suggested change

" reg",

"reg",

Is the space a typo?

No, it's intentional. The strings correspond to msquic logging strings which use the space to align everything in the log file.

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.PendingConnection.cs

gfoidl · 2022-07-02T17:03:09Z

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs

        {
-            if (!IsSupported)
+            ref var data = ref listenerEvent.NEW_CONNECTION;


Concrete type instead of var.

This is the only place where it's not reasonable since the type name is generated from an anonymous union and looks like this: _Anonymous_e__Union._NEW_CONNECTION_e__Struct. So I'd prefer to keep the var here.

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs

ManickaP · 2022-07-05T16:16:38Z

Failing test is #45868

ManickaP · 2022-07-05T16:17:11Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2022-07-05T16:17:25Z

Azure Pipelines successfully started running 1 pipeline(s).

ManickaP · 2022-07-05T16:17:39Z

/azp run runtime-libraries stress-http

azure-pipelines · 2022-07-05T16:17:47Z

Azure Pipelines successfully started running 1 pipeline(s).

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs

src/libraries/System.Net.Quic/src/Resources/Strings.resx

stephentoub · 2022-07-07T21:00:53Z

.../System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicAddressHelpers.cs

        {
            // MsQuic always uses storage size as if IPv6 was used
-            Span<byte> addressBytes = new Span<byte>((byte*)pInetAddress, Internals.SocketAddress.IPv6AddressSize);
+            Span<byte> addressBytes = new Span<byte>((byte*)Unsafe.AsPointer(ref quicAddress), Internals.SocketAddress.IPv6AddressSize);


This is potential corruption if quicAddress isn't fixed in memory. I assume it is, in which case it's worth a comment.

It's native memory, so it cannot be moved by GC. I'll add a comment.

stephentoub · 2022-07-07T21:01:51Z

...Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/SafeMsQuicConfigurationHandle.cs

+                settings.IsSet.IdleTimeoutMs = 1;
+                if (options.IdleTimeout != Timeout.InfiniteTimeSpan)
+                {
+                    if (options.IdleTimeout <= TimeSpan.Zero) throw new Exception("IdleTimeout must not be negative.");


This should throw something derived from Exception, and the message should be in the resx.

Addressed in #71783 apart from the message.

stephentoub · 2022-07-07T21:03:37Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs

-            _state.ListenerState = listenerState;
+            _remoteEndPoint = info->RemoteAddress->ToIPEndPoint();
+            _localEndPoint = info->LocalAddress->ToIPEndPoint();
+            _negotiatedAlpnProtocol = new SslApplicationProtocol(new Span<byte>(info->NegotiatedAlpn, info->NegotiatedAlpnLength).ToArray());


Is NegotiatedAlpn here likely one of only a few common values? If yes, you could special-case the most common ones in order to avoid the array allocation.

We might. We do this when passing ALPN from HttpClient to QuicConnection. On the QUIC level we'd have to be opinionated, but we could be biased towards H/3. At the moment, I will leave it as-is. We had it like this till now, it's not gonna move the needle significantly. We can improve it in the future.

stephentoub · 2022-07-07T21:04:17Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs

-                Exception ex = new MsQuicException(connectionEvent.SHUTDOWN_INITIATED_BY_TRANSPORT.Status, "Connection has been shutdown by transport");
-                state.ConnectTcs!.SetException(ExceptionDispatchInfo.SetCurrentStackTrace(ex));
-                state.ConnectTcs = null;
+                state.ConnectTcs.TrySetException(new MsQuicException(connectionEvent.SHUTDOWN_INITIATED_BY_TRANSPORT.Status, "Connection has been shutdown by transport"));


Here and elsewhere, .resx.

We should address this in #71432 which touches all the exceptions. I'm merely shuffling code here.

stephentoub · 2022-07-07T21:05:34Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs

+                        }
+                        else
+                        {
+                            IPAddress[] addresses = Dns.GetHostAddressesAsync(dnsHost, cancellationToken).GetAwaiter().GetResult();


We shouldn't be doing I/O synchronously like this. This is in a ConnectAsync method... why can't it be async and this be awaited?

Only code shuffle, came from #71428, see #71428 (comment). Addressed in #71783

stephentoub · 2022-07-07T21:05:48Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs

+                        else
+                        {
+                            IPAddress[] addresses = Dns.GetHostAddressesAsync(dnsHost, cancellationToken).GetAwaiter().GetResult();
+                            cancellationToken.ThrowIfCancellationRequested();


Why is this needed?

Based on @wfurt comment: #71428 (comment), on some platforms (e.g. Linux) the token is not processed by the resolution. I'm only shuffling code here.

stephentoub · 2022-07-07T21:14:07Z

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnectionOptions.cs

+    /// Idle timeout for connections, after which the connection will be closed.
+    /// Default <see cref="TimeSpan.Zero"/> means underlying implementation default idle timeout.
+    /// </summary>
+    public TimeSpan IdleTimeout { get; set; } = TimeSpan.Zero;


Zero seems a bit inconsistent for this. With SocketsHttpHandler, if you set its idle timeout to 0, that means connections won't be pooled since they'll be torn down the moment they're added to the pool.

We should discuss this in API review next Tuesday. I summarized the reasoning here: #68902 (comment), but I'm open to changes. The zero meaning here is trying to be consistent with ListenBacklog = 0 meaning use whatever is the best for the current platform.
Apart from this, 0 in msquic means infinite (for which we already have -1), and we don't have a mechanism to automatically close the connection after ... I don't even know what would be single use in this space, one outbound stream?

Moreover, there's no explicit pooling, it's a timeout to automatically tear down a connection if no data flow on it. So it's slightly different concept than SocketsHttpHandler.

stephentoub · 2022-07-07T21:15:17Z

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnectionOptions.cs

+        MaxBidirectionalStreams = 0;
+        MaxUnidirectionalStreams = 0;


These look to be zero'ing values that are already zero.

Yes, I wanted to be explicit and consistent with QuicServerConnectionOptions ctor. Showing the defaults so that you can see them and understand them in the same manner as for the server connection options.

stephentoub · 2022-07-07T21:15:34Z

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnectionOptions.cs

+    /// <summary>
+    /// Initializes a new instance of the <see cref="QuicClientConnectionOptions"/> class.
+    /// </summary>
+    public QuicClientConnectionOptions()


Our style generally has us putting ctors before properties.

Addressed in #71783.

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.PendingConnection.cs

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs

CarnaViire

Generally LGTM. I would love if @stephentoub could take a closer look at ValueTaskSource though (I also wonder why we don't have a default implementation for the interface, the implementation seems default enough, but now it's hidden inside Quic?)

src/libraries/System.Net.Quic/src/System.Net.Quic.csproj

src/libraries/System.Net.Quic/src/System/Net/Quic/Interop/MsQuicSafeHandle.cs

CarnaViire · 2022-07-08T13:43:50Z

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.PendingConnection.cs

+            _cancellationTokenSource = new CancellationTokenSource();
+        }
+
+        public async void StartHandshake(QuicConnection connection, SslClientHelloInfo clientHello, Func<QuicConnection, SslClientHelloInfo, CancellationToken, ValueTask<QuicServerConnectionOptions>> connectionOptionsCallback)


I'm a bit confused at the naming. While the name is "StartHandshake", the method actually not only starts, but completes the whole handshake as well (it awaits connection.FinishHandshakeAsync).

I understand that the method is never awaited (judging by the (only?) usage in QuicListener.cs)

// Kicks off the rest of the handshake in the background. pendingConnection.StartHandshake(connection, clientHello, _connectionOptionsCallback);

so I wonder whether this method should either be synchronous (and hide kicking off the whole operation inside), or somehow not await FinishHandshakeAsync, or be named without "Start" prefix?

The method is async void and is like that on purpose. When a new connection arrives, we want to kick off the handshake process and let it finish on the background. Then, we need a vehicle to actually get the handshake result (ie: the connected connection), for which we use _finishHandshakeTask exposed by PendingConnection.FinishHandshakeAsync and used in AcceptConnectionAsync.

In another words, we don't want to and cannot await it from NEW_CONNECTION, but we also need to react to the result of QuicConnection.FinishHandshakeAsync --> async void. Other option would be to do the same thing with Task.ContinueWith, but everything returns ValueTask here, so this seemed more straightforward.

Does this help? Do you want me to improve the comments around this? If this helped you to understand it better, I can move it into the code comments, just let me know which part makes it more clear. It's hard for me to see which explanation helps since I wrote the code 😅

I did understand that you don't ever want to await it (though I missed it's async void), what was strange to me is that method named "Start..." waits until finish 😅 Maybe adding a comment highlighting that it is called Start.. because it is async void so it is never awaited will be better 😅

P.S.: I expected to see something like this

runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs

Lines 502 to 506 in e55c908

// Queue the creation of the connection to escape the held lock

ThreadPool.QueueUserWorkItem(static state =>

{

_ = state.thisRef.AddHttp11ConnectionAsync(state.queueItem); // ignore returned task

}, (thisRef: this, queueItem), preferLocal: true);

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs

stephentoub · 2022-07-08T14:09:08Z

I also wonder why we don't have a default implementation for the interface, the implementation seems default enough

That's what ManualResetValueTaskSourceCore<T> provides, the core logic for implementing an IValueTaskSource and IValueTaskSource<T>. The most basic implementation of the interfaces can effectively just delegate to members of the struct. More complicated implementations can use the MRVTSC as part of the implementation but layer on additional functionality (as this one does, with additional logic around GCHandles and cancellation and whatnot).

CarnaViire

LGTM, thanks!

ManickaP · 2022-07-11T10:09:57Z

@JamesNK you'll need to react to this, especially around the QuicListenerOptions where we added the callback.

ManickaP added 3 commits July 2, 2022 09:41

QuicListener new API shape including compilable implementation

31625e9

Quic and Http tests compile

bdce792

Tests work

29f6fc7

dotnet-issue-labeler bot added area-System.Net.Quic new-api-needs-documentation labels Jul 2, 2022

ghost assigned ManickaP Jul 2, 2022

gfoidl reviewed Jul 2, 2022

View reviewed changes

Feedback

607df72

ManickaP force-pushed the mapichov/quic-listener branch 2 times, most recently from 97ad0ce to 1329933 Compare July 4, 2022 10:13

Fix Windows error

035a47a

ManickaP force-pushed the mapichov/quic-listener branch from 1329933 to 035a47a Compare July 5, 2022 09:18

ManickaP marked this pull request as ready for review July 5, 2022 16:17