-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Description
Our current API for opening a QuicStream is synchronous and will throw if stream limit is reached. Waiting for available streams was intended to be a loop of checking for available stream count:
runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3Connection.cs
Lines 183 to 200 in c9f7f73
| while (true) | |
| { | |
| lock (SyncObj) | |
| { | |
| if (_connection == null) | |
| { | |
| break; | |
| } | |
| if (_connection.GetRemoteAvailableBidirectionalStreamCount() > 0) | |
| { | |
| quicStream = _connection.OpenBidirectionalStream(); | |
| requestStream = new Http3RequestStream(request, this, quicStream); | |
| _activeRequests.Add(quicStream, requestStream); | |
| break; | |
| } | |
| waitTask = _connection.WaitForAvailableBidirectionalStreamsAsync(cancellationToken); |
This proved to be highly inefficient - adding this loop around the opening eats up 80% of perf even when there's no actual need to wait.
MsQuic has it's own waiting logic implemented, and opening a stream can be asynchronous. As soon as stream is available, START_COMPLETE event will come to the stream. This can be done by passing ASYNC flag instead of FAIL_BLOCKED to StreamStart.
We need to:
- Change Open(Uni|Bidi)rectionalStream to Open(Uni|Bidi)rectionalStreamAsync, that will create the stream and then wait for the stream to be started
- We would need additional API on QuicStream; most possibly add "StartAsync" that will contain StreamStart call moved from .ctor and then wait on TCS for the event
- Remove wait for available streams loop from H3 (the code linked above)
Some thoughts:
- If cancellation occurs, if stream is already created but not started, it needs to be closed and disposed.
- Need to consider all places where TCS on opening a stream should be set to canceled (connection close, dispose etc)
- How dispose should work if stream is not started? Consider race conditions
- Do we need state checks (throwing on Send if stream is not started yet? etc)
Note: since msquic 2.0 I believe there will be no ASYNC flag anymore because async behavior is the default. (NONE flag)
Note 2: this is only for outbound streams. Inbound streams are already started when accepted.