[QUIC] API QuicListener by ManickaP · Pull Request #71579 · dotnet/runtime

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

Fixes#67560
Contributes to #68902

ghost · 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:	-

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

+ // 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.

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

+ if (listenerEvent.Type == QUIC_LISTENER_EVENT_TYPE.NEW_CONNECTION)
 {
- 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.

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).

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

 {
 // 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

+ 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

- _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

- 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

+ }
+ 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

+ 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

+ /// 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

+ 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

+ /// <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.

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?)

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

+ _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);

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 · 2022-07-02T11:39:04Z

Fixes#67560
Contributes to #68902

ghost · 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:	-

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

+ // 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.

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

+ if (listenerEvent.Type == QUIC_LISTENER_EVENT_TYPE.NEW_CONNECTION)
 {
- 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.

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).

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

 {
 // 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

+ 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

- _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

- 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

+ }
+ 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

+ 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

+ /// 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

+ 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

+ /// <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.

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?)

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

+ _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);

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 · 2022-07-02T11:39:04Z

Fixes#67560
Contributes to #68902

ghost · 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:	-

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

+ // 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.

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

+ if (listenerEvent.Type == QUIC_LISTENER_EVENT_TYPE.NEW_CONNECTION)
 {
- 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.

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).

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

 {
 // 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

+ 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

- _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

- 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

+ }
+ 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

+ 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

+ /// 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

+ 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

+ /// <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.

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?)

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

+ _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);

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 · 2022-07-02T11:39:04Z

Fixes#67560
Contributes to #68902

ghost · 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:	-

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

+ // 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.

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

+ if (listenerEvent.Type == QUIC_LISTENER_EVENT_TYPE.NEW_CONNECTION)
 {
- 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.

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).

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

 {
 // 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

+ 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

- _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

- 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

+ }
+ 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

+ 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

+ /// 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

+ 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

+ /// <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.

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?)

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

+ _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);

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 · 2022-07-02T11:39:04Z

Fixes#67560
Contributes to #68902

ghost · 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:	-

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

+ // 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.

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

+ if (listenerEvent.Type == QUIC_LISTENER_EVENT_TYPE.NEW_CONNECTION)
 {
- 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.

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).

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

 {
 // 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

+ 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

- _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

- 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

+ }
+ 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

+ 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

+ /// 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

+ 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

+ /// <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.

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?)

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

+ _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);

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 · 2022-07-02T11:39:04Z

Fixes#67560
Contributes to #68902

ghost · 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:	-

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

+ // 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.

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

+ if (listenerEvent.Type == QUIC_LISTENER_EVENT_TYPE.NEW_CONNECTION)
 {
- 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.

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).

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

 {
 // 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

+ 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

- _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

- 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

+ }
+ 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

+ 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

+ /// 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

+ 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

+ /// <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.

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?)

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

+ _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);

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 · 2022-07-02T11:39:04Z

Fixes#67560
Contributes to #68902

ghost · 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:	-

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

+ // 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.

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

+ if (listenerEvent.Type == QUIC_LISTENER_EVENT_TYPE.NEW_CONNECTION)
 {
- 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.

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).

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

 {
 // 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

+ 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

- _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

- 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

+ }
+ 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

+ 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

+ /// 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

+ 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

+ /// <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.

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?)

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

+ _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);

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 · 2022-07-02T11:39:04Z

Fixes#67560
Contributes to #68902

ghost · 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:	-

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

+ // 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.

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

+ if (listenerEvent.Type == QUIC_LISTENER_EVENT_TYPE.NEW_CONNECTION)
 {
- 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.

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).

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

 {
 // 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

+ 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

- _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

- 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

+ }
+ 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

+ 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

+ /// 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

+ 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

+ /// <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.

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?)

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

+ _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);

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

ghost 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 1329933Compare July 4, 2022 10:13

Fix Windows error
035a47a

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

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

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs Outdated

ManickaP mentioned this pull request Jul 7, 2022
[QUIC] API QuicConnection #71783
Merged

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs Outdated

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Quic/src/Resources/Strings.resx Outdated

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.PendingConnection.cs Outdated

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs Outdated

Merge branch 'main' into mapichov/quic-listener
ba996a0

ManickaP force-pushed the mapichov/quic-listener branch from 6fbd597 to 762ec71Compare July 8, 2022 10:04

Feedback
2d88936

ManickaP force-pushed the mapichov/quic-listener branch 2 times, most recently from 5e558d3 to 25276edCompare July 8, 2022 13:16

Listener comment; PreviewFeature attribute
4e5b919

ManickaP force-pushed the mapichov/quic-listener branch from 25276ed to 4e5b919Compare July 8, 2022 13:25

Updated helix image with msquic
d9fe39e

CarnaViire reviewed Jul 8, 2022
View reviewed changes

ManickaP added 3 commits July 8, 2022 16:28

Cleaned up PreviewFeature attribute.
e724aea

Feedback
ac2f09a

Split event handlers into methods.
61ce63f

ManickaP force-pushed the mapichov/quic-listener branch from 453f6aa to 61ce63fCompare July 8, 2022 17:46

ManickaP added 2 commits July 8, 2022 20:31

Added comments for pending connection.
21f6bcf

Switch expression for HandleEvent
74936c5

CarnaViire approved these changes Jul 10, 2022
View reviewed changes

ManickaP merged commit 60af631 into dotnet:mainJul 10, 2022

ManickaP deleted the mapichov/quic-listener branch July 10, 2022 20:13

This was referenced Jul 12, 2022
System.Net.Quic.Functional.Tests crash on coreclr/windows/x64 #71995
Closed
QUIC: Should not need finalization on MsQuicStream/Connection/Listener #55103
Closed

karelz added this to the 7.0.0 milestone Jul 19, 2022

ghost locked as resolved and limited conversation to collaborators Aug 18, 2022

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

ghost 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 1329933Compare July 4, 2022 10:13

Fix Windows error
035a47a

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

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

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs Outdated

ManickaP mentioned this pull request Jul 7, 2022
[QUIC] API QuicConnection #71783
Merged

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs Outdated

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Quic/src/Resources/Strings.resx Outdated

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.PendingConnection.cs Outdated

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs Outdated

Merge branch 'main' into mapichov/quic-listener
ba996a0

ManickaP force-pushed the mapichov/quic-listener branch from 6fbd597 to 762ec71Compare July 8, 2022 10:04

Feedback
2d88936

ManickaP force-pushed the mapichov/quic-listener branch 2 times, most recently from 5e558d3 to 25276edCompare July 8, 2022 13:16

Listener comment; PreviewFeature attribute
4e5b919

ManickaP force-pushed the mapichov/quic-listener branch from 25276ed to 4e5b919Compare July 8, 2022 13:25

Updated helix image with msquic
d9fe39e

CarnaViire reviewed Jul 8, 2022
View reviewed changes

ManickaP added 3 commits July 8, 2022 16:28

Cleaned up PreviewFeature attribute.
e724aea

Feedback
ac2f09a

Split event handlers into methods.
61ce63f

ManickaP force-pushed the mapichov/quic-listener branch from 453f6aa to 61ce63fCompare July 8, 2022 17:46

ManickaP added 2 commits July 8, 2022 20:31

Added comments for pending connection.
21f6bcf

Switch expression for HandleEvent
74936c5

CarnaViire approved these changes Jul 10, 2022
View reviewed changes

ManickaP merged commit 60af631 into dotnet:mainJul 10, 2022

ManickaP deleted the mapichov/quic-listener branch July 10, 2022 20:13

This was referenced Jul 12, 2022
System.Net.Quic.Functional.Tests crash on coreclr/windows/x64 #71995
Closed
QUIC: Should not need finalization on MsQuicStream/Connection/Listener #55103
Closed

karelz added this to the 7.0.0 milestone Jul 19, 2022

ghost locked as resolved and limited conversation to collaborators Aug 18, 2022

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

ghost 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 1329933Compare July 4, 2022 10:13

Fix Windows error
035a47a

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

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

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs Outdated

ManickaP mentioned this pull request Jul 7, 2022
[QUIC] API QuicConnection #71783
Merged

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs Outdated

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Quic/src/Resources/Strings.resx Outdated

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.PendingConnection.cs Outdated

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs Outdated

Merge branch 'main' into mapichov/quic-listener
ba996a0

ManickaP force-pushed the mapichov/quic-listener branch from 6fbd597 to 762ec71Compare July 8, 2022 10:04

Feedback
2d88936

ManickaP force-pushed the mapichov/quic-listener branch 2 times, most recently from 5e558d3 to 25276edCompare July 8, 2022 13:16

Listener comment; PreviewFeature attribute
4e5b919

ManickaP force-pushed the mapichov/quic-listener branch from 25276ed to 4e5b919Compare July 8, 2022 13:25

Updated helix image with msquic
d9fe39e

CarnaViire reviewed Jul 8, 2022
View reviewed changes

ManickaP added 3 commits July 8, 2022 16:28

Cleaned up PreviewFeature attribute.
e724aea

Feedback
ac2f09a

Split event handlers into methods.
61ce63f

ManickaP force-pushed the mapichov/quic-listener branch from 453f6aa to 61ce63fCompare July 8, 2022 17:46

ManickaP added 2 commits July 8, 2022 20:31

Added comments for pending connection.
21f6bcf

Switch expression for HandleEvent
74936c5

CarnaViire approved these changes Jul 10, 2022
View reviewed changes

ManickaP merged commit 60af631 into dotnet:mainJul 10, 2022

ManickaP deleted the mapichov/quic-listener branch July 10, 2022 20:13

This was referenced Jul 12, 2022
System.Net.Quic.Functional.Tests crash on coreclr/windows/x64 #71995
Closed
QUIC: Should not need finalization on MsQuicStream/Connection/Listener #55103
Closed

karelz added this to the 7.0.0 milestone Jul 19, 2022

ghost locked as resolved and limited conversation to collaborators Aug 18, 2022

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

ghost 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 1329933Compare July 4, 2022 10:13

Fix Windows error
035a47a

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

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

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs Outdated

ManickaP mentioned this pull request Jul 7, 2022
[QUIC] API QuicConnection #71783
Merged

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs Outdated

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Quic/src/Resources/Strings.resx Outdated

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.PendingConnection.cs Outdated

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs Outdated

Merge branch 'main' into mapichov/quic-listener
ba996a0

ManickaP force-pushed the mapichov/quic-listener branch from 6fbd597 to 762ec71Compare July 8, 2022 10:04

Feedback
2d88936

ManickaP force-pushed the mapichov/quic-listener branch 2 times, most recently from 5e558d3 to 25276edCompare July 8, 2022 13:16

Listener comment; PreviewFeature attribute
4e5b919

ManickaP force-pushed the mapichov/quic-listener branch from 25276ed to 4e5b919Compare July 8, 2022 13:25

Updated helix image with msquic
d9fe39e

CarnaViire reviewed Jul 8, 2022
View reviewed changes

ManickaP added 3 commits July 8, 2022 16:28

Cleaned up PreviewFeature attribute.
e724aea

Feedback
ac2f09a

Split event handlers into methods.
61ce63f

ManickaP force-pushed the mapichov/quic-listener branch from 453f6aa to 61ce63fCompare July 8, 2022 17:46

ManickaP added 2 commits July 8, 2022 20:31

Added comments for pending connection.
21f6bcf

Switch expression for HandleEvent
74936c5

CarnaViire approved these changes Jul 10, 2022
View reviewed changes

ManickaP merged commit 60af631 into dotnet:mainJul 10, 2022

ManickaP deleted the mapichov/quic-listener branch July 10, 2022 20:13

This was referenced Jul 12, 2022
System.Net.Quic.Functional.Tests crash on coreclr/windows/x64 #71995
Closed
QUIC: Should not need finalization on MsQuicStream/Connection/Listener #55103
Closed

karelz added this to the 7.0.0 milestone Jul 19, 2022

ghost locked as resolved and limited conversation to collaborators Aug 18, 2022

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

ghost 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 1329933Compare July 4, 2022 10:13

Fix Windows error
035a47a

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

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

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs Outdated

ManickaP mentioned this pull request Jul 7, 2022
[QUIC] API QuicConnection #71783
Merged

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs Outdated

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Quic/src/Resources/Strings.resx Outdated

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.PendingConnection.cs Outdated

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs Outdated

Merge branch 'main' into mapichov/quic-listener
ba996a0

ManickaP force-pushed the mapichov/quic-listener branch from 6fbd597 to 762ec71Compare July 8, 2022 10:04

Feedback
2d88936

ManickaP force-pushed the mapichov/quic-listener branch 2 times, most recently from 5e558d3 to 25276edCompare July 8, 2022 13:16

Listener comment; PreviewFeature attribute
4e5b919

ManickaP force-pushed the mapichov/quic-listener branch from 25276ed to 4e5b919Compare July 8, 2022 13:25

Updated helix image with msquic
d9fe39e

CarnaViire reviewed Jul 8, 2022
View reviewed changes

ManickaP added 3 commits July 8, 2022 16:28

Cleaned up PreviewFeature attribute.
e724aea

Feedback
ac2f09a

Split event handlers into methods.
61ce63f

ManickaP force-pushed the mapichov/quic-listener branch from 453f6aa to 61ce63fCompare July 8, 2022 17:46

ManickaP added 2 commits July 8, 2022 20:31

Added comments for pending connection.
21f6bcf

Switch expression for HandleEvent
74936c5

CarnaViire approved these changes Jul 10, 2022
View reviewed changes

ManickaP merged commit 60af631 into dotnet:mainJul 10, 2022

ManickaP deleted the mapichov/quic-listener branch July 10, 2022 20:13

This was referenced Jul 12, 2022
System.Net.Quic.Functional.Tests crash on coreclr/windows/x64 #71995
Closed
QUIC: Should not need finalization on MsQuicStream/Connection/Listener #55103
Closed

karelz added this to the 7.0.0 milestone Jul 19, 2022

ghost locked as resolved and limited conversation to collaborators Aug 18, 2022

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

ghost 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 1329933Compare July 4, 2022 10:13

Fix Windows error
035a47a

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

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

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs Outdated

ManickaP mentioned this pull request Jul 7, 2022
[QUIC] API QuicConnection #71783
Merged

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs Outdated

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Quic/src/Resources/Strings.resx Outdated

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.PendingConnection.cs Outdated

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs Outdated

Merge branch 'main' into mapichov/quic-listener
ba996a0

ManickaP force-pushed the mapichov/quic-listener branch from 6fbd597 to 762ec71Compare July 8, 2022 10:04

Feedback
2d88936

ManickaP force-pushed the mapichov/quic-listener branch 2 times, most recently from 5e558d3 to 25276edCompare July 8, 2022 13:16

Listener comment; PreviewFeature attribute
4e5b919

ManickaP force-pushed the mapichov/quic-listener branch from 25276ed to 4e5b919Compare July 8, 2022 13:25

Updated helix image with msquic
d9fe39e

CarnaViire reviewed Jul 8, 2022
View reviewed changes

ManickaP added 3 commits July 8, 2022 16:28

Cleaned up PreviewFeature attribute.
e724aea

Feedback
ac2f09a

Split event handlers into methods.
61ce63f

ManickaP force-pushed the mapichov/quic-listener branch from 453f6aa to 61ce63fCompare July 8, 2022 17:46

ManickaP added 2 commits July 8, 2022 20:31

Added comments for pending connection.
21f6bcf

Switch expression for HandleEvent
74936c5

CarnaViire approved these changes Jul 10, 2022
View reviewed changes

ManickaP merged commit 60af631 into dotnet:mainJul 10, 2022

ManickaP deleted the mapichov/quic-listener branch July 10, 2022 20:13

This was referenced Jul 12, 2022
System.Net.Quic.Functional.Tests crash on coreclr/windows/x64 #71995
Closed
QUIC: Should not need finalization on MsQuicStream/Connection/Listener #55103
Closed

karelz added this to the 7.0.0 milestone Jul 19, 2022

ghost locked as resolved and limited conversation to collaborators Aug 18, 2022

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

ghost 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 1329933Compare July 4, 2022 10:13

Fix Windows error
035a47a

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

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

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs Outdated

ManickaP mentioned this pull request Jul 7, 2022
[QUIC] API QuicConnection #71783
Merged

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs Outdated

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Quic/src/Resources/Strings.resx Outdated

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.PendingConnection.cs Outdated

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs Outdated

Merge branch 'main' into mapichov/quic-listener
ba996a0

ManickaP force-pushed the mapichov/quic-listener branch from 6fbd597 to 762ec71Compare July 8, 2022 10:04

Feedback
2d88936

ManickaP force-pushed the mapichov/quic-listener branch 2 times, most recently from 5e558d3 to 25276edCompare July 8, 2022 13:16

Listener comment; PreviewFeature attribute
4e5b919

ManickaP force-pushed the mapichov/quic-listener branch from 25276ed to 4e5b919Compare July 8, 2022 13:25

Updated helix image with msquic
d9fe39e

CarnaViire reviewed Jul 8, 2022
View reviewed changes

ManickaP added 3 commits July 8, 2022 16:28

Cleaned up PreviewFeature attribute.
e724aea

Feedback
ac2f09a

Split event handlers into methods.
61ce63f

ManickaP force-pushed the mapichov/quic-listener branch from 453f6aa to 61ce63fCompare July 8, 2022 17:46

ManickaP added 2 commits July 8, 2022 20:31

Added comments for pending connection.
21f6bcf

Switch expression for HandleEvent
74936c5

CarnaViire approved these changes Jul 10, 2022
View reviewed changes

ManickaP merged commit 60af631 into dotnet:mainJul 10, 2022

ManickaP deleted the mapichov/quic-listener branch July 10, 2022 20:13

This was referenced Jul 12, 2022
System.Net.Quic.Functional.Tests crash on coreclr/windows/x64 #71995
Closed
QUIC: Should not need finalization on MsQuicStream/Connection/Listener #55103
Closed

karelz added this to the 7.0.0 milestone Jul 19, 2022

ghost locked as resolved and limited conversation to collaborators Aug 18, 2022

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

ghost 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 1329933Compare July 4, 2022 10:13

Fix Windows error
035a47a

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

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

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs Outdated

ManickaP mentioned this pull request Jul 7, 2022
[QUIC] API QuicConnection #71783
Merged

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs Outdated

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Quic/src/Resources/Strings.resx Outdated

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.PendingConnection.cs Outdated

stephentoub reviewed Jul 7, 2022
View reviewed changes

Comment threadsrc/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs Outdated

Merge branch 'main' into mapichov/quic-listener
ba996a0

ManickaP force-pushed the mapichov/quic-listener branch from 6fbd597 to 762ec71Compare July 8, 2022 10:04

Feedback
2d88936

ManickaP force-pushed the mapichov/quic-listener branch 2 times, most recently from 5e558d3 to 25276edCompare July 8, 2022 13:16

Listener comment; PreviewFeature attribute
4e5b919

ManickaP force-pushed the mapichov/quic-listener branch from 25276ed to 4e5b919Compare July 8, 2022 13:25

Updated helix image with msquic
d9fe39e

CarnaViire reviewed Jul 8, 2022
View reviewed changes

ManickaP added 3 commits July 8, 2022 16:28

Cleaned up PreviewFeature attribute.
e724aea

Feedback
ac2f09a

Split event handlers into methods.
61ce63f

ManickaP force-pushed the mapichov/quic-listener branch from 453f6aa to 61ce63fCompare July 8, 2022 17:46

ManickaP added 2 commits July 8, 2022 20:31

Added comments for pending connection.
21f6bcf

Switch expression for HandleEvent
74936c5

CarnaViire approved these changes Jul 10, 2022
View reviewed changes

ManickaP merged commit 60af631 into dotnet:mainJul 10, 2022

ManickaP deleted the mapichov/quic-listener branch July 10, 2022 20:13

This was referenced Jul 12, 2022
System.Net.Quic.Functional.Tests crash on coreclr/windows/x64 #71995
Closed
QUIC: Should not need finalization on MsQuicStream/Connection/Listener #55103
Closed

karelz added this to the 7.0.0 milestone Jul 19, 2022

ghost locked as resolved and limited conversation to collaborators Aug 18, 2022

	// 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);

Uh oh!

Conversation

ManickaP commented Jul 2, 2022

Uh oh!

ghost commented Jul 2, 2022

Uh oh!

ghost commented Jul 2, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ManickaPJul 2, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ManickaP commented Jul 5, 2022

Uh oh!

ManickaP commented Jul 5, 2022

Uh oh!

azure-pipelinesBot commented Jul 5, 2022

Uh oh!

ManickaP commented Jul 5, 2022

Uh oh!

azure-pipelinesBot commented Jul 5, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ManickaPJul 8, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ManickaPJul 8, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ManickaPJul 8, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ManickaPJul 2, 2022•
edited
Loading

ManickaPJul 8, 2022•
edited
Loading

ManickaPJul 8, 2022•
edited
Loading

ManickaPJul 8, 2022•
edited
Loading