Fix StreamPriorityKey::cmp implementation #2287

PSUdaemon · 2025-12-15T22:11:20Z

Fix StreamPriorityKey::cmp implementation so it doesn't violate Rust's Ord trait contract while keeping the intended round-robin behavior.

This is based on the discussion that was had in #2282. @antoniovicente had some concerns about the implementation with respect to the Ord trait contract.

This is my attempt at fixing that and maintaining the round-robin requirement.

Added a sequence field to preserve consistent ordering
Added cycle_flushable() to assign new sequence numbers to push incremental streams to the back once they have been visited

LPardue · 2025-12-16T00:01:58Z

quiche/src/h3/mod.rs

            more_frames: true,
        };
        assert_eq!(ev, ev_headers);
+        assert_eq!(s.poll_server(), Ok((0, Event::Data)));


Needing to change this test seems a bit fishy to me.

While RFC 9218 primarily targets prioritization of serving responses, we apply stream scheduling to all QUIC streams. The default priority for a new stream is, in quiche's model, for urgency=127 and incremental=true (see

quiche/quiche/src/stream/mod.rs

Line 798 in 86a79a7

fn default() -> Self {

). That should trigger round-robin sending of all request data.

In the previous test, we saw the server reading events in the round-robin order of stream sends. Although that's only partly true because the recv_body() function attempts to greedily read all available data frames independent of the peer's sender scheduling.

The changes to this test seem to imply that sending has turned into FIFO, where HEADERS and DATA are sent together (or, the peer is somehow receiving them together). It's not abundantly clear to me what's going on. I think it would pay to get some more understanding why this test triggered some change while others didn't

The old "interleaved" test results were an artifact of undefined behavior from the broken Ord implementation. With the fix, the h3 test now shows correct receive side sequential (priority) ordering per-stream.

To demonstrate that round-robin actually works, I added a new end to end test (incremental_stream_round_robin) that tracks per-packet send order. It shows the expected interleaving: [4, 8, 12, 4, 8, 12, 4, 8, 12].

Interesting, thanks for the explanation. This sounds a bit like a Bob Ross happy accident. It might be an undesirable change since it can be in the server's interest to "skim" across headers that is is receiving in order to do some useful work.

It might be that we need to make the choice of that behaviour more application configurable. That would potentially involves separate read and write priorities (which I appreciate is beyond the scope of a simple fix up).

A stop gap might be, on the reader side, to cycle the stream after generating an Event::Headers?

A stop gap might be, on the reader side, to cycle the stream after generating an Event::Headers?

Yeah, that's an interesting idea. Are you looking to have that added in this PR, or as a followup one?

I think for parity with the current behaviour, we'd need that in this PR. Would you mind trying it and see if it achieves the old ordering?

This was simpler than I expected.

LPardue · 2025-12-16T00:03:32Z

Thanks Phil. I appreciate the work to try and make this area more rusty. I'd like to dig in more to one of the tests as I commented above. But I think this is probably a path we can follow through with.

LPardue · 2025-12-16T08:03:31Z

quiche/src/stream/mod.rs

+    /// This is used for round-robin scheduling: after processing headers from
+    /// a stream, it should be moved to the back of its priority group to give
+    /// other streams a chance.


nit: the QUIC layer doesn't know anything about headers. Maybe we can say something like

Suggested change

/// This is used for round-robin scheduling: after processing headers from

/// a stream, it should be moved to the back of its priority group to give

/// other streams a chance.

/// This is used for round-robin scheduling: after processing some data from

/// a stream, it can be moved to the back of its priority group to give

/// other streams a chance of being read.

LPardue · 2025-12-16T08:07:23Z

quiche/src/tests.rs

+    // order, which may vary; we just need some data to be sent.
    let (len, _) = pipe.server.send(&mut buf).unwrap();
-    assert_eq!(len, 1200);
+    assert!(len > 0);


This also seems suspect - the round robin should be determininstic no?

It is, but I didn't think the specific value here was important and it could just cause churn in the future if other changes instigated a change here. I will change this to the new value.

PSUdaemon · 2026-01-05T17:57:41Z

Happy new year! 🎆

Checking back in to see if there are any more concerns for this PR?

LPardue · 2026-01-22T18:45:47Z

quiche/src/tests.rs

    // to update last_tx_data.
    let (len, _) = pipe.server.send(&mut buf).unwrap();
-    assert_eq!(len, 1200);
+    assert_eq!(len, 848);


How come this decreased?

I believe the stream ordering changed. If you look at lines 8241-8242 above, you can see 1200 bytes vs 800 bytes in two different streams. It's 800 bytes + 48 bytes of overhead. Why not 1248 then? The buffer on line 8240 is only 1200 bytes.

If you would like I can change this test more to prove that out.

I looked into this in detail, and it's a little more complicated (dealing with PTO and retransmission), but switching the order that we buffer the streams shows that it's stream 8 being selected. Specifically this is because we had emit_flight sending on the other stream (4) which increases the sequence number (sends it to the back) and thus a different stream (8) is selected.

…s Ord trait contract while keeping the intended round-robin behavior.

…d stream as the next chosen in new Ord implementation.

PSUdaemon requested a review from a team as a code owner December 15, 2025 22:11

PSUdaemon force-pushed the stream_ordering branch from a1dedff to 45a3337 Compare December 15, 2025 23:01

LPardue reviewed Dec 16, 2025

View reviewed changes

PSUdaemon force-pushed the stream_ordering branch from a0bfa9d to ad4d6eb Compare December 16, 2025 01:02

LPardue reviewed Dec 16, 2025

View reviewed changes

PSUdaemon force-pushed the stream_ordering branch from b74bc30 to bbc2c17 Compare December 18, 2025 21:33

PSUdaemon force-pushed the stream_ordering branch from bbc2c17 to 5fc3864 Compare January 13, 2026 23:39

LPardue reviewed Jan 22, 2026

View reviewed changes

PSUdaemon force-pushed the stream_ordering branch 4 times, most recently from 5e96cc0 to 87d88d7 Compare January 27, 2026 18:36

PSUdaemon force-pushed the stream_ordering branch 2 times, most recently from 85599d6 to 9179e4a Compare January 30, 2026 16:12

PSUdaemon added 5 commits January 30, 2026 10:52

Fix StreamPriorityKey::cmp implementation so it doesn't violate Rust'…

43a957b

…s Ord trait contract while keeping the intended round-robin behavior.

Add test to demonstrate sender side interleaving.

e412618

Cycle readable streams after Event::Headers.

a03fa38

Update comment and put specific value in test.

0ea46c6

Change the ordering of the stream buffering to keep the first buffere…

acbbbfa

…d stream as the next chosen in new Ord implementation.

PSUdaemon force-pushed the stream_ordering branch from 9179e4a to acbbbfa Compare January 30, 2026 17:52

Fix StreamPriorityKey::cmp implementation #2287

Are you sure you want to change the base?

Fix StreamPriorityKey::cmp implementation #2287

Uh oh!

Conversation

PSUdaemon commented Dec 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LPardue commented Dec 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PSUdaemon commented Jan 5, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants