msquic icon indicating copy to clipboard operation
msquic copied to clipboard

Add increments into STREAMS_AVAILABLE event

Open ManickaP opened this issue 1 year ago • 2 comments

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

  • Report the number from the frame / initial setting directly (MaxStreams value) instead of the increments.
  • Use different naming for the properties.

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

ManickaP avatar Jun 27 '24 08:06 ManickaP

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

codecov[bot] avatar Jun 27 '24 08:06 codecov[bot]

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.

ManickaP avatar Jun 27 '24 13:06 ManickaP