Asynchronous opening of QuicStreams

[rzikm]

This PR removes Open(Uni|Bi)directionalStream methods on QuicConnection and replaces them with asynchronous variants. If no stream is available, the methods block until the peer's stream limit is increased.

Most of the changes outside MsQuicStream and MsQuicConnection are just updating usages.

Fixes #67302.

[dotnet-issue-labeler]

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]

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

Issue Details

---

This PR removes Open(Uni|Bi)directionalStream methods on QuicConnection and replaces them with asynchronous variants. If no stream is available, the methods block until the peer's stream limit is increased.

Fixes #67302.

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

[rzikm]

/azp run runtime-libraries-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ManickaP]

Apart from the comments, have you also thought about how we could solve the problem with multiple H/3 connections and opening streams on the first connection that has streams available? Without relying on WaitForStreamsAvailable and GetAvailableStreamCount since they both are rather expensive operations.

I'm not saying it needs to be solved in this issue, we can postpone it to API discussion for stream.

[rzikm]

have you also thought about how we could solve the problem with multiple H/3 connections and opening streams on the first connection that has streams available?

Isn't this against the RFC?

Clients SHOULD NOT open more than one HTTP/3 connection to a given IP
address and UDP port, where the IP address and port might be derived
from a URI, a selected alternative service ([ALTSVC]), a configured
proxy, or name resolution of any of these. A client MAY open
multiple HTTP/3 connections to the same IP address and UDP port using
different transport or TLS configurations but SHOULD avoid creating
multiple connections with the same configuration.

Anyway, I thought about it and it seems to me that achieving this would be very complex. Part of the problem could be covere by having a method such as TryOpen(Uni|BI]directionalStreamAsync which would return null if no streams are available, then we could just loop over a set of connections. This should be relatively straightforward to accomplish, as we can just use FAIL_BLOCKED flag when opening a stream.

The part where no connection has available streams and we need to wait untile one does is the problematic part. The core part of a solution that occured to me was to use the STREAMS_AVAILABLE event to complete a completion source provided by the pool with just-created stream instance (the stream would be opened and started inline still on the callback thread to avoid cata races). But the plumbing required to achieve this is too complex IMO and would require some private API to be used by http connection pool.

alternative would be handling all this externally using a SemaphoreSlim in the http connection pool, but that solution would have problems when a connection is closed unexpectedly...

[ManickaP]

have you also thought about how we could solve the problem with multiple H/3 connections and opening streams on the first connection that has streams available?

Isn't this against the RFC?

Clients SHOULD NOT open more than one HTTP/3 connection to a given IP
address and UDP port, where the IP address and port might be derived
from a URI, a selected alternative service ([ALTSVC]), a configured
proxy, or name resolution of any of these. A client MAY open
multiple HTTP/3 connections to the same IP address and UDP port using
different transport or TLS configurations but SHOULD avoid creating
multiple connections with the same configuration.

Yes it is, the same way HTTP/2 multiple connections are. We default to a single connection and it can be turned on via SocketsHandler property, we wouldn't have it on by default. Scenarios like gRPC and server - 2 - server take advantage of this feature for performance reasons.

Anyway, I thought about it and it seems to me that achieving this would be very complex. Part of the problem could be covere by having a method such as TryOpen(Uni|BI]directionalStreamAsync which would return null if no streams are available, then we could just loop over a set of connections. This should be relatively straightforward to accomplish, as we can just use FAIL_BLOCKED flag when opening a stream.

The part where no connection has available streams and we need to wait untile one does is the problematic part. The core part of a solution that occured to me was to use the STREAMS_AVAILABLE event to complete a completion source provided by the pool with just-created stream instance (the stream would be opened and started inline still on the callback thread to avoid cata races). But the plumbing required to achieve this is too complex IMO and would require some private API to be used by http connection pool.

That's what we had before, more or less, and it was very perf intensive. So we'd like to take advantage of msquic being able to do the stream counting/waiting for us, since that seems much more performant than what we attempted to do. Anyway, this is not a hard requirement for this PR.

[ManickaP]

For some reason I cannot reply to your last comment about exception type @rzikm, but you can use ThrowsAnyAsync in that test, it'll accept all derived exception types.

[rzikm]

For some reason I cannot reply to your last comment about exception type @rzikm, but you can use ThrowsAnyAsync in that test, it'll accept all derived exception types.

@ManickaP, I don't believe this is the right thing to do. From the user's perspective the code should throw only QuicOperationAbortedException, since he aborted the connection locally, and we should not relax this to plain QuicException to accommodate the data race.

[CarnaViire]

LGTM, I think 😄

[ManickaP]

Thanks! 🚀

[rzikm]

Test failures are unrelated (work item failures in unrelated libraries)

[AndyAyersMS]

Perf auto-filing seems to think this PR might have lead to these improvements: not sure though.

• [Perf] Changes at 4/26/2022 11:56:10 PM perf-autofiling-issues#5093
• [Perf] Changes at 4/27/2022 1:24:17 PM perf-autofiling-issues#5094