fix: Handle ENOBUFS in UDP send#3482
Conversation
larseggert
requested review from
KershawChang,
martinthomson and
mxinden
as code owners
There was a problem hiding this comment.
Pull request overview
Updates neqo-udp’s UDP send error handling to avoid treating ENOBUFS as a fatal send error, aligning send behavior with transient NIC/stack buffer exhaustion on some platforms.
Changes:
- Treat
ENOBUFSfromUdpSocketState::try_send()asio::ErrorKind::WouldBlockinstead of a hard error. - Add
is_enobufs()helper (unix-gated) and update unit tests to account for the new predicate.
You can also share your feedback on Copilot code review. Take the survey.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3482 +/- ##
==========================================
- Coverage 95.15% 95.00% -0.16%
==========================================
Files 111 116 +5
Lines 38012 38373 +361
Branches 38012 38373 +361
==========================================
+ Hits 36172 36455 +283
- Misses 1152 1219 +67
- Partials 688 699 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
|
larseggert
force-pushed
the
fix-enobufs
branch
3 times, most recently
from
04de82b to
345f3cf
Compare
Based on a discussion with @ianswett yesterday, don't treat `ENOBUFS` as a fatal error when sending UDP packets.
|
No objections! |
|
@mxinden quinn-rs/quinn#2293 got closed - what now? |
|
@larseggert let's try to get to the root of the discussion. I prepared quinn-rs/quinn#2594. Can you help out? |
|
Claude finished @larseggert's task in 2m 3s —— View job Review of PR #3482
SummaryReviewed the ENOBUFS handling changes. The approach is correct — callers already handle
Also addressed previous copilot review comments (original error loss is acceptable given |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
Maps ENOBUFS (and WSAENOBUFS on Windows) to WouldBlock during UDP send, so the existing retry loops in both client and server correctly wait for writability and retry rather than treating it as fatal. Also cleans up is_emsgsize by switching from map_or to is_some_and, removing the clippy suppression.
The approach is sound — I verified that callers (neqo-bin/src/client/mod.rs:484, neqo-bin/src/server/mod.rs:325) handle WouldBlock with a writable().await + retry loop, which is exactly the right recovery for a transient send queue exhaustion. The try_io wrapper in neqo-bin/src/udp.rs:93 also correctly propagates the WouldBlock through tokio's readiness machinery.
Notes
- Comment/code scope mismatch: The inline comment says "On macOS/BSD" but the predicate fires on all unix + Windows. See inline suggestion.
- Windows test gap:
is_enobufson Windows (WSAENOBUFS) has no test coverage. See inline suggestion. - Upstream dedup: Agree with @mxinden that this could live in
quinn-udpto benefit all consumers. If quinn-rs/quinn#2594 lands, this local workaround can be removed. Worth tracking.
On previous review comments
- @copilot re: losing the original error (line 90): Agreed in principle, but callers only inspect
e.kind()and the original is captured byqdebug!. Not blocking. - @copilot re: missing
send_innerintegration test (line 91): Triggering ENOBUFS in a test environment is impractical without mocking the socket layer. The unit tests on the predicates are sufficient. - @copilot re: EAGAIN coverage dropped (line 384): This is addressed — the combined
is_emsgsize_and_is_enobufs_are_disjointtest at line 377 asserts both predicates returnfalseforEAGAIN.
Reviewing PR #3482
|
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Signed-off-by: Lars Eggert <lars@eggert.org>
|
Claude finished @larseggert's task in 2m 44s —— View job Reviewing PR #3482
SummaryReviewed the ENOBUFS → WouldBlock mapping. The approach integrates correctly with tokio's readiness model via |
There was a problem hiding this comment.
Approach is sound. ENOBUFS → WouldBlock integrates correctly with the callers: both client/mod.rs:484 and server/mod.rs:325 retry in a writable().await loop, and critically, the try_io wrapper in neqo-bin/src/udp.rs:93 ensures tokio clears the socket's readiness flag on WouldBlock, preventing a busy-loop.
The is_emsgsize cleanup (is_some_and replacing map_or + clippy suppression) and the combined disjointness test are nice improvements.
One inline comment for missing Windows test coverage. Previous reviewer comments about upstream dedup in quinn-udp (quinn-rs/quinn#2594) are worth tracking — if that lands, this local workaround can be removed.
|
This is on hold pending the discussion in quinn-rs/quinn#2594 |
There was a problem hiding this comment.
Clean, well-motivated fix. Treating ENOBUFS as fatal was clearly wrong for a QUIC implementation — mapping it to a retryable condition is the right call.
What's good:
- The refactoring of
is_emsgsize/is_enobufsinto separate per-platform functions (addressing @martinthomson'scfg!()concern) is much cleaner than the previous#[cfg]-blocks-inside-closures approach, and drops theclippy::unnecessary_map_orsuppression. - Test coverage is thorough: disjointness test on Unix, per-error-code tests on Windows, non-OS-error test.
- Previous review feedback (comment wording, Windows
WSAENOBUFStest gap) has all been addressed.
One note (see inline): the synthetic WouldBlock interacts with tokio's try_io readiness clearing. Unlikely to cause issues in practice, but worth being aware of.
Agree with @mxinden that the long-term home for this is quinn-udp (quinn-rs/quinn#2293). Worth tracking so this local workaround can be removed when upstream support lands.
|
Note to self: Add a test that confirms this is basically packet loss and triggers an RTX. |
There was a problem hiding this comment.
Clean, well-motivated fix. ENOBUFS should not be fatal for a QUIC stack — mapping it to a retryable condition is correct.
What looks good:
- Splitting
is_emsgsize/is_enobufsinto separate per-platform functions (addressing @martinthomson'scfg!()readability concern) is a clear improvement over the#[cfg]-blocks-inside-closures approach, and drops theclippy::unnecessary_map_orsuppression. - All prior review feedback has been addressed: the comment wording is now platform-neutral, the Windows
WSAENOBUFStest has been added, and test coverage is thorough (disjointness on Unix, per-code on Windows, non-OS-error fallback). - Callers in both
client/mod.rsandserver/mod.rshandleWouldBlockwith awritable().await+ retry loop, which correctly re-sends the dropped batch.
Acknowledged concern (already discussed): The synthetic WouldBlock interacts with tokio's try_io readiness clearing. On edge-triggered platforms (Linux epoll), if ENOBUFS doesn't correlate with actual socket send buffer exhaustion, writable().await could stall until a QUIC timer fires. As previous reviews noted, this is low-risk in practice — ENOBUFS from UDP sendmsg is rare on Linux, and QUIC idle/loss-detection timers provide a safety net. Worth keeping in mind if this is ever observed to cause stalls.
Upstream tracking: Agree with @mxinden that the long-term home for this is quinn-udp (quinn-rs/quinn#2293). Once upstream support lands, this local workaround can be removed.
Benchmark resultsNo significant performance differences relative to 9a957f6. All resultstransfer/1-conn/1-100mb-resp (aka. Download): Change within noise threshold. time: [184.08 ms 184.51 ms 184.97 ms]
thrpt: [540.63 MiB/s 541.97 MiB/s 543.24 MiB/s]
change:
time: [+0.4975% +0.7970% +1.1185] (p = 0.00 < 0.05)
thrpt: [-1.1062% -0.7907% -0.4950]
Change within noise threshold.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high severetransfer/1-conn/10_000-parallel-1b-resp (aka. RPS): No change in performance detected. time: [283.57 ms 285.36 ms 287.16 ms]
thrpt: [34.824 Kelem/s 35.044 Kelem/s 35.265 Kelem/s]
change:
time: [-0.0354% +0.8726% +1.7553] (p = 0.06 > 0.05)
thrpt: [-1.7250% -0.8650% +0.0355]
No change in performance detected.
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high mildtransfer/1-conn/1-1b-resp (aka. HPS): Change within noise threshold. time: [39.073 ms 39.224 ms 39.399 ms]
thrpt: [25.381 B/s 25.494 B/s 25.593 B/s]
change:
time: [-1.5761% -0.8648% -0.1572] (p = 0.02 < 0.05)
thrpt: [+0.1574% +0.8723% +1.6013]
Change within noise threshold.
Found 8 outliers among 100 measurements (8.00%)
3 (3.00%) high mild
5 (5.00%) high severetransfer/1-conn/1-100mb-req (aka. Upload): Change within noise threshold. time: [189.50 ms 190.13 ms 191.07 ms]
thrpt: [523.38 MiB/s 525.94 MiB/s 527.70 MiB/s]
change:
time: [+0.3627% +0.8334% +1.3505] (p = 0.00 < 0.05)
thrpt: [-1.3325% -0.8265% -0.3614]
Change within noise threshold.
Found 8 outliers among 100 measurements (8.00%)
1 (1.00%) low mild
4 (4.00%) high mild
3 (3.00%) high severestreams/walltime/1-streams/each-1000-bytes: No change in performance detected. time: [590.13 µs 592.28 µs 594.76 µs]
change: [-0.2820% +0.2688% +0.7713] (p = 0.34 > 0.05)
No change in performance detected.
Found 9 outliers among 100 measurements (9.00%)
1 (1.00%) high mild
8 (8.00%) high severestreams/walltime/1000-streams/each-1-bytes: No change in performance detected. time: [11.913 ms 11.952 ms 12.013 ms]
change: [-0.2859% +0.0996% +0.6311] (p = 0.72 > 0.05)
No change in performance detected.
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high severestreams/walltime/1000-streams/each-1000-bytes: Change within noise threshold. time: [42.434 ms 42.472 ms 42.511 ms]
change: [+0.5524% +0.6924% +0.8256] (p = 0.00 < 0.05)
Change within noise threshold.
Found 3 outliers among 100 measurements (3.00%)
3 (3.00%) high mildstreams-flow-controlled/walltime/1-streams/each-4194304-bytes: Change within noise threshold. time: [33.503 ms 33.562 ms 33.622 ms]
change: [+0.9917% +1.3217% +1.6221] (p = 0.00 < 0.05)
Change within noise threshold.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mildstreams-flow-controlled/walltime/10-streams/each-1048576-bytes: No change in performance detected. time: [94.693 ms 95.940 ms 97.226 ms]
change: [-0.3679% +1.5294% +3.3250] (p = 0.11 > 0.05)
No change in performance detected.transfer/walltime/pacing-false/varying-seeds: Change within noise threshold. time: [22.642 ms 22.669 ms 22.704 ms]
change: [+1.2730% +1.4503% +1.6367] (p = 0.00 < 0.05)
Change within noise threshold.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high severetransfer/walltime/pacing-true/varying-seeds: Change within noise threshold. time: [22.835 ms 22.866 ms 22.915 ms]
change: [+0.9532% +1.1164% +1.3911] (p = 0.00 < 0.05)
Change within noise threshold.
Found 9 outliers among 100 measurements (9.00%)
5 (5.00%) low mild
2 (2.00%) high mild
2 (2.00%) high severetransfer/walltime/pacing-false/same-seed: Change within noise threshold. time: [22.610 ms 22.630 ms 22.652 ms]
change: [+2.1062% +2.2234% +2.3427] (p = 0.00 < 0.05)
Change within noise threshold.
Found 3 outliers among 100 measurements (3.00%)
1 (1.00%) high mild
2 (2.00%) high severetransfer/walltime/pacing-true/same-seed: Change within noise threshold. time: [22.872 ms 22.901 ms 22.946 ms]
change: [+0.1805% +0.3212% +0.5158] (p = 0.00 < 0.05)
Change within noise threshold.
Found 4 outliers among 100 measurements (4.00%)
3 (3.00%) high mild
1 (1.00%) high severeDownload data for |
Failed Interop TestsQUIC Interop Runner, client vs. server, differences relative to
All resultsSucceeded Interop TestsQUIC Interop Runner, client vs. server neqo-pr as client
neqo-pr as server
Unsupported Interop TestsQUIC Interop Runner, client vs. server neqo-pr as client
neqo-pr as server
|
Client/server transfer resultsPerformance differences relative to f21a27d. Transfer of 33554432 bytes over loopback, min. 100 runs. All unit-less numbers are in milliseconds.
Table above only shows statistically significant changes. See all results below. All resultsTransfer of 33554432 bytes over loopback, min. 100 runs. All unit-less numbers are in milliseconds.
Download data for |
Based on a discussion with @ianswett yesterday, don't treat
ENOBUFSas a fatal error when sending UDP packets.