Apply -w to UDP SO_SNDBUF / SO_RCVBUF on both ends#75
Merged
Conversation
The -w/--window value already plumbed from the client through TestStart to the server, but applied only to TCP sockets. For UDP the kernel buffer size matters more than for TCP — high-rate flows against a receiver that can't drain fast enough silently tail-drop new arrivals, and the loss only surfaces as a sequence-gap when traffic finally lulls (reproduced as a 'live counter stuck at 0%, final reports 74%' bug when an existing tester ran against a saturated server). New net::set_udp_buffer_size helper sets both SO_SNDBUF and SO_RCVBUF on a tokio UdpSocket, mirroring the validation in tcp::configure_socket_buffers (c_int range, positive). Applied at: * server-side per-stream UDP socket creation (src/serve.rs Protocol::Udp branch), gated on the client's window_size from TestStart * client-side per-stream UDP socket spawn (src/client.rs), gated on the client's local config.window_size setsockopt failures log at debug level to avoid spamming users on kernels with low rmem_max ceilings; the kernel's clamped value still beats the default in most cases. Two new tests: * validation rejects zero and c_int-overflow sizes up front * Linux-only end-to-end check that getsockopt-reads back show SO_RCVBUF and SO_SNDBUF both grew vs. the default after a 4 MB set_udp_buffer_size call (kernel doubles + clamps, so we assert growth rather than equality)
Two corrections from review: * set_udp_buffer_size used to log setsockopt rejections at debug level and return Ok(()), which silently swallowed the exact failure case users running -w 16M for #70-style troubleshooting most need to know about — a kernel rejection because the request exceeds net.core.rmem_max without CAP_NET_ADMIN. Errors now bubble up; the existing call sites in client.rs and serve.rs already pattern-match on Err and warn!(), so the failure becomes visible without further changes. * The --window CLI help still said 'TCP window size' from when -w only affected TCP. Reword to cover both protocols and call out that UDP uses the kernel default when unset (TCP autotunes). A new test (set_udp_buffer_size_propagates_kernel_rejection) feeds a near-c_int::MAX value to exercise the surfacing path. Most kernels silently clamp rather than error in this case, so the test handles both outcomes — the contract it pins is 'errors are surfaced when they happen,' not 'this specific value is always rejected.'
Previous version returned early on the first setsockopt failure, which defeated the receiver-side mitigation in exactly the case it was meant for: a server whose wmem_max is below the requested size would fail SO_SNDBUF and skip SO_RCVBUF entirely, even though the recv-buffer bump is the part that actually addresses the kernel-tail-drop story behind #70. set_one_buffer is a small helper that wraps a single setsockopt call and returns Option<io::Error>; set_udp_buffer_size now invokes it for SO_SNDBUF and SO_RCVBUF independently and combines the results, attaching the option name(s) to the error message so the caller's warn! actually identifies which buffer failed. Tests: * set_one_buffer_surfaces_setsockopt_failure passes -1 for the fd to deterministically trigger EBADF, exercising the propagation path without depending on environment-specific sysctls (the previous near-c_int::MAX test was a smoke guard that often took the Ok branch via silent kernel clamping) * set_udp_buffer_size_attempts_both_buffers_on_failure pins the regression via a successful 64 KB request — small enough that no realistic rmem_max/wmem_max ceiling rejects it, so we can require Ok(()) and fail loudly if the helper ever short-circuits again
Three small follow-ups: * README: the config-file example, options table, and Low-throughput troubleshooting section all described --window as TCP-only. Reword to cover UDP too, and add an entry to UDP packet loss explaining that bumping -w is the immediate workaround for kernel tail-drops hiding as live 0.0% loss. * set_udp_buffer_size doc comment claimed 'returns the first error' — stale from the earlier early-return draft. Updated to describe the current both-attempted, combined-error semantics and explain why receive-side independence is the load-bearing detail for #70. * Renamed set_udp_buffer_size_attempts_both_buffers_on_failure (which only proved a valid request succeeded) to set_udp_buffer_size_succeeds_within_kernel_ceilings. Added a real per-option independence test (set_one_buffer_per_option_naming_round_trip) that exercises set_one_buffer twice with an invalid fd and asserts each call returns a distinct error keyed off the option passed in.
The previous name (set_one_buffer_per_option_naming_round_trip) suggested set_one_buffer returns something keyed by option name. It doesn't — both calls just return the kernel's EBADF on an invalid fd. The test pins per-call error surfacing for each option; independence of the two invocations is a property of the call site in set_udp_buffer_size, not of this helper.
Refactor: factor set_udp_buffer_size into three pieces with testability in mind: * validate_udp_buffer_size — c_int range / positivity check, returns the validated size_c * combine_udp_buffer_errors — turns the (Option<Err>, Option<Err>) outcomes from the two setsockopt calls into a labeled io::Result * set_udp_buffer_size_with<F> — orchestrates the validate-then-call- twice flow, taking the per-option setter as a closure Production set_udp_buffer_size now boils down to: set_udp_buffer_size_with(size, |opt, sz| set_one_buffer(fd, opt, sz)) Four new tests cover the actual receiver-side mitigation contract for #70 — the place earlier suites only verified at code-review level: * fake_setter_sndbuf_failure_still_attempts_rcvbuf — the regression this whole iteration cared about. Fake setter fails on SO_SNDBUF, succeeds on SO_RCVBUF; assert both calls were made (in order) and the returned error names only SO_SNDBUF. * fake_setter_rcvbuf_failure_reports_rcvbuf_only — opposite case; error names only SO_RCVBUF. * fake_setter_both_failures_combine_into_one_error — both fail; one io::Error string contains both option names. * fake_setter_both_success_returns_ok_and_attempts_both — happy path with call-tracking to confirm exactly two setsockopt invocations with the validated size. These run anywhere unix tests run (no kernel sysctl dependency, no socket bind required) and pin the contract independent of platform buffer ceilings.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #70. The `-w`/`--window` value already travels client → `TestStart.window_size` → server, but historically only the TCP code path applied it (issue #60, v0.9.9). For UDP the kernel buffer size has a bigger UX impact: high-rate UDP flows against a receiver that can't drain fast enough silently tail-drop new arrivals at the kernel level, and the loss only becomes visible to xfr's sequence-gap accounting once traffic stops and the queue drains. That mechanism is what's behind the "live Packet Loss stuck at 0%, final summary reports 74%" symptom on #70 — see the diagnosis comment for the full chain.
Changes
Tests
Two new tests in `src/net.rs::tests`:
Test plan
What this doesn't fix
If the receive buffer is already at the kernel ceiling and packets are still being tail-dropped (CPU-bound receiver, incoming line rate exceeds drain rate), this PR doesn't help — for that we still want the Linux `SO_RXQ_OVFL` cmsg path to surface kernel drops directly, and longer-term a sender-side heartbeat protocol so live loss can be computed without depending on later sequence gaps. Both noted on #70.