microsoft / msquic

Cross-platform, C implementation of the IETF QUIC protocol, exposed to C, C++, C# and Rust.
MIT License
3.92k stars 525 forks source link

Add increments into STREAMS_AVAILABLE event #4376

Closed ManickaP closed 2 days ago

ManickaP commented 1 week ago

Description

In case the current count of available streams is bellow 0, STREAMS_AVAILABLE event will indicate 0, making it impossible to know the actual update that came over the wire.

Alternatives

Testing

No existing tests cover the pre-existing values in this event, but this was tested with .NET runtime and System.Net.Quic that plans to take advantage of this feature for it's own stream counting (https://github.com/dotnet/runtime/commit/36210649c502c006e0bdd1439f9ba4c8a976df28)

Documentation

Updated.

cc @MihaZupan @rzikm @CarnaViire @liveans

codecov[bot] commented 1 week ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 84.21%. Comparing base (8a741dc) to head (cdce3c6).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #4376 +/- ## ========================================== - Coverage 85.46% 84.21% -1.25% ========================================== Files 56 56 Lines 15426 15429 +3 ========================================== - Hits 13184 12994 -190 - Misses 2242 2435 +193 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ManickaP commented 1 week ago

Generally, the change looks fine, but I'd really like to understand more why you want this. How will this help you?

We're exposing a callback similar to STREAMS_AVAILABLE event in System.Net.Quic (https://github.com/dotnet/runtime/issues/101534 - code: https://github.com/dotnet/runtime/blob/f7636f44caa24c1e9613aa417cbbd649d2e37154/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnectionOptions.cs#L142-L148). The main use is to support opening of new connection when the limit is reached on an existing one. But we also want to know when we can start re-using the original one.

The goal of this change is to report correct numbers, which is not possible due to https://github.com/microsoft/msquic/blob/f5bec530c8ef0824df7294f11d7b3516919899b1/src/core/stream_set.c#L482-L484

I'm also not happy about the uint_16.max cut off for the max value, so I'm starting to get more inclined to expose the raw number from MAX_STREAMS frame in here.