QuicListener.AcceptConnectionAsync usability

[pepone]

When you call QuicListener.AcceptConnectionAsync this call cannot complete until the peer certificate validation callback completes, a server will typically have one or a few threads accepting connections, but this is a problem with Quic API as if some clients are slow performing the TLS handshake this can affect the rate at which the server is capable of accepting new connections.

I think it would be more flexible if the TLS handshake was performed from a separate call on the accepted connection.

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

Issue Details

---

When you call QuicListener.AcceptConnectionAsync this call cannot complete until the peer certificate validation callback completes, a server will typically have one or a few threads accepting connections, but this is a problem with Quic API as if some clients are slow performing the TLS handshake this can affect the rate at which the server is capable of accepting new connections.

I think it would be more flexible if the TLS handshake was performed from a separate call on the accepted connection.

Author:	pepone
Assignees:	-
Labels:	area-System.Net.Http
Milestone:	-

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

Issue Details

---

When you call QuicListener.AcceptConnectionAsync this call cannot complete until the peer certificate validation callback completes, a server will typically have one or a few threads accepting connections, but this is a problem with Quic API as if some clients are slow performing the TLS handshake this can affect the rate at which the server is capable of accepting new connections.

I think it would be more flexible if the TLS handshake was performed from a separate call on the accepted connection.

Author:	pepone
Assignees:	-
Labels:	untriaged, area-System.Net.Quic
Milestone:	-

[ManickaP]

It's true that we queue the connections (in the accept queue) in order as they come and we won't hand out a connection unless the handshake was finished. But we kick off the handshake on the background, it's not happening within AcceptConnectionAsync, and it's not serialized to a single thread for all connections. I.e. we can have multiple handshakes happening at the same time and still be receiving new connections. But we will not hand any of them out until the connection is fully established (including TLS).

Have you observed any performance issues with AcceptConnectionAsync? Or is this just a hunch of potential problems based on the API shape?

[pepone]

Have you observed any performance issues with AcceptConnectionAsync? Or is this just a hunch of potential problems based on the API shape?

I haven't observed any performance issues, but this API makes it difficult for the server to accept new connections in the face of clients performing a slow TLS handshake.

With SSL over TCP, you can typically have a single thread or a few calling accept, and then queue the TlsHandshake call to be performed in the thread pool, so that slow clients don't have that much impact on the ability to accept other queued connections.

[ManickaP]

TLS is part of QUIC, so we cannot hand out connections that are not fully established, including TLS handshake (we had an issue related to this #57246). Note that we do handshakes in parallel already.
What we could do to improve this is to queue only established connections, keeping the ones with an ongoing TLS handshake in a separate collection. Users of the API would get the connections as fast as possible, the ones with fast handshake would be handed out sooner than the ones with slow handshakes. Disadvantages are that we'd need to keep 2 collections and enforce connection backlog size ourselves, for which we now conveniently use bounded channel. We had something similar before 7.0, but we didn't properly enforce backlog size with that implementation.

Triage: I don't think this is critical for 8.0 atm. We don't have any concrete data that would support that this is an actual problem. However, I'm not against this proposition, and we might consider this. We can either change the priority based on perf results or keep it for the future.

[pepone]

To me, the main issue is that is easy for a malicious client to exploit the current API, such a client can start the connection and never terminate the handshake, and there isn't much the caller of AcceptConnectionAsync can do, you can use a cancellation token with a timeout for the call but still you cannot set this too low or you will get false positives.

[ManickaP]

We have a 10s timeout for handshakes built-in.