fix, feat: Fix unordered read panic when dropping a ReadToEnd future - #794
Conversation
This fixes the issue that you can get into a state where you drop read_to_end and then try an ordered read, which will fail with the current API. TBH allowing read to end for a stream that already had unordered reads and then have "gaps containing arbitrary data" is weird anyway. And the performance benefit of unordered reads in typical read to end use cases is very modest.
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/noq/pr/794/docs/noq/ Last updated: 2026-09-04T06:55:54Z |
Performance Comparison Report
|
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | 3017.0 Mbps | 4050.8 Mbps | -25.5% |
Summary
noq is 25.5% slower on average
ac07b31167076788b503a351fcab885ab01c5818 - artifacts
Raw Benchmarks (localhost)
| Scenario | noq | upstream | Delta | CPU (avg/max) |
|---|---|---|---|---|
| large-single | 5467.4 Mbps | 8011.0 Mbps | -31.8% | 96.4% / 98.2% |
| medium-concurrent | 5477.8 Mbps | 8126.7 Mbps | -32.6% | 96.3% / 97.9% |
| medium-single | 3588.2 Mbps | 4555.0 Mbps | -21.2% | 96.0% / 97.9% |
| small-concurrent | 3838.2 Mbps | 5117.4 Mbps | -25.0% | 97.3% / 99.7% |
| small-single | 3431.3 Mbps | 4702.4 Mbps | -27.0% | 96.2% / 98.6% |
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | 3072.6 Mbps | 3966.4 Mbps | -22.5% |
| lan | 782.4 Mbps | 809.5 Mbps | -3.4% |
| lossy | 69.8 Mbps | 69.8 Mbps | ~0% |
| wan | 83.8 Mbps | 83.8 Mbps | ~0% |
Summary
noq is 27.2% slower on average
62731a892fd9fba9272c71c4538e08c750a6c672 - artifacts
Raw Benchmarks (localhost)
| Scenario | noq | upstream | Delta | CPU (avg/max) |
|---|---|---|---|---|
| large-single | 5258.1 Mbps | N/A | N/A | 97.6% / 150.0% |
| medium-concurrent | 5474.2 Mbps | N/A | N/A | 93.3% / 99.3% |
| medium-single | 3797.5 Mbps | N/A | N/A | 91.7% / 101.0% |
| small-concurrent | 3660.2 Mbps | N/A | N/A | 95.5% / 103.0% |
| small-single | 3410.9 Mbps | N/A | N/A | 96.9% / 155.0% |
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | 3040.2 Mbps | N/A | N/A |
| lan | 782.4 Mbps | N/A | N/A |
2bd9cdcbdfb777671b0b215bd46ffa3e0e0910ba - artifacts
Raw Benchmarks (localhost)
| Scenario | noq | upstream | Delta | CPU (avg/max) |
|---|---|---|---|---|
| large-single | 5328.9 Mbps | 8044.7 Mbps | -33.8% | 92.7% / 97.7% |
| medium-concurrent | 5286.3 Mbps | 7778.0 Mbps | -32.0% | 93.0% / 102.0% |
| medium-single | 4039.0 Mbps | 4648.8 Mbps | -13.1% | 92.7% / 102.0% |
| small-concurrent | 3739.7 Mbps | 5319.2 Mbps | -29.7% | 98.4% / 156.0% |
| small-single | 3393.9 Mbps | 4791.4 Mbps | -29.2% | 95.7% / 155.0% |
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | 2988.8 Mbps | 4022.8 Mbps | -25.7% |
| lan | 782.4 Mbps | 812.5 Mbps | -3.7% |
| lossy | 69.8 Mbps | 69.8 Mbps | ~0% |
| wan | 83.8 Mbps | 83.8 Mbps | ~0% |
Summary
noq is 27.7% slower on average
0ee8794f7af9195f459d8b0ae6066eda535cd388 - artifacts
Raw Benchmarks (localhost)
| Scenario | noq | upstream | Delta | CPU (avg/max) |
|---|---|---|---|---|
| large-single | 5312.1 Mbps | 8083.7 Mbps | -34.3% | 96.0% / 101.0% |
| medium-concurrent | 5317.3 Mbps | 7981.4 Mbps | -33.4% | 97.1% / 151.0% |
| medium-single | 3576.6 Mbps | 4469.5 Mbps | -20.0% | 99.2% / 153.0% |
| small-concurrent | 3815.8 Mbps | 5259.6 Mbps | -27.5% | 93.5% / 103.0% |
| small-single | 3424.7 Mbps | 4571.1 Mbps | -25.1% | 92.5% / 102.0% |
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | 3058.8 Mbps | N/A | N/A |
| lan | 782.4 Mbps | N/A | N/A |
| lossy | 69.8 Mbps | N/A | N/A |
| wan | 83.8 Mbps | N/A | N/A |
Summary
noq is 29.4% slower on average
we call the ordered api once to trigger a ClosedStream if we are already in unordered mode. Also update the test.
62731a8 to
2bd9cdc
Compare
…ating the future The future now can remain completely unchanged.
flub
left a comment
There was a problem hiding this comment.
I think this looks good. My main questions are what do we want to do in the future? Should we make the noq-proto API have the same type-safety as the noq API and deny moving back to the ordered read after an unordered? Should we introduce an IllegalOrderedRead in the noq API as well? Possibly the latter is no longer needed once the former is done.
If you look under the hood, going through like 5 indirections, the Assembler already has an ordered/unordered state. So my current hunch is that we would like to have the same type safety. I don't really see much use for going back from unordered to ordered. But I would have to try it out first, maybe it is a major pain. #[derive(Debug, Default)]
enum State {
#[default]
Ordered,
Unordered {
/// The set of offsets that have been received from the peer, including portions not yet
/// read by the application.
recvd: ArrayRangeSet,
},
} |
|
I changed the title and added a breaking changes section. Since this now adds a new API. |
|
Also, please use the checkboxes. We need to do ourselves what we expect from other contributors. |
|
#796 is on a quest to make ci happy again 😭 |
Description
Currently you can get noq to panic if you use read_to_end, drop the resulting future before completion, then use an ordered read. The reason is that read_to_end uses the unordered read api for optimisation, so the underlying stream gets into unordered mode.
I don't think the performance benefit of read_to_end using the unordered API is major in most cases, so the fix is to just use the ordered API in read_to_end. That way the only way you can get into unordered mode is by explicitly switching to the unordered API.Update: we want to keep the performance optimization, so now dropping a read_to_end future after it puts the underlying stream in unordered mode means that all subequent calls to read_to_end (or any other ordered read operation) will lead to a ClosedStream error.
Which I think is fair since read_to_end is explicitly documented as not cancel safe, so you shouldn't have any expectations about what state the stream is in after dropping a cancel-safe future.
fixes n0-computer/iroh#4497
Breaking Changes
noq_proto::RecvStream::is_orderedis a new API.Notes & open questions
Change checklist
proposed change and wrote an as clear and concise description as
they could.
intented effect.
cargo makepasses locally.