Fix StreamPriorityKey::cmp implementation #2287
Conversation
a1dedff to
45a3337
Compare
quiche/src/h3/mod.rs
Outdated
| more_frames: true, | ||
| }; | ||
| assert_eq!(ev, ev_headers); | ||
| assert_eq!(s.poll_server(), Ok((0, Event::Data))); |
There was a problem hiding this comment.
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
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
There was a problem hiding this comment.
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].
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
This was simpler than I expected.
|
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. |
a0bfa9d to
ad4d6eb
Compare
quiche/src/stream/mod.rs
Outdated
| /// 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. |
There was a problem hiding this comment.
nit: the QUIC layer doesn't know anything about headers. Maybe we can say something like
| /// 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. |
quiche/src/tests.rs
Outdated
| // 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); |
There was a problem hiding this comment.
This also seems suspect - the round robin should be determininstic no?
There was a problem hiding this comment.
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.
…s Ord trait contract while keeping the intended round-robin behavior.
b74bc30 to
bbc2c17
Compare
Fix
StreamPriorityKey::cmpimplementation so it doesn't violate Rust'sOrdtrait 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
Ordtrait contract.This is my attempt at fixing that and maintaining the round-robin requirement.
cycle_flushable()to assign new sequence numbers to push incremental streams to the back once they have been visited