-
Notifications
You must be signed in to change notification settings - Fork 43
Add Incoming struct to inspect connections before accepting #155
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis pull request introduces a new public 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web-transport-quiche/src/ez/server.rs (1)
240-240:⚠️ Potential issue | 🟡 MinorChannel capacity equals the number of sockets (typically 1).
mpsc::channel(sockets.len())creates a very small buffer. Under load, many connections may complete the TLS handshake concurrently but block onaccept.send(...)waiting forServer::accept()to drain. During this wait,resume(running)hasn't been called yet, so the QUIC connection's post-handshake driver isn't running — potentially causing idle timeouts for connections that handshook successfully but aren't being serviced.Consider using a larger fixed capacity or making it configurable.
🤖 Fix all issues with AI agents
In `@web-transport-quiche/src/h3/connect.rs`:
- Around line 160-163: The caller destructures the result of
connect.into_inner() as a 2-tuple but into_inner now returns three items
(ez::SendStream, ez::RecvStream, Vec<u8>), so update the pattern where
connect.into_inner() is used (e.g. the let (_send, mut recv) =
connect.into_inner() site) to unpack all three values and either bind or ignore
the leftover buffer (e.g. use a third binding like _buf) so the code compiles
and correctly captures the leftover bytes.
🧹 Nitpick comments (3)
web-transport-quiche/src/h3/connect.rs (2)
53-69: Unbounded read loop — consider a size cap to mitigate slow-loris-style abuse.If a peer sends a never-completing CONNECT frame,
bufgrows without bound (limited only by QUIC flow-control windows). A reasonable upper-bound check (e.g. a few KB) with an error return would make this more robust against misbehaving peers.Example guard
+const MAX_CONNECT_HEADER_SIZE: usize = 16 * 1024; + let mut buf = Vec::new(); let request = loop { if recv.read_buf(&mut buf).await?.is_none() { return Err(ConnectError::UnexpectedEnd); } + if buf.len() > MAX_CONNECT_HEADER_SIZE { + return Err(ConnectError::Proto(web_transport_proto::ConnectError::UnexpectedEnd)); + } let mut cursor = std::io::Cursor::new(buf.as_slice());
113-129: Duplicate decode-loop pattern — consider extracting a helper.The buffered decode loop in
open(lines 113-129) is structurally identical to the one inaccept(lines 53-69), differing only in the type being decoded. A generic helper (e.g.async fn decode_buffered<T: Decode>(recv, buf) -> Result<T, ConnectError>) would eliminate this duplication and ensure the same size-cap guard applies to both paths.The same unbounded-growth concern from the
acceptloop applies here.web-transport-quiche/src/ez/server.rs (1)
265-285: Handshake errors are silently swallowed.When
initial.handshake(driver).await?fails, the?propagates into thetokio::spawnasync block'sResult, but since no one inspects theJoinHandle, the error is silently dropped. Adding atracing::warn!for failed handshakes would improve operational visibility.Suggested improvement
tokio::spawn(async move { let accept_bi = flume::unbounded(); let accept_uni = flume::unbounded(); let state = Lock::new(DriverState::new(true)); let driver = Driver::new(state.clone(), accept_bi.0, accept_uni.0); - // Wait for the TLS handshake to complete so ALPN is available. - let (qconn, running) = initial.handshake(driver).await?; + // Wait for the TLS handshake to complete so ALPN is available. + let (qconn, running) = match initial.handshake(driver).await { + Ok(v) => v, + Err(e) => { + tracing::debug!("TLS handshake failed: {e}"); + return Ok(()); + } + }; let connection = Connection::new(qconn, state, accept_bi.1, accept_uni.1);
| /// Returns the inner streams and any leftover bytes from reading the CONNECT handshake. | ||
| pub fn into_inner(self) -> (ez::SendStream, ez::RecvStream, Vec<u8>) { | ||
| (self.send, self.recv, self.buf) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
rg -n --type rust 'into_inner' -C 3Repository: moq-dev/web-transport
Length of output: 15632
🏁 Script executed:
cat -n web-transport-quiche/src/connection.rs | sed -n '110,120p'Repository: moq-dev/web-transport
Length of output: 527
Breaking API change on into_inner — update caller to handle 3-tuple.
into_inner now returns (SendStream, RecvStream, Vec<u8>) instead of a 2-tuple. At web-transport-quiche/src/connection.rs:115, the destructuring pattern let (_send, mut recv) = connect.into_inner(); attempts to unpack only 2 values from a 3-tuple and will fail to compile. Update to unpack all 3 values:
let (_send, mut recv, _buf) = connect.into_inner();🤖 Prompt for AI Agents
In `@web-transport-quiche/src/h3/connect.rs` around lines 160 - 163, The caller
destructures the result of connect.into_inner() as a 2-tuple but into_inner now
returns three items (ez::SendStream, ez::RecvStream, Vec<u8>), so update the
pattern where connect.into_inner() is used (e.g. the let (_send, mut recv) =
connect.into_inner() site) to unpack all three values and either bind or ignore
the leftover buffer (e.g. use a third binding like _buf) so the code compiles
and correctly captures the leftover bytes.
Adds an Incoming struct that wraps a QUIC connection before it's fully accepted. This allows inspecting the peer address before starting the post-handshake driver. The Server::accept() method now returns Incoming instead of Connection, and callers must call incoming.accept() to get the Connection. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
4b6085e to
37c5d71
Compare
Summary
Incomingstruct that wraps a QUIC connection before it's fully acceptedChanges
ez::Incomingstruct withpeer_addr()andaccept()methodsez::Server::accept()to returnIncominginstead ofConnectionincoming.accept()to get the connectionUse Case
This enables servers to inspect the peer address (and potentially other connection metadata in the future) before accepting the connection and starting the driver.
🤖 Generated with Claude Code