feat(quiche): add keep-alive and a GSO opt-out#321
Conversation
Upstream enablers for the "QUIC transport controls and routing" gaps in moq-dev/moq#2296, where moq-native currently ignores or rejects these options on the quiche backend. Each is exposed on both the ez (raw QUIC) and WebTransport builders, client and server. Keep-alive (with_keep_alive): quiche has no keep-alive of its own and QuicSettings has no interval, so the ez driver now runs an interval that calls send_ack_eliciting() and returns Ready so the io loop flushes the PING. quiche only adds a PING when the next packet wouldn't already be ack-eliciting, so a tick on a busy connection costs nothing. GSO opt-out (with_gso): the client applied max socket capabilities unconditionally and the server did the same when adopting a socket. apply_all_and_get_compatibility always enables GSO and the flags are pub(crate), so opting out reproduces it via SocketCapabilitiesBuilder minus gso(). The server holds listeners and its own sockets in one insertion-ordered enum and applies capabilities at build time, so with_gso works regardless of call order without clobbering the capabilities on a caller-supplied listener. Dial address vs SNI (with_server_name, connect_to): decouples the dial target from the TLS server name, so a caller can resolve the host itself (address-family-aware selection) and verify against a different name. Also fixes a pre-existing bug this surfaced: Url::host() renders IPv6 in bracketed URL form, so connecting to an IPv6-literal URL always failed DNS resolution. It sits directly in the dial/SNI path, so the feature would not work for its main use case without it. This promotes url from a dev-dependency to a real one. Preferred address remains blocked in quiche. QUIC-LB needs no change here: with_listener already accepts a QuicListener carrying its own cid_generator. The keep-alive test is paired with a control proving an un-kept connection genuinely dies of the idle timeout, and was verified to fail when send_ack_eliciting() is removed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Sorry @kixelated, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
WalkthroughAdds configurable keep-alive intervals and UDP GSO behavior to client and server builders. Refactors server socket capability setup to occur during build, adds URL target handling for domain and literal IP hosts, and sends periodic ack-eliciting QUIC packets through the driver. Integration tests cover keep-alive, idle timeout, disabled GSO, and IPv4/IPv6 literal URL connections. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rs/web-transport-quiche/src/ez/client.rs`:
- Around line 197-199: Update the implicit bind logic in the client connection
path around self.with_bind so it selects 0.0.0.0:0 for IPv4 remote addresses and
[::]:0 for IPv6 remotes, preserving explicit socket bindings. Add an IPv4 test
that connects without calling with_bind and verifies the connection succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 08db4466-5df9-469f-902c-4b6abd159c2f
📒 Files selected for processing (10)
rs/web-transport-quiche/Cargo.tomlrs/web-transport-quiche/src/client.rsrs/web-transport-quiche/src/ez/client.rsrs/web-transport-quiche/src/ez/driver.rsrs/web-transport-quiche/src/ez/mod.rsrs/web-transport-quiche/src/ez/server.rsrs/web-transport-quiche/src/ez/socket.rsrs/web-transport-quiche/src/server.rsrs/web-transport-quiche/tests/transport.rsrs/web-transport-quiche/tests/verify.rs
| if self.socket.is_none() { | ||
| self = self.with_bind("[::]:0")?; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline rs/web-transport-quiche/src/ez/client.rs --match 'connect|connect_to' --view expanded
rg -n -C4 '\.connect(_to)?\s*\(' rs/web-transport-quiche/tests
rg -n -C3 'with_bind|\[::\]:0|0\.0\.0\.0:0' rs/web-transport-quicheRepository: moq-dev/web-transport
Length of output: 27222
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant client module and inspect the connect path around the implicit bind.
ast-grep outline rs/web-transport-quiche/src/ez/client.rs --view expanded
sed -n '140,230p' rs/web-transport-quiche/src/ez/client.rs
# Inspect address-family helpers / socket binding logic in the module.
rg -n -C3 'is_ipv4|is_ipv6|SocketAddr|ToSocketAddrs|with_bind\(' rs/web-transport-quiche/src/ez/client.rs rs/web-transport-quiche/src -g '!**/target/**'Repository: moq-dev/web-transport
Length of output: 18319
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Probe whether the proposed bind-address choice is actually required for IPv4 URLs.
python3 - <<'PY'
from urllib.parse import urlparse
cases = [
"https://127.0.0.1:443/",
"https://[::1]:443/",
]
for u in cases:
p = urlparse(u)
host = p.hostname
print(u, "hostname=", host, "ipv4=", host and host.count('.') == 3, "ipv6=", host and ':' in host)
PYRepository: moq-dev/web-transport
Length of output: 281
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import socket
def probe(bind_addr, remote_addr, family):
s = socket.socket(family, socket.SOCK_DGRAM)
try:
s.bind(bind_addr)
print("bind ok", bind_addr, "family", family, "sockname", s.getsockname())
try:
s.connect(remote_addr)
print("connect ok", remote_addr)
except OSError as e:
print("connect error", remote_addr, repr(e))
finally:
s.close()
print("IPV6_V6ONLY default:", socket.socket(socket.AF_INET6, socket.SOCK_DGRAM).getsockopt(socket.IPPROTO_IPV6, socket.IPV6_V6ONLY))
probe(("::", 0), ("127.0.0.1", 12345), socket.AF_INET6)
probe(("::", 0, 0, 0), ("127.0.0.1", 12345), socket.AF_INET6)
probe(("0.0.0.0", 0), ("127.0.0.1", 12345), socket.AF_INET)
probe(("::", 0), ("::1", 12345, 0, 0), socket.AF_INET6)
PYRepository: moq-dev/web-transport
Length of output: 669
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the public client wrapper's connect path for similar implicit-bind behavior.
sed -n '150,220p' rs/web-transport-quiche/src/client.rsRepository: moq-dev/web-transport
Length of output: 2563
Bind the implicit socket to the remote family. In rs/web-transport-quiche/src/ez/client.rs:197-199, [::]:0 will break IPv4 connect/connect_to; pick 0.0.0.0:0 for IPv4 remotes and add an IPv4 test that omits with_bind.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rs/web-transport-quiche/src/ez/client.rs` around lines 197 - 199, Update the
implicit bind logic in the client connection path around self.with_bind so it
selects 0.0.0.0:0 for IPv4 remote addresses and [::]:0 for IPv6 remotes,
preserving explicit socket bindings. Add an IPv4 test that connects without
calling with_bind and verifies the connection succeeds.
Reconciles with #322, which dropped ClientBuilder::with_metrics and the `M: Metrics` type parameter after this branch flagged the field as dead. Takes main's non-generic ClientBuilder and drops the `#[allow(dead_code)] metrics: M` field this branch had added to silence the lint, keeping the new server_name/keep_alive/gso fields and methods on top. Also keeps this branch's direct `mut self` mutation over the struct-literal `..self` form for with_socket/with_single_cert. #322 notes the `metrics: self.metrics` literal moves are what masked the dead field from the lint, so avoiding the pattern entirely suits its intent, and it matches the surrounding builders that already mutate in place. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
#325 landed ClientBuilder::with_server_name on main with the same field and the same connect_with_config approach this branch used, so keep main's and drop this branch's duplicate. Also drop connect_to. It was proposed as the enabler for address-family-aware DNS selection, but the existing surface already covers that: std's ToSocketAddrs parses IP literals before falling back to DNS, so connect(&addr.ip().to_string(), port) already dials a chosen address without a lookup, and #325's with_server_name supplies the name to verify. connect_to only saved a stringify-and-reparse. Taking the first DNS result is also a bug rather than a knob to route around -- binding 0.0.0.0 and resolving AAAA first fails the same way -- so the fix belongs in connect() picking an address compatible with the socket, not in new API that every caller must opt into. Tracked separately along with the implicit-bind EINVAL that CodeRabbit found on this PR. What remains is what only this branch provides: keep-alive, the GSO opt-out, and the IPv6-literal URL fix. Server-side conflicts were with #325's with_client_auth landing beside these builder methods; both are kept. tests/verify.rs returns to main's copy, since #325's tests/server_name.rs now covers SNI. Those tests dial a hostname, so the IPv6-literal path is covered by the new tests/connect.rs, whose IPv6 case fails without the fix. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
rs/web-transport-quiche/src/ez/client.rs (1)
163-191: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBind the implicit socket to the remote family.
This was flagged in a previous review round and remains unresolved. Binding implicitly to
[::]:0before resolving the remote host breaks IPv4 connections on systems where dual-stack sockets are restricted (e.g.,IPV6_V6ONLYis enabled). Defer the socket binding until after the remote address is resolved, then bind to0.0.0.0:0for IPv4 remotes and[::]:0for IPv6 remotes. Also, omitwith_bindfrom the integration tests so they genuinely verify this implicit binding behavior.
rs/web-transport-quiche/src/ez/client.rs#L163-L191: Move theself.socket.is_none()check after DNS resolution and select the bind address dynamically based onremote.is_ipv4().rs/web-transport-quiche/tests/connect.rs#L81-L87: Remove.with_bind((Ipv6Addr::LOCALHOST, 0))?to verify the implicit IPv6 bind logic.rs/web-transport-quiche/tests/connect.rs#L104-L109: Remove.with_bind((Ipv4Addr::LOCALHOST, 0))?to verify the implicit IPv4 bind logic.🛠️ Proposed fix
rs/web-transport-quiche/src/ez/client.rs- if self.socket.is_none() { - self = self.with_bind("[::]:0")?; - } - - let socket = self.socket.take().unwrap(); - let mut remotes = match tokio::net::lookup_host((host, port)).await { Ok(remotes) => remotes, Err(err) => { return Err(io::Error::new( io::ErrorKind::HostUnreachable, err.to_string(), )); } }; // Return the first entry. let remote = match remotes.next() { Some(remote) => remote, None => { return Err(io::Error::new( io::ErrorKind::HostUnreachable, "no addresses found for host", )) } }; + if self.socket.is_none() { + let bind_addr = if remote.is_ipv4() { + "0.0.0.0:0" + } else { + "[::]:0" + }; + self = self.with_bind(bind_addr)?; + } + + let socket = self.socket.take().unwrap(); + socket.connect(remote).await?;
rs/web-transport-quiche/tests/connect.rs(Lines 81-87)let session = client() - .with_bind((Ipv6Addr::LOCALHOST, 0))? .connect(url) .await .context("an IPv6 literal URL should resolve to the address it names")? .established() .await?;
rs/web-transport-quiche/tests/connect.rs(Lines 104-109)let session = client() - .with_bind((Ipv4Addr::LOCALHOST, 0))? .connect(url) .await? .established() .await?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/web-transport-quiche/src/ez/client.rs` around lines 163 - 191, Defer the implicit bind in the client connection flow until after DNS resolution, then use 0.0.0.0:0 for an IPv4 remote and [::]:0 for an IPv6 remote before socket.connect. In rs/web-transport-quiche/tests/connect.rs lines 81-87 and 104-109, remove the explicit with_bind calls so the tests exercise this family-specific implicit binding.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@rs/web-transport-quiche/src/ez/client.rs`:
- Around line 163-191: Defer the implicit bind in the client connection flow
until after DNS resolution, then use 0.0.0.0:0 for an IPv4 remote and [::]:0 for
an IPv6 remote before socket.connect. In
rs/web-transport-quiche/tests/connect.rs lines 81-87 and 104-109, remove the
explicit with_bind calls so the tests exercise this family-specific implicit
binding.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 011e0ef1-5916-4f70-8c03-3ca50bc43fa3
📒 Files selected for processing (7)
rs/web-transport-quiche/src/client.rsrs/web-transport-quiche/src/ez/client.rsrs/web-transport-quiche/src/ez/driver.rsrs/web-transport-quiche/src/ez/mod.rsrs/web-transport-quiche/src/ez/server.rsrs/web-transport-quiche/src/server.rsrs/web-transport-quiche/tests/connect.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- rs/web-transport-quiche/src/ez/mod.rs
- rs/web-transport-quiche/src/server.rs
- rs/web-transport-quiche/src/ez/driver.rs
- rs/web-transport-quiche/src/ez/server.rs
Upstream enablers for the QUIC transport controls and routing section of moq-dev/moq#2296, where
moq-nativecurrently ignores or rejects these options on the quiche backend. Both are exposed on theez(raw QUIC) and WebTransport builders, client and server.What changed
Keep-alive —
with_keep_alive(Duration)quiche has no keep-alive of its own, and
Settingsis a re-export of tokio-quiche'sQuicSettings, which has no such field. So this is built in theezdriver: a lazily-created interval that callssend_ack_eliciting()and returnsReadyso the io loop flushes the PING. quiche only adds a PING when the next packet wouldn't already be ack-eliciting, so a tick on a busy connection costs nothing. The timer is created on first poll, inside the runtime that actually drives the connection.GSO opt-out —
with_gso(bool)The client called
apply_max_capabilities()unconditionally and the server did the same when adopting a socket, so--client-quic-gso=false/--server-quic-gso=falsehad nowhere to land.SocketCapabilities::apply_all_and_get_compatibilityalways enables GSO and the flags arepub(crate), so opting out means reproducing it viaSocketCapabilitiesBuilderminusgso(). tokio-quiche's own comment (quic/mod.rs:153, "Don't apply_max_capabilities(): some NICs don't support GSO") corroborates the need.IPv6-literal URL fix (drive-by)
Url::host()renders IPv6 in bracketed URL form, soconnect("https://[::1]:443/")passed"[::1]"to the resolver and always failed DNS. Fixed by matching theHostenum. This promotesurlfrom a dev-dependency to a real one; it stays a private dependency, andurl::Urlwas already reachable viaConnectRequest.Dropped from this PR
with_server_name— feat(quiche)!: add mTLS client auth and TLS server name control #325 landed the same feature first, with the same field and the sameconnect_with_configapproach. Main's is kept.connect_to(remote)— proposed as the enabler for address-family-aware DNS selection, but the existing surface already covers it.std'sToSocketAddrsparses IP literals before falling back to DNS, soconnect(&addr.ip().to_string(), port)already dials a chosen address with no lookup, and feat(quiche)!: add mTLS client auth and TLS server name control #325'swith_server_namesupplies the name to verify.connect_toonly saved a stringify-and-reparse.Taking the first DNS result is a bug, not a knob to route around — binding
0.0.0.0and resolving AAAA first fails the same way. That fix belongs inconnect()choosing a socket-compatible address, not in new API every caller must opt into. Tracked separately with the implicit-bind EINVAL below.Reviewer notes
Server builder ordering. Caller-supplied listeners and our own sockets are held in one insertion-ordered enum, with capabilities applied at build time. This makes
with_gsoorder-independent —.with_bind(x)?.with_gso(false)would otherwise silently do nothing — without clobbering the capabilities on a listener you configured yourself, and keepslocal_addrs()in insertion order.with_gsodeliberately does not apply towith_listener, which carries its own capabilities and CID generator.Known bug, deliberately not fixed here (@coderabbitai found it):
connect()without an explicitwith_bind()binds[::]:0and then connects to an IPv4 remote, failing withInvalid argument (os error 22). I reproduced it, and confirmed it's pre-existing — base commitf0c64c1has byte-identical bind-then-connect logic. Every test in the repo binds explicitly, which is why it went unnoticed. It gets its own PR together with the DNS-selection fix above.Testing
tests/transport.rs— keep-alive and GSO.tests/connect.rs— IPv6/IPv4 literal URLs.Both regression tests are verified load-bearing: the keep-alive test fails when
send_ack_eliciting()is removed, and the IPv6 test fails against main's host extraction (failed to lookup address information) while the IPv4 case still passes. The keep-alive test is paired with a control proving an un-kept connection genuinely dies of the idle timeout.All 28 tests in the crate pass on the merge, including #325's
client_authandserver_namesuites; clippy-D warningsandcargo fmt --checkare clean.🤖 Generated with Claude Code