-
Notifications
You must be signed in to change notification settings - Fork 138
Add a builder pattern for constructing clients/servers #862
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
This refactors the Client and Server session builders to use a fluent builder pattern with with_publish() and with_consume() methods. The with_stats() methods are commented out pending the observability feature merge. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
WalkthroughThe PR refactors MoQ session setup from positional calls (e.g. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 2
🤖 Fix all issues with AI agents
In `@rs/hang/examples/video.rs`:
- Around line 30-33: The comment above the call to
client.with_publish(origin).connect(url).await? is inaccurate: it refers to
"consuming" and `OriginProducer` but this line uses `with_publish()` which
accepts an `OriginConsumer` and indicates we are only publishing (omitting
`with_consume()`). Update the comment to state that this establishes a
WebTransport/QUIC connection and MoQ handshake for publishing only, that
`with_publish(origin)` registers an OriginConsumer for outgoing data, and note
that `with_consume()` would be used if we wanted to subscribe/consume; keep the
reference to optional WebSocket fallback if desired.
In `@rs/moq-lite/src/server.rs`:
- Around line 85-101: The code wrongly decodes server.parameters instead of the
peer's parameters: change the call to use client.parameters when deriving
request_id_max so the server reads the client's MaxRequestId (use
ietf::Parameters::decode(&mut client.parameters, version)), keep the rest of the
logic that constructs request_id_max (ietf::RequestId(...)) and passes it into
ietf::start(session.clone(), stream, request_id_max, false,
self.publish.clone(), self.consume.clone(), version). This ensures the server
learns the client's maximum request ID rather than re-reading its own.
🧹 Nitpick comments (4)
rs/moq-lite/src/server.rs (1)
107-110: Consider ifsession.clone()is needed before wrapping inSession::new.On line 109,
session.clone()is called, but the session is moved into theSession::new()wrapper. SinceSession::newalready wraps the session in anArc, the clone may be unnecessary unless it's needed for thelite::start/ietf::startcalls earlier (which do usesession.clone()).If the clone is intentional for symmetry, consider adding a brief comment explaining why.
Suggested simplification
- Ok(Session::new(session.clone())) + Ok(Session::new(session))rs/moq-native/src/client.rs (2)
252-270: Review the error handling in the transport race.The
tokio::select!pattern has a subtle issue: if WebSocket is disabled (ws_handlereturnsNone), and QUIC fails with an error, theelsebranch triggers with "failed to connect to server" rather than surfacing the actual QUIC error.Consider propagating the QUIC error when WebSocket is disabled:
Suggested improvement for better error reporting
// Race the connection futures Ok(tokio::select! { Ok(quic) = quic_handle => self.moq.connect(quic).await?, Some(Ok(ws)) = ws_handle => self.moq.connect(ws).await?, // If both attempts fail, return an error - else => anyhow::bail!("failed to connect to server"), + else => { + if !self.websocket.enabled { + anyhow::bail!("QUIC connection failed (WebSocket disabled)"); + } + anyhow::bail!("failed to connect to server via QUIC or WebSocket"); + } })
339-341: Redundant enabled check.The
connect_websocketmethod is only called fromws_handlewhich already guards onself.websocket.enabled. Theanyhow::ensure!on line 340 is defensive but will never trigger in the current code flow.This is fine to keep for safety if the method might be called directly in the future.
rs/moq-native/src/server.rs (1)
248-256: Consider documenting the server cloning semantics.The
moq_lite::Serveris cloned for each incoming connection (self.moq.clone()). This means eachRequestgets its own copy of the publish/consume configuration, which is correct for the per-request builder pattern (Request::with_publish/with_consume).However, if
OriginConsumer/OriginProducercontain shared state (e.g.,Arcinternally), changes to theRequest's server won't affect other connections, which is likely the intended behavior.A brief doc comment on
Server::acceptor themoqfield clarifying that each request gets an independent copy of the server config would help future maintainers.Also applies to: 278-278, 306-317, 320-320, 330-343
- Fix comment in video.rs to accurately describe with_publish() - Fix server.rs to decode client.parameters instead of server.parameters to get the client's MaxRequestId Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary
with_publish()andwith_consume()methods for configuring sessionswith_stats()methods are commented out pending the observability feature merge fromfeat/observabilityTest plan
just checkpasses🤖 Generated with Claude Code