Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[API Proposal]: QUIC stream priority support #90281

Open
alexrp opened this issue Aug 9, 2023 · 14 comments
Open

[API Proposal]: QUIC stream priority support #90281

alexrp opened this issue Aug 9, 2023 · 14 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Quic
Milestone

Comments

@alexrp
Copy link
Contributor

alexrp commented Aug 9, 2023

MsQuic has support for stream prioritization: https://github.com/microsoft/msquic/blob/main/docs/Settings.md#stream-parameters (QUIC_PARAM_STREAM_PRIORITY)

This is something I'd like to use in my application, so I wanted to first check if there are already existing plans to support this in System.Net.Quic? (I saw there's discussion about adding more options in #72984, but that seems to be specifically about connection-level options.)

If there are no existing plans for this, I can turn this issue into an actual API proposal.


API proposal after discussion

+public sealed class QuicStreamOptions
+{
+    public byte? Priority { get; set; }
+}

 public sealed class QuicConnection
 {
     public ValueTask<QuicStream> AcceptInboundStreamAsync(CancellationToken cancellationToken = default);
+    public ValueTask<QuicStream> AcceptInboundStreamAsync(QuicStreamOptions options, CancellationToken cancellationToken = default);
     public ValueTask<QuicStream> OpenOutboundStreamAsync(QuicStreamType type, CancellationToken cancellationToken = default);
+    public ValueTask<QuicStream> OpenOutboundStreamAsync(QuicStreamType type, QuicStreamOptions options, CancellationToken cancellationToken = default);
}

Considerations:

  • While MsQuic supports ushort priority values, quiche only supports byte, hence Priority being typed as the latter.
    • Alternatively, we could make it an int and just limit the range until such a time that we might want to allow a broader range of values.
    • Another approach would be to use a QuicStreamPriority enum, akin to ThreadPriority, but perhaps this is too constraining.
  • Should it be possible to change the stream priority after stream creation? I think both MsQuic and quiche support this.
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Aug 9, 2023
@ghost
Copy link

ghost commented Aug 9, 2023

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

Issue Details

MsQuic has support for stream prioritization: https://github.com/microsoft/msquic/blob/main/docs/Settings.md#stream-parameters (QUIC_PARAM_STREAM_PRIORITY)

This is something I'd like to use in my application, so I wanted to first check if there are already existing plans to support this in System.Net.Quic? (I saw there's discussion about adding more options in #72984, but that seems to be specifically about connection-level options.)

If there are no existing plans for this, I can turn this issue into an actual API proposal.

Author: alexrp
Assignees: -
Labels:

untriaged, area-System.Net.Quic

Milestone: -

@ManickaP ManickaP added this to the 9.0.0 milestone Aug 10, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Aug 10, 2023
@ManickaP
Copy link
Member

If there are no existing plans for this, I can turn this issue into an actual API proposal.

Hey, no plans for stream level configuration yet. If you wish to work an API proposal, we'll gladly take it and help wherever needed.

I did a quick search on the spec (https://www.rfc-editor.org/rfc/rfc9000.html#name-stream-prioritization) and Rust quiche implementation of this feature (https://docs.rs/quiche/latest/quiche/struct.Connection.html#method.stream_priority). The spec doesn't say anything about representation of this and it doesn't affect the wire operations, it's just a suggestion on possible implementation. Thus, MsQuic use different type (ushort) then Rust quiche (byte) for priority. So we'd like to avoid getting locked in MsQuic way and the proposal should take into consideration other implementations.

Whether you feel like looking into it or not, this will not get in earlier than 9.0 as all API changes for 8.0 are past due.

@ManickaP ManickaP added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Aug 10, 2023
@alexrp
Copy link
Contributor Author

alexrp commented Aug 10, 2023

Given the difference in priority type, do you think it would make more sense to go with byte, or perhaps a QuicStreamPriority enum with a much more limited set of values (along the lines of ThreadPriority)? The latter would be fine for my purposes as I just have a few streams, so even byte range is way more than I'll ever need. But I'm not sure what other people are using QUIC for, so hard for me to extrapolate from that.

Additionally, would we want this to be configurable at stream creation time (i.e. with a QuicStreamOptions class) rather than just making it a settable property on QuicStream?

🤔

@Softhead
Copy link

I think it should be a settable property on QuicStream. As well, create a QuicStreamOptions class so that when you call QuicConnection.OpenOutboundStreamAsync you can provide the options, instead of having a large group of different definitions for OpenOutboundStreamAsync, one for each combination of options.

@alexrp
Copy link
Contributor Author

alexrp commented Jan 8, 2024

Given the difference in priority type, do you think it would make more sense to go with byte, or perhaps a QuicStreamPriority enum with a much more limited set of values (along the lines of ThreadPriority)? The latter would be fine for my purposes as I just have a few streams, so even byte range is way more than I'll ever need. But I'm not sure what other people are using QUIC for, so hard for me to extrapolate from that.

@ManickaP any thoughts on this part?

@ManickaP
Copy link
Member

ManickaP commented Jan 9, 2024

Given the difference in priority type, do you think it would make more sense to go with byte, or perhaps a QuicStreamPriority enum with a much more limited set of values (along the lines of ThreadPriority)? The latter would be fine for my purposes as I just have a few streams, so even byte range is way more than I'll ever need. But I'm not sure what other people are using QUIC for, so hard for me to extrapolate from that.

Additionally, would we want this to be configurable at stream creation time (i.e. with a QuicStreamOptions class) rather than just making it a settable property on QuicStream?

🤔

Both byte and QuicStreamPriority seem reasonable, I personally don't have preference, we can suggest one as preferred and put the other one as an alternative design. And this should be configurable with stream creation (QuicStreamOptions seems appropriate and in line with other options we have), not a settable property. In general, we don't allow changing settings after the thing is in use.

And sorry for such a late reply, it got off my radar.

@alexrp
Copy link
Contributor Author

alexrp commented Jan 9, 2024

No problem, thanks for the reply. 🙂

I'll type up an updated proposal later today.

@alexrp
Copy link
Contributor Author

alexrp commented Jan 9, 2024

Updated the issue with a proposed API surface and some considerations for API review.

@alexrp alexrp changed the title QUIC stream priority support [API Proposal]: QUIC stream priority support Jan 9, 2024
@alexrp
Copy link
Contributor Author

alexrp commented Apr 26, 2024

Just checking in, do I need to do anything else for this to become ready for API review? 👀

@ManickaP
Copy link
Member

ManickaP commented Apr 26, 2024

I just put it on my team's radar, we'll discuss it in 2 weeks time in the latest (I'll be OOF for 2 weeks). Though be aware that as you're the only one asking for this (no upvotes, nobody else commenting 1 person commenting) it might get pushed out of 9.0.

@alexrp
Copy link
Contributor Author

alexrp commented Apr 26, 2024

Yeah, that's fine, just wanted to make sure it wasn't forgotten. 🙂

@ManickaP
Copy link
Member

ManickaP commented Jun 10, 2024

Triage: we decided to wait with this API as we have only one person wanting this at the moment. If there're more people out there looking for this, please upvote so we can prioritize this better (pun intended 😄 )

@ManickaP ManickaP modified the milestones: 9.0.0, Future Jun 10, 2024
@alexrp
Copy link
Contributor Author

alexrp commented Jul 12, 2024

Can it be reconsidered for .NET 10 planning? Or is it just dependent on more people needing it?

@ManickaP
Copy link
Member

ManickaP commented Jul 12, 2024

dependent on more people needing it

Yes and no. This is backed by RFC, which gives us an argument to introduce it without a great number of people asking for it. On the other hand, you're the only one asking so that gives us argument to chop it.
Having few more people upvoting / commenting would improve the chances of being introduced in 10.0.

EDIT: I moved to 10.0 for now.

@ManickaP ManickaP modified the milestones: Future, 10.0.0 Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Quic
Projects
None yet
Development

No branches or pull requests

3 participants