Subprotocol negotiation #139
Conversation
WalkthroughThis pull request transitions the web-transport-proto crate from a custom QPACK implementation to an external qpack dependency. It extends the ConnectRequest and ConnectResponse structures to include HeaderMap fields, enabling HTTP header support throughout the protocol stack. Error types are refactored to remove Clone derivations. The Client and Session APIs are updated to accept HeaderMap parameters for custom headers. A new protocol_negotation module is introduced with header constants for WebTransport subprotocol negotiation. New examples demonstrating multiprotocol support (echo and ping services) are added. The custom huffman.rs and qpack.rs modules are removed entirely in favor of the external qpack crate. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web-transport-proto/CHANGELOG.md (1)
19-32: Remove duplicate and conflicting changelog entries.The file contains duplicate header sections and conflicting version entries that appear to be a merge conflict artifact. Lines 19–32 duplicate the changelog format header (lines 3–7) and re-introduce an "Unreleased" section, breaking the file structure. Additionally, version entries are not in reverse chronological order (0.2.7 → 0.2.8 → 0.2.6 → 0.2.5 → ...), making the changelog unmaintainable.
🔎 Proposed fix: Remove duplicate content
Delete lines 19–32 entirely. The changelog should maintain a clean, single-pass structure with entries in reverse chronological order:
- Rename the repo. ([#94](https://github.com/moq-dev/web-transport/pull/94)) - Fix clippy warnings ([#91](https://github.com/moq-dev/web-transport/pull/91)) - Add support for session closed capsule ([#86](https://github.com/moq-dev/web-transport/pull/86)) -# Changelog -All notable changes to this project will be documented in this file. - -The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), -and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - -## [Unreleased] - -## [0.2.8](https://github.com/moq-dev/web-transport/compare/web-transport-proto-v0.2.7...web-transport-proto-v0.2.8) - 2025-10-17 - -### Other - -- Make traits compatible with WASM ([#107](https://github.com/moq-dev/web-transport/pull/107)) -- Check all feature combinations ([#102](https://github.com/moq-dev/web-transport/pull/102)) - ## [0.2.6](https://github.com/moq-dev/web-transport/compare/web-transport-proto-v0.2.5...web-transport-proto-v0.2.6) - 2025-05-15web-transport-proto/src/connect.rs (1)
227-244: Replaceexpect()with proper error handling.Line 238 uses
expect()for header encoding. While the risk is lower since this encodes internally-controlled data, encoding should still be fallible and return aResult.🔎 Proposed fix
Update the method signature to return
Result:- pub fn encode<B: BufMut>(&self, buf: &mut B) { + pub fn encode<B: BufMut>(&self, buf: &mut B) -> Result<(), ConnectError> { let mut headers = self.headers.clone(); headers.insert( "sec-webtransport-http3-draft", HeaderValue::from_static("draft02"), ); let headers = Header::response(self.status, headers); // Use a temporary buffer so we can compute the size. let mut tmp = Vec::new(); - headers.encode(&mut tmp).expect("Header encoding failed"); + headers.encode(&mut tmp)?; let size = VarInt::from_u32(tmp.len() as u32); Frame::HEADERS.encode(buf); size.encode(buf); buf.put_slice(&tmp); + Ok(()) }And update the
writemethod accordingly:pub async fn write<S: AsyncWrite + Unpin>(&self, stream: &mut S) -> Result<(), ConnectError> { let mut buf = BytesMut::new(); - self.encode(&mut buf); + self.encode(&mut buf)?; stream.write_all_buf(&mut buf).await?; Ok(()) }
🤖 Fix all issues with AI Agents
In @web-transport-proto/CHANGELOG.md:
- Line 10: Update the changelog entry to follow the repository's existing
pattern by adding the PR reference (e.g., "([#139](...))") and expand the text
to list the scope: addition of the qpack dependency, integration of a header map
for WebTransport sub-protocol headers, the new protocol negotiation module, and
updates to examples; keep wording concise and consistent with other entries'
style and formatting.
In @web-transport-proto/src/connect.rs:
- Around line 81-99: The uri_to_url function currently calls
Url::parse(...).expect(...), which can panic on malformed input; change this to
propagate a proper ConnectError instead: replace the expect call with handling
the Result from Url::parse and map the parse error into a ConnectError variant
(e.g., ConnectError::InvalidUrl or a new variant that includes the parse error
message), returning Err(...) on failure so uri_to_url returns Err when
Url::parse fails; keep the rest of the function (scheme/authority/path_and_query
extraction) unchanged and reference Url::parse, uri_to_url, and the ConnectError
enum when adding the mapping.
In @web-transport-quinn/Cargo.toml:
- Line 55: Update the rcgen dependency version in Cargo.toml from "0.14.0" to
"0.14.6" (i.e., change the rcgen = "0.14.0" entry to rcgen = "0.14.6") and then
run cargo update / rebuild to refresh Cargo.lock and ensure the project compiles
with the updated rcgen.
In @web-transport-quinn/examples/multiproto-server.rs:
- Around line 153-156: The code logs an incorrect command and calls
session.close(1, b"unknown command") but does not exit the surrounding loop, so
execution continues (and will likely hit accept_bi() again); update the branch
that handles the unknown command to (1) log the corrected message (change
"inccorect" → "incorrect") and (2) immediately break/return from the loop after
calling session.close() so the loop does not continue trying to accept streams;
ensure this change is applied where ping_msg is used and affects the same loop
that calls accept_bi().
In @web-transport-quinn/examples/README.md:
- Around line 17-40: Fix the minor grammar in the Multiprotocol negotiation
section of the README: locate the phrase "an dangerously insecure client setup"
and change it to "a dangerously insecure client setup" so the article agrees
with the following consonant-starting word.
- Line 19: Replace the temporary Internet-Draft archive link
"https://www.ietf.org/archive/id/draft-ietf-webtrans-http3-14.html#section-3.3"
with the stable IETF Datatracker URL
"https://datatracker.ietf.org/doc/draft-ietf-webtrans-http3/" so the README
references the datatracker page that automatically redirects to the latest
WebTransport HTTP/3 draft/RFC.
In @web-transport/src/quinn.rs:
- Line 294: Add a breaking-changes entry to CHANGELOG.md stating that Clone was
removed from the Error enum (the #[derive(Clone)] was intentionally dropped from
the Error type to align with quiche API changes where ClientError and
ServerError are not Clone), explain that downstream code relying on Error: Clone
must be updated (e.g., switch to Arc<Error> or refactor ownership), and include
a brief migration note indicating affected symbols: Error, ClientError,
ServerError.
🧹 Nitpick comments (7)
web-transport-quiche/src/h3/connect.rs (2)
62-71: Server cannot send subprotocol headers in response.The
respond()method hardcodes empty headers, preventing the server from responding with the negotiated subprotocol (e.g.,Sec-Webtransport-Subprotocol-*headers). Consider extending the method signature to accept optional headers:🔎 Proposed fix
- pub async fn respond(&mut self, status: http::StatusCode) -> Result<(), ConnectError> { + pub async fn respond( + &mut self, + status: http::StatusCode, + headers: http::HeaderMap, + ) -> Result<(), ConnectError> { let response = ConnectResponse { status, - headers: Default::default(), + headers, };
76-103: Client cannot send subprotocol offer headers in request.Similar to
respond(), theopen()method hardcodes empty headers, so the client cannot offer subprotocols during the CONNECT handshake. Consider accepting an optionalHeaderMapparameter:🔎 Proposed fix
- pub async fn open(conn: &ez::Connection, url: Url) -> Result<Self, ConnectError> { + pub async fn open( + conn: &ez::Connection, + url: Url, + headers: http::HeaderMap, + ) -> Result<Self, ConnectError> { tracing::debug!("opening bi"); // Create a new stream that will be used to send the CONNECT frame. let (mut send, mut recv) = conn.open_bi().await?; // Create a new CONNECT request that we'll send using HTTP/3 let request = ConnectRequest { url, - headers: Default::default(), + headers, };If full subprotocol negotiation support for quiche is out of scope for this PR, consider adding a TODO comment to track the gap.
web-transport-quinn/examples/multiproto-client.rs (1)
36-42: Optional: Simplify HeaderMap construction.The current approach is correct but could be more concise.
🔎 Proposed simplification
- let headers = HeaderMap::from_iter( - [( - protocol_negotation::AVAILABLE_NAME, - HeaderValue::from_str(args.protocol.as_str())?, - )] - .into_iter(), - ); + let mut headers = HeaderMap::new(); + headers.insert( + protocol_negotation::AVAILABLE_NAME, + HeaderValue::from_str(&args.protocol)? + );web-transport-quinn/src/client.rs (1)
216-216: Breaking API change:connectnow requiresheadersparameter.This is a breaking change for all existing callers. Consider adding a convenience method for callers that don't need custom headers:
🔎 Suggested convenience method
/// Connect to the server without custom headers. pub async fn connect(&self, url: Url) -> Result<Session, ClientError> { self.connect_with_headers(url, HeaderMap::new()).await } /// Connect to the server with custom headers for subprotocol negotiation. pub async fn connect_with_headers(&self, url: Url, headers: HeaderMap) -> Result<Session, ClientError> { // ... existing implementation }This would maintain backward compatibility while still exposing the new functionality.
web-transport-quinn/src/server.rs (1)
189-200: Doc comment doesn't describe the subprotocol parameter.The doc comment is identical to
ok()but doesn't explain theselected_protocolparameter or its purpose in subprotocol negotiation.🔎 Suggested doc improvement
- /// Accept the session, returning a 200 OK. + /// Accept the session with a selected subprotocol, returning a 200 OK. + /// + /// The `selected_protocol` will be sent in the `wt-protocol` response header + /// to indicate which subprotocol was negotiated. pub async fn ok_with_subprotocol(web-transport-quinn/src/connect.rs (1)
33-35: Consider using accessor methods instead of public fields.Exposing
requestandresponseas public fields (pub request,pub response) allows direct mutation of internal state. Accessor methods would provide better encapsulation.🔎 Suggested accessor approach
- pub request: ConnectRequest, - pub response: Option<ConnectResponse>, + request: ConnectRequest, + response: Option<ConnectResponse>, // Add accessor methods: + pub fn request(&self) -> &ConnectRequest { + &self.request + } + + pub fn response(&self) -> Option<&ConnectResponse> { + self.response.as_ref() + }This would require updating
server.rsline 178-180 to useself.connect.request().headers.get_all(...).web-transport-quinn/src/session.rs (1)
124-127: Consider returningOption<&str>to avoid allocation on each call.The
protocol()method clones the String on every call. Returning a reference would be more efficient for callers that only need to inspect the value.🔎 Suggested change
/// The webtransport subprotocol that was negotiated with the server, if any - pub fn protocol(&self) -> Option<String> { - self.protocol.clone() + pub fn protocol(&self) -> Option<&str> { + self.protocol.as_deref() }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
web-transport-proto/CHANGELOG.mdweb-transport-proto/Cargo.tomlweb-transport-proto/src/connect.rsweb-transport-proto/src/huffman.rsweb-transport-proto/src/lib.rsweb-transport-proto/src/qpack.rsweb-transport-quiche/src/client.rsweb-transport-quiche/src/h3/connect.rsweb-transport-quiche/src/server.rsweb-transport-quinn/Cargo.tomlweb-transport-quinn/examples/README.mdweb-transport-quinn/examples/multiproto-client.rsweb-transport-quinn/examples/multiproto-server.rsweb-transport-quinn/src/client.rsweb-transport-quinn/src/connect.rsweb-transport-quinn/src/error.rsweb-transport-quinn/src/lib.rsweb-transport-quinn/src/server.rsweb-transport-quinn/src/session.rsweb-transport/src/quinn.rs
💤 Files with no reviewable changes (3)
- web-transport-proto/src/huffman.rs
- web-transport-proto/src/qpack.rs
- web-transport-proto/src/lib.rs
🧰 Additional context used
🧬 Code graph analysis (4)
web-transport-quinn/examples/multiproto-client.rs (3)
web-transport-quinn/src/connect.rs (1)
url(128-130)web-transport-quinn/src/session.rs (4)
url(315-317)new(58-103)new(368-390)protocol(125-127)web-transport-quinn/src/client.rs (4)
default(162-164)default(257-259)new(41-46)new(211-213)
web-transport-quinn/src/session.rs (2)
web-transport-quinn/src/connect.rs (2)
url(128-130)open_with_headers(89-117)web-transport-quinn/src/client.rs (1)
connect(216-252)
web-transport-quinn/src/connect.rs (2)
web-transport-quiche/src/h3/connect.rs (1)
url(112-114)web-transport-quiche/src/h3/request.rs (1)
url(30-32)
web-transport-proto/src/connect.rs (4)
web-transport-quiche/src/h3/connect.rs (1)
url(112-114)web-transport-quinn/src/connect.rs (1)
url(128-130)web-transport-quinn/src/session.rs (1)
url(315-317)web-transport-wasm/src/session.rs (1)
url(122-124)
🔇 Additional comments (29)
web-transport-quiche/src/h3/connect.rs (1)
9-25: RemovingCloneis reasonable, but note the breaking change.The contained error types (
ez::ConnectionError,ez::StreamError) likely don't implementClone, making this fix necessary. However, this is a breaking change for any downstream code that was cloningConnectError. If this crate follows semver pre-1.0 conventions, this is acceptable.web-transport-quinn/examples/README.md (1)
17-40: Excellent documentation of the new multiprotocol negotiation feature.The new section clearly explains the subprotocol negotiation example with step-by-step instructions, expected outputs, and a link to the relevant specification. The examples for
echo/0andping/0protocols are well-documented and align with the PR objectives. This will help users understand how to implement protocol negotiation in their own applications.web-transport/src/quinn.rs (1)
51-51: Good: Headers parameter enables protocol negotiation.The addition of
Default::default()as a parameter aligns with the PR's protocol negotiation feature. This provides a backward-compatible default while allowing the underlying API to accept custom headers for subprotocol negotiation.web-transport-quinn/examples/multiproto-client.rs (5)
1-19: LGTM: Well-structured CLI interface.The argument structure is clear with appropriate defaults and helpful documentation comments.
29-31: Certificate verification disabled - appropriate for example only.Using
with_no_certificate_verification()is suitable for this example client connecting to a local server with self-signed certificates. Ensure users understand this should not be used in production code.
46-61: Good: Robust protocol negotiation validation.The protocol validation correctly handles both missing and mismatched protocol responses from the server, with appropriate logging and cleanup.
82-83: LGTM: Proper session cleanup.The session is closed gracefully and the code waits for closure confirmation, ensuring proper resource cleanup.
79-79: Theread_to_end(1024)call is safe and will not silently truncate responses. If the response exceeds 1024 bytes, the call fails withReadToEndError::TooLonginstead of truncating. This is the correct behavior. For example code with simple echo/ping messages, a 1024 byte limit is reasonable.web-transport-quiche/src/server.rs (1)
10-10: This review comment is incorrect. TheServerErrorenum was introduced withoutClonein the derive macro; it was never publicly derived in any version of the codebase. There is no breaking change here, and no code in the repository depends on cloningServerError. Removing a trait that was never present cannot be a breaking change.Likely an incorrect or invalid review comment.
web-transport-quinn/src/error.rs (2)
221-222: LGTM!The error message now includes the inner
ConnectErrordetails, improving debuggability.
8-8: This is a newly created error module, not a breaking change to existing types.The file
web-transport-quinn/src/error.rswas created in this commit, not modified from existing code.ClientErrorandServerErrornever previously derivedClone—they were not present before this change. While other error types in the same file (e.g.,SessionError,WebTransportError) do deriveClone, the absence ofCloneonClientErrorandServerErroris a deliberate design choice in the new module, not a removal of existing functionality.Likely an incorrect or invalid review comment.
web-transport-quinn/examples/multiproto-server.rs (2)
111-139: LGTM!The echo protocol implementation correctly handles both bidirectional streams and datagrams using
tokio::select!.
67-109: LGTM!The protocol negotiation logic correctly iterates through offered subprotocols, selects the first supported one, and gracefully rejects unsupported requests with
BAD_REQUEST.web-transport-quinn/src/server.rs (2)
96-102: LGTM!The
Derefimplementation provides ergonomic access to the underlyingquinn::Endpoint, consistent with theSessiontype'sDereftoquinn::Connection.
176-181: LGTM!Correctly exposes the available subprotocols using
GetAllto handle the case where multiple protocols may be offered.web-transport-quinn/src/connect.rs (2)
72-87: LGTM!The
AlreadyRespondedguard properly prevents double responses, and the response is stored only after successful transmission.
89-117: LGTM!The
open_with_headersfunction correctly handles the client-side CONNECT flow with custom headers, validates the response status, and stores the response for later protocol extraction.web-transport-quinn/src/session.rs (2)
85-90: LGTM!The protocol extraction correctly handles the optional response and gracefully handles non-UTF8 header values via
to_str().ok().
131-147: LGTM!The
connectmethod correctly propagates headers toConnect::open_with_headersfor subprotocol negotiation support.web-transport-proto/src/connect.rs (9)
1-13: LGTM!The import additions appropriately support the QPACK integration and header handling functionality.
18-73: LGTM!The error enum refactoring is well-structured with comprehensive error variants and appropriate use of Arc for the io::Error to avoid Clone requirement.
75-79: LGTM!Adding the
headersfield enables WebTransport subprotocol negotiation as specified in the draft 14 spec.
101-112: LGTM!The
url_to_uriconversion handles errors properly using theResultreturn type.
157-177: LGTM!The encode method now properly returns a
Result, enabling error propagation for QPACK encoding failures.
179-184: LGTM!The write method now properly propagates encoding errors.
187-191: LGTM!Adding the
headersfield enables response header support for subprotocol negotiation.
194-209: LGTM!The decode method properly validates the response status and extracts headers.
115-139: No changes needed. The usage ofqpack::decode_stateless(&mut data, u64::MAX)on line 123 is correct according to the qpack 0.1.0 API. The second parameter (max_size: u64) specifies the maximum allowed decoded size, and usingu64::MAXmeans no practical size limit is enforced, which aligns with the code comment explaining that the buffer is known to be large enough.web-transport-proto/Cargo.toml (1)
19-19: qpack dependency verification complete.The qpack v0.1.0 dependency is confirmed to exist on crates.io as the latest published version (owned by gnunicorn). No known security vulnerabilities or RustSec advisories were found for this crate. The dependency is safe to use.
|
|
||
| ## [Unreleased] | ||
|
|
||
| - Switch to full qpack support to allow for webtransport sub-protocol header negotiation |
There was a problem hiding this comment.
Add PR reference and expand changelog entry for clarity.
The new entry should follow the established pattern used in other entries by including the PR reference number (e.g., ([#139](...))). Additionally, consider expanding the entry to reflect the scope of changes mentioned in the PR objectives: the qpack dependency addition, header map integration, the new protocol negotiation module, and example updates.
🔎 Proposed enhancement
-- Switch to full qpack support to allow for webtransport sub-protocol header negotiation
+- Switch to full qpack support to enable webtransport sub-protocol header negotiation ([#139](https://github.com/moq-dev/web-transport/pull/139))Consider expanding with more detail:
+- Switch to full qpack support to enable webtransport sub-protocol header negotiation. Add HeaderMap to connect request/response, implement protocol negotiation module, and add multiprotocol examples ([#139](https://github.com/moq-dev/web-transport/pull/139))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - Switch to full qpack support to allow for webtransport sub-protocol header negotiation | |
| - Switch to full qpack support to enable webtransport sub-protocol header negotiation ([#139](https://github.com/moq-dev/web-transport/pull/139)) |
| - Switch to full qpack support to allow for webtransport sub-protocol header negotiation | |
| - Switch to full qpack support to enable webtransport sub-protocol header negotiation. Add HeaderMap to connect request/response, implement protocol negotiation module, and add multiprotocol examples ([#139](https://github.com/moq-dev/web-transport/pull/139)) |
🤖 Prompt for AI Agents
In @web-transport-proto/CHANGELOG.md at line 10, Update the changelog entry to
follow the repository's existing pattern by adding the PR reference (e.g.,
"([#139](...))") and expand the text to list the scope: addition of the qpack
dependency, integration of a header map for WebTransport sub-protocol headers,
the new protocol negotiation module, and updates to examples; keep wording
concise and consistent with other entries' style and formatting.
| fn uri_to_url(uri: Uri) -> Result<Url, ConnectError> { | ||
| let Parts { | ||
| scheme, | ||
| authority, | ||
| path_and_query, | ||
| .. | ||
| } = uri.into_parts(); | ||
| let scheme = scheme.unwrap_or(Scheme::HTTPS); | ||
| let Some(authority) = authority else { | ||
| return Err(ConnectError::WrongAuthority); | ||
| }; | ||
| let Some(path_and_query) = path_and_query else { | ||
| return Err(ConnectError::WrongPath); | ||
| }; | ||
| Ok( | ||
| Url::parse(&format!("{scheme}://{authority}{path_and_query}")) | ||
| .expect("Failed to parse URL"), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Replace expect() with proper error handling.
Line 97 uses expect() which can panic if URL parsing fails. Malformed network input could trigger this panic. Return a proper error instead.
🔎 Proposed fix
- Ok(
- Url::parse(&format!("{scheme}://{authority}{path_and_query}"))
- .expect("Failed to parse URL"),
- )
+ Url::parse(&format!("{scheme}://{authority}{path_and_query}"))
+ .map_err(|_| ConnectError::WrongPath)🤖 Prompt for AI Agents
In @web-transport-proto/src/connect.rs around lines 81 - 99, The uri_to_url
function currently calls Url::parse(...).expect(...), which can panic on
malformed input; change this to propagate a proper ConnectError instead: replace
the expect call with handling the Result from Url::parse and map the parse error
into a ConnectError variant (e.g., ConnectError::InvalidUrl or a new variant
that includes the parse error message), returning Err(...) on failure so
uri_to_url returns Err when Url::parse fails; keep the rest of the function
(scheme/authority/path_and_query extraction) unchanged and reference Url::parse,
uri_to_url, and the ConnectError enum when adding the mapping.
There was a problem hiding this comment.
Yeah this is spooky, just return an error. Or better yet, remove the need to convert between both of these crates.
There was a problem hiding this comment.
@kixelated, thanks for confirming! The Url::parse method returns Result<Url, ParseError>, so you can map the ParseError to one of the existing ConnectError variants. The suggested fix using map_err(|_| ConnectError::WrongPath) would work, or you could add a new variant like InvalidUrl(url::ParseError) if you want to preserve the specific parse error details for debugging.
/)/)
(˶•̀ ᴗ •́˶)
/づ🔧 Safety first!
|
|
||
| /// An error returned when connecting to a WebTransport endpoint. | ||
| #[derive(thiserror::Error, Debug, Clone)] | ||
| #[derive(thiserror::Error, Debug)] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let me check the current state of ClientError definition in client.rs
fd client.rs | head -1 | xargs cat -n | head -20Repository: moq-dev/web-transport
Length of output: 700
🏁 Script executed:
# Search for ClientError definition and Clone derives
rg -n "ClientError" web-transport-quiche/src/client.rs -B2 -A5 | head -50Repository: moq-dev/web-transport
Length of output: 1879
🏁 Script executed:
# Execute the provided search script for clone usage
rg -nP --type=rust 'ClientError.*\.clone\(\)|\.clone\(\).*ClientError' -C3Repository: moq-dev/web-transport
Length of output: 47
🏁 Script executed:
# Also search for Clone bounds or trait requirements on ClientError
rg -nP --type=rust 'ClientError.*Clone|Clone.*ClientError' -C3Repository: moq-dev/web-transport
Length of output: 47
🏁 Script executed:
# Compare ServerError in server.rs
rg -n "ServerError" web-transport-quiche/src/server.rs -B2 -A5 | head -50Repository: moq-dev/web-transport
Length of output: 1009
Breaking change: Clone removed from public error type.
Removing Clone from ClientError mirrors the state of ServerError (both now only derive thiserror::Error and Debug). This is a breaking change for external code depending on cloning errors; ensure this is documented in release notes and that any client code using cloned ClientError instances is updated.
| env_logger = "0.11" | ||
| rustls-pemfile = "2" | ||
| tokio = { version = "1", features = ["full"] } | ||
| rcgen = "0.14.0" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
rcgen Rust crate latest version crates.io
💡 Result:
The latest rcgen crate version is 0.14.6 — released December 13, 2025. [1][2]
Sources:
- rcgen 0.14.6 documentation (docs.rs). [1]
- rcgen version history (cargo-run). [2]
Update rcgen to the latest stable version 0.14.6.
The rcgen crate version 0.14.0 is outdated; the latest stable release is 0.14.6 (December 2025). Update the dependency to benefit from recent bug fixes and improvements.
🤖 Prompt for AI Agents
In @web-transport-quinn/Cargo.toml at line 55, Update the rcgen dependency
version in Cargo.toml from "0.14.0" to "0.14.6" (i.e., change the rcgen =
"0.14.0" entry to rcgen = "0.14.6") and then run cargo update / rebuild to
refresh Cargo.lock and ensure the project compiles with the updated rcgen.
| } else { | ||
| log::info!("inccorect command, {}. closing sessions", ping_msg); | ||
| session.close(1, b"unknown command"); | ||
| } |
There was a problem hiding this comment.
Missing return after session.close() causes continued loop execution.
After closing the session with an error, the loop continues and will attempt accept_bi() again, likely causing an error. Also, there's a typo: "inccorect" → "incorrect".
🔎 Proposed fix
} else {
- log::info!("inccorect command, {}. closing sessions", ping_msg);
+ log::info!("incorrect command: {}. closing session", ping_msg);
session.close(1, b"unknown command");
+ return Ok(());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else { | |
| log::info!("inccorect command, {}. closing sessions", ping_msg); | |
| session.close(1, b"unknown command"); | |
| } | |
| } else { | |
| log::info!("incorrect command: {}. closing session", ping_msg); | |
| session.close(1, b"unknown command"); | |
| return Ok(()); | |
| } |
🤖 Prompt for AI Agents
In @web-transport-quinn/examples/multiproto-server.rs around lines 153 - 156,
The code logs an incorrect command and calls session.close(1, b"unknown
command") but does not exit the surrounding loop, so execution continues (and
will likely hit accept_bi() again); update the branch that handles the unknown
command to (1) log the corrected message (change "inccorect" → "incorrect") and
(2) immediately break/return from the loop after calling session.close() so the
loop does not continue trying to accept streams; ensure this change is applied
where ping_msg is used and affects the same loop that calls accept_bi().
| ## Multiprotocol negotiation | ||
|
|
||
| There is another example that shows how to implement webtransport sub protocol negotiation to support several protocols, in accordance with [draft 14 of the WebTransport specification](https://www.ietf.org/archive/id/draft-ietf-webtrans-http3-14.html#section-3.3). | ||
|
|
||
| This example uses a self-signed certificate and an dangerously insecure client setup for easy bootstrapping. Don't use that at home! | ||
|
|
||
| To try the example, run the server (it uses the port `56789` by default) from the root: | ||
| ``` | ||
| cargo run --example multiproto-server | ||
| ``` | ||
|
|
||
| Once that is up, you can try the client with the different protocols in a separate terminal: | ||
|
|
||
| for the `echo` protocol run: | ||
| ``` | ||
| cargo run --example multiproto-client -- --protocol echo/0 "any test string" | ||
| ``` | ||
| You should see the server sending back the same string. | ||
|
|
||
| To run the `ping` protocol run: | ||
| ``` | ||
| cargo run --example multiproto-client -- --protocol ping/0 ping | ||
| ``` | ||
| You will see the server sending back a `ack` response. |
There was a problem hiding this comment.
Minor grammatical fix needed.
Line 21 contains a grammatical error: "an dangerously insecure" should be "a dangerously insecure".
🔎 Proposed fix
-This example uses a self-signed certificate and an dangerously insecure client setup for easy bootstrapping. Don't use that at home!
+This example uses a self-signed certificate and a dangerously insecure client setup for easy bootstrapping. Don't use that at home!🤖 Prompt for AI Agents
In @web-transport-quinn/examples/README.md around lines 17 - 40, Fix the minor
grammar in the Multiprotocol negotiation section of the README: locate the
phrase "an dangerously insecure client setup" and change it to "a dangerously
insecure client setup" so the article agrees with the following
consonant-starting word.
|
|
||
| ## Multiprotocol negotiation | ||
|
|
||
| There is another example that shows how to implement webtransport sub protocol negotiation to support several protocols, in accordance with [draft 14 of the WebTransport specification](https://www.ietf.org/archive/id/draft-ietf-webtrans-http3-14.html#section-3.3). |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
WebTransport specification IETF draft latest version RFC status
💡 Result:
As of January 6, 2026:
-
Current IETF working-group drafts:
- The WebTransport Protocol Framework: draft-ietf-webtrans-overview-11 (updated 20 Oct 2025). [1]
- WebTransport over HTTP/3: draft-ietf-webtrans-http3-14 (updated 20 Oct 2025). [2]
- WebTransport over HTTP/2: draft-ietf-webtrans-http2-13 (WG listing). [3]
-
RFC status: these are active Internet‑Drafts (WG documents) and have not been published as RFCs. [1][2][3]
Sources: IETF Datatracker entries for the WebTransport WG and the individual drafts. [1][2][3]
🌐 Web query:
https://www.ietf.org/archive/id/draft-ietf-webtrans-http3-14.html validity
💡 Result:
The URL is an active IETF Internet‑Draft (draft-ietf-webtrans-http3-14): published 20 October 2025 and expires 23 April 2026. Internet‑Drafts are working documents valid for a maximum of six months and are not final RFCs (i.e., "work in progress"). [1][2]
Sources: IETF draft page and IETF Datatracker. [1] https://www.ietf.org/archive/id/draft-ietf-webtrans-http3-14.html [2] https://datatracker.ietf.org/doc/draft-ietf-webtrans-http3/
Link references a temporary Internet-Draft; use the stable IETF Datatracker instead.
The URL points to draft-ietf-webtrans-http3-14, which is an active Internet-Draft that expires on 23 April 2026. When the draft expires, the direct archive link will become outdated. Instead, link to the stable IETF Datatracker page: https://datatracker.ietf.org/doc/draft-ietf-webtrans-http3/, which automatically forwards to the latest version of the WebTransport HTTP/3 specification and will remain valid as the specification evolves or is published as an RFC.
🤖 Prompt for AI Agents
In @web-transport-quinn/examples/README.md at line 19, Replace the temporary
Internet-Draft archive link
"https://www.ietf.org/archive/id/draft-ietf-webtrans-http3-14.html#section-3.3"
with the stable IETF Datatracker URL
"https://datatracker.ietf.org/doc/draft-ietf-webtrans-http3/" so the README
references the datatracker page that automatically redirects to the latest
WebTransport HTTP/3 draft/RFC.
| pub mod protocol_negotation { | ||
| /// WebTransport sub-protocol negotiation, | ||
| /// | ||
| /// according to [draft 14](https://www.ietf.org/archive/id/draft-ietf-webtrans-http3-14.html#section-3.3) | ||
| use http::HeaderName; | ||
|
|
||
| /// The header name for the available protocols, sent within the WebTransport Connect request. | ||
| pub const AVAILABLE_NAME: HeaderName = HeaderName::from_static("wt-available-protocols"); | ||
| /// The header name for the selected protocol, sent within the WebTransport Connect response. | ||
| pub const SELECTED_NAME: HeaderName = HeaderName::from_static("wt-protocol"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the actual file content to confirm the typo
find . -name "lib.rs" -path "*/web-transport-quinn/*" | head -5Repository: moq-dev/web-transport
Length of output: 97
🏁 Script executed:
# Search for references to protocol_negotation to see what needs to be updated
rg "protocol_negotation" --type rustRepository: moq-dev/web-transport
Length of output: 860
🌐 Web query:
WebTransport HTTP/3 draft 14 wt-available-protocols wt-protocol header names specification
💡 Result:
Summary (from draft-ietf-webtrans-http3-14):
- Header names: WT-Available-Protocols (client request) and WT-Protocol (server 2xx response). [1]
- Structured-field types: WT-Available-Protocols is a Structured Fields List; WT-Protocol is a Structured Fields Item. Only string values are valid; other types must cause the entire field to be ignored. Parameters on these fields are ignored. [1]
- Semantics: WT-Available-Protocols lists protocols in preference order. If present, the server MAY return WT-Protocol with a single choice that MUST be one of the client's listed values; otherwise WT-Protocol must be ignored. Servers may reject the request if no suitable protocol is offered. The protocol tokens/strings need not be registered in IANA ALPN registry; semantics are determined by the WebTransport resource. [1]
- Usage in Web APIs: The WebTransport client/API sets WT-Available-Protocols from the user-facing protocols list when creating a CONNECT/webtransport request. See the WebTransport spec for the client-side behavior. [2]
Sources:
[1] draft-ietf-webtrans-http3-14 (WebTransport over HTTP/3), Section 3.3.
[2] W3C WebTransport API (client behavior for WT-Available-Protocols).
Typo in module name: protocol_negotation → protocol_negotiation. Since this is a public API, this typo will become a breaking change once released.
Additionally, header names must match the specification's prescribed format. WebTransport HTTP/3 draft 14 defines the canonical forms as WT-Available-Protocols and WT-Protocol (title case), but the code uses lowercase wt-available-protocols and wt-protocol. While HTTP headers are case-insensitive, spec compliance requires matching the prescribed format.
Fix required in:
web-transport-quinn/src/lib.rs: Module name and header constant valuesweb-transport-quinn/src/server.rs: Module import and usagesweb-transport-quinn/src/session.rs: Module import and usageweb-transport-quinn/examples/multiproto-client.rs: Module import and usage
| /// The source can either be a session error or a stream error. | ||
| /// TODO This interface is currently not generic. | ||
| #[derive(Debug, thiserror::Error, Clone)] | ||
| #[derive(Debug, thiserror::Error)] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's look at the Error enum definition in quinn.rs around line 294
wc -l web-transport/src/quinn.rsRepository: moq-dev/web-transport
Length of output: 95
🏁 Script executed:
#!/bin/bash
# Read the Error enum definition with context around line 294
sed -n '280,310p' web-transport/src/quinn.rsRepository: moq-dev/web-transport
Length of output: 924
🏁 Script executed:
#!/bin/bash
# Search for .clone() calls on Error instances in web-transport
rg -nP --type=rust 'Error.*\.clone\(\)|\bError\b.*\.clone\(\)' web-transport/src/ -C 3Repository: moq-dev/web-transport
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Also search more broadly for cloning patterns with Error
rg -nP --type=rust '\.clone\(\)' web-transport/src/quinn.rs -B 2 -A 2Repository: moq-dev/web-transport
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check for Clone trait derives on Error enum
rg -nP --type=rust '#\[derive.*Clone.*\]|Error.*enum|pub.*enum.*Error' web-transport/src/quinn.rs -C 2Repository: moq-dev/web-transport
Length of output: 625
🏁 Script executed:
#!/bin/bash
# Look at quiche module for similar error types pattern
fd -e rs quiche web-transport/src/ -x grep -l "ClientError\|ServerError" {}Repository: moq-dev/web-transport
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check git log for changes to Error enum definition
git log --oneline -n 20 -- web-transport/src/quinn.rs | head -20Repository: moq-dev/web-transport
Length of output: 101
🏁 Script executed:
#!/bin/bash
# Check if there was a previous version with Clone on Error
git log -p -- web-transport/src/quinn.rs | grep -A 10 "pub enum Error" | head -50Repository: moq-dev/web-transport
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check quinn types to see if they implement Clone
# First find where quinn types are used
rg -nP --type=rust 'quinn::SessionError|quinn::ServerError|quinn::ClientError|quinn::WriteError|quinn::ReadError' web-transport/src/ | head -20Repository: moq-dev/web-transport
Length of output: 995
🏁 Script executed:
#!/bin/bash
# Look for any CHANGELOG or breaking changes documentation
fd -e md -e txt -name "*CHANGE*" -o -name "*change*" -o -name "*BREAK*" web-transport/ | head -20Repository: moq-dev/web-transport
Length of output: 295
🏁 Script executed:
#!/bin/bash
# Check if the quiche module referenced in the comment exists
find web-transport -name "*.rs" -type f | xargs grep -l "quiche" | head -10Repository: moq-dev/web-transport
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check the full git history of the Error enum to see if Clone was ever present
git log --all -p -- web-transport/src/quinn.rs | grep -B 5 -A 15 "#\[derive.*Error\]" | head -100Repository: moq-dev/web-transport
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check if the commit that "updated quiche to API changes" modified Error
git show 1db7302:web-transport/src/quinn.rs | grep -A 20 "pub enum Error" 2>/dev/null || echo "File might not exist in that commit"Repository: moq-dev/web-transport
Length of output: 608
🏁 Script executed:
#!/bin/bash
# Check current HEAD and previous commits for Error definition
git log --oneline --all -- web-transport/src/quinn.rs | head -10Repository: moq-dev/web-transport
Length of output: 626
🏁 Script executed:
#!/bin/bash
# Let's see the actual diff of that commit
git show 1db7302 --stat | head -30Repository: moq-dev/web-transport
Length of output: 1717
🏁 Script executed:
#!/bin/bash
# Search for any Clone derive on Error across git history
git log --all -S "#[derive(Clone" -- web-transport/src/quinn.rsRepository: moq-dev/web-transport
Length of output: 816
🏁 Script executed:
#!/bin/bash
# Check what Clone looks like in the git history before the recent change
# Look at the previous commits for Error enum
git show 625a54b:web-transport/src/quinn.rs 2>/dev/null | grep -A 20 "pub enum Error" || echo "File may not have existed"Repository: moq-dev/web-transport
Length of output: 583
🏁 Script executed:
#!/bin/bash
# Let's see what changed between the last two commits of quinn.rs
git show 1db7302:web-transport/src/quinn.rs > /tmp/current_quinn.rs
git show 1db7302^:web-transport/src/quinn.rs > /tmp/prev_quinn.rs 2>/dev/null
diff -u /tmp/prev_quinn.rs /tmp/current_quinn.rs | grep -A 10 -B 10 "enum Error" || echo "No Error enum changes or file didn't exist in previous commit"Repository: moq-dev/web-transport
Length of output: 607
🏁 Script executed:
#!/bin/bash
# Check if there's a web-transport-quiche directory with error types
find . -path "*/quiche*" -name "*.rs" -type f 2>/dev/null | head -20Repository: moq-dev/web-transport
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Look for any documentation about this being a breaking change
rg -i "breaking.*change|clone.*removed" web-transport/ -t md -t txt 2>/dev/nullRepository: moq-dev/web-transport
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check the Cargo.toml to understand quinn dependency
grep -A 5 "quinn" web-transport/Cargo.toml 2>/dev/null | head -20Repository: moq-dev/web-transport
Length of output: 519
🏁 Script executed:
#!/bin/bash
# Let's verify if there's a separate web-transport-quiche package
ls -la web-transport* 2>/dev/null | head -20Repository: moq-dev/web-transport
Length of output: 837
🏁 Script executed:
#!/bin/bash
# Check if Error type had Clone in any earlier commits
git log --all --oneline -- web-transport/src/quinn.rs | while read commit msg; do
commit_hash=$(echo "$commit" | cut -d' ' -f1)
git show "$commit_hash:web-transport/src/quinn.rs" 2>/dev/null | grep -A 2 "pub enum Error" | grep -i clone && echo "Found Clone at $commit_hash: $msg"
doneRepository: moq-dev/web-transport
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Double-check: does Error enum have Clone anywhere in current codebase?
grep -r "impl Clone for Error\|enum Error {" web-transport/src/quinn.rs -A 5 | grep -i cloneRepository: moq-dev/web-transport
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check the web-transport-quiche error types mentioned in the review
find web-transport-quiche -name "*.rs" -type f -exec grep -l "ClientError\|ServerError" {} \;Repository: moq-dev/web-transport
Length of output: 212
🏁 Script executed:
#!/bin/bash
# Look at error types in web-transport-quiche
cat web-transport-quiche/src/*.rs | grep -A 10 "enum.*Error\|struct.*Error" | head -50Repository: moq-dev/web-transport
Length of output: 1111
🏁 Script executed:
#!/bin/bash
# Let's check the previous commit state more carefully
git show 1db7302^:web-transport/src/quinn.rs 2>/dev/null | head -400 | tail -50Repository: moq-dev/web-transport
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check all Error enum definitions in the codebase
find . -name "*.rs" -type f -exec grep -l "enum Error" {} \; | head -20Repository: moq-dev/web-transport
Length of output: 159
🏁 Script executed:
#!/bin/bash
# Check if quiche error types have Clone derived
grep -A 5 "pub enum ClientError\|pub enum ServerError" web-transport-quiche/src/*.rs | grep -i cloneRepository: moq-dev/web-transport
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Let's verify the exact change in the latest commit for Error enum
git diff 1db7302^ 1db7302 -- web-transport/src/quinn.rs | grep -A 15 -B 5 "enum Error"Repository: moq-dev/web-transport
Length of output: 95
🏁 Script executed:
#!/bin/bash
# Check if Error ever had Clone in any historical version
git log --all -p -- web-transport/src/quinn.rs | grep -B 2 -A 8 "pub enum Error" | grep -i "derive.*clone\|clone.*derive" | head -5Repository: moq-dev/web-transport
Length of output: 193
🏁 Script executed:
#!/bin/bash
# Let's see the actual full diff for Error enum changes in the recent commit
git log -p --all -- web-transport/src/quinn.rs | grep -B 10 -A 10 "derive.*thiserror.*Clone" | head -80Repository: moq-dev/web-transport
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check the current state again to confirm Clone is not present
sed -n '294,296p' web-transport/src/quinn.rsRepository: moq-dev/web-transport
Length of output: 151
🏁 Script executed:
#!/bin/bash
# Get more context on recent changes to Error
git log --oneline -n 5 -- web-transport/src/quinn.rsRepository: moq-dev/web-transport
Length of output: 101
🏁 Script executed:
#!/bin/bash
# Let's see if the latest commit message explains the change
git show --no-patch 1db7302 | head -30Repository: moq-dev/web-transport
Length of output: 230
🏁 Script executed:
#!/bin/bash
# Check if quinne error types (SessionError, ServerError, ClientError) implement Clone
rg -A 3 "derive.*Debug.*thiserror" web-transport-quiche/src/ --type rustRepository: moq-dev/web-transport
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check CHANGELOG for information about this change
cat web-transport/CHANGELOG.md | head -100Repository: moq-dev/web-transport
Length of output: 3814
🏁 Script executed:
#!/bin/bash
# Let's look at the exact git show output for the commit to see all changes
git show 1db7302 | head -200Repository: moq-dev/web-transport
Length of output: 5498
Document breaking change in CHANGELOG: Clone was removed from the Error enum to align with quiche's API changes (ClientError and ServerError also lack Clone). Add an entry to CHANGELOG.md under a breaking changes section to notify downstream users of this removal.
🤖 Prompt for AI Agents
In @web-transport/src/quinn.rs at line 294, Add a breaking-changes entry to
CHANGELOG.md stating that Clone was removed from the Error enum (the
#[derive(Clone)] was intentionally dropped from the Error type to align with
quiche API changes where ClientError and ServerError are not Clone), explain
that downstream code relying on Error: Clone must be updated (e.g., switch to
Arc<Error> or refactor ownership), and include a brief migration note indicating
affected symbols: Error, ClientError, ServerError.
gnunicorn
left a comment
gnunicorn
left a comment
There was a problem hiding this comment.
just comments for context
|
|
||
| // Errors that can occur during the connect request. | ||
| #[derive(Error, Debug, Clone)] | ||
| #[derive(Error, Debug)] |
There was a problem hiding this comment.
the qpack and header errors aren't Clone, so this was removed all the way up the tree. But it also didn't seem necessary (not even for any of the examples) to have it Clone in the first place.
There was a problem hiding this comment.
I think moq_lite assumes they are clone, so yeah this would be a breaking change.
| fn uri_to_url(uri: Uri) -> Result<Url, ConnectError> { | ||
| let Parts { | ||
| scheme, | ||
| authority, | ||
| path_and_query, | ||
| .. | ||
| } = uri.into_parts(); | ||
| let scheme = scheme.unwrap_or(Scheme::HTTPS); | ||
| let Some(authority) = authority else { | ||
| return Err(ConnectError::WrongAuthority); | ||
| }; | ||
| let Some(path_and_query) = path_and_query else { | ||
| return Err(ConnectError::WrongPath); | ||
| }; | ||
| Ok( | ||
| Url::parse(&format!("{scheme}://{authority}{path_and_query}")) | ||
| .expect("Failed to parse URL"), | ||
| ) | ||
| } | ||
|
|
||
| fn url_to_uri(url: &Url) -> Result<Uri, ConnectError> { | ||
| let mut path_and_query = url.path().to_owned(); | ||
| if let Some(query) = url.query() { | ||
| path_and_query.push('?'); | ||
| path_and_query.push_str(query); | ||
| } | ||
| Ok(Uri::builder() | ||
| .authority(url.authority()) | ||
| .scheme(url.scheme()) | ||
| .path_and_query(path_and_query) | ||
| .build()?) |
There was a problem hiding this comment.
some rather crude URL-> http::URI converters ... maybe there is something out there we can reuse that has tests?!?
| let mut headers = self.headers.clone(); | ||
| headers.insert( | ||
| "sec-webtransport-http3-draft", | ||
| HeaderValue::from_static("draft02"), |
There was a problem hiding this comment.
it stands to argue that this should now be draft14 actually, but I wasn't sure if that would break something, like browser support ...
There was a problem hiding this comment.
heh yeah don't touch it unless you're willing to test all of the browsers.
There was a problem hiding this comment.
removed as this and qpack are done more complete with the qpack crate
| let client = web_transport_quinn::ClientBuilder::new() | ||
| .dangerous() | ||
| .with_no_certificate_verification()?; | ||
|
|
There was a problem hiding this comment.
this example isn't about credentials so I simplified this compared to the echo examples
| request: ConnectRequest, | ||
| pub request: ConnectRequest, | ||
| // The response that was sent by the server. | ||
| pub response: Option<ConnectResponse>, |
There was a problem hiding this comment.
the response holds the agreed upon protocol, that's why we need to keep it around, too.
There was a problem hiding this comment.
IDK I feel like the application should keep track of the selected protocol. And it being an Option is gross.
There was a problem hiding this comment.
Right now, the selected protocol is only known once that ConnectResponse is sent and parsed. Which is happening in this struct internally only and then is discarded - so the outer app would never be able to read it. The only way for the app to keep track of it (from the client side) is to somehow gain access to the connect response. We could parse it here directly of course, but then any other headers (which are allowed as far as I understand) would become inaccessible to the App. I went for the more general approach allowing the client access to all of it without caring about the details.
And it being an Option is gross.
Sure is. Sure is. Reasons is that the Connect is used for both the incoming as well as the outgoing type and thus has this intermediate state (on the server side) where the response isn't sent yet. I decided to stay clear of doing that refactor as it would blow up the type changes and complexity and I wasn't sure this was accepted.
| /// The HTTP/3 ALPN is required when negotiating a QUIC connection. | ||
| pub const ALPN: &str = "h3"; | ||
|
|
||
| pub mod protocol_negotation { |
There was a problem hiding this comment.
should this be on the proto-crate? Wasn't clear as the ALPN above was also here ... so I kept it here for now.
There was a problem hiding this comment.
Yeah exactly my point, all of this header stuff should be hidden in -proto behind type-safe fields.
| pub const AVAILABLE_NAME: HeaderName = HeaderName::from_static("wt-available-protocols"); | ||
| /// The header name for the selected protocol, sent within the WebTransport Connect response. | ||
| pub const SELECTED_NAME: HeaderName = HeaderName::from_static("wt-protocol"); |
There was a problem hiding this comment.
would be nice to have integration tests that these encode correctly in the headers (as the spec says, they should become WT-Available-Protocols and WT-Protocol respectively, which is bit off default scheme with its first two letter being uppercase)
| // The URL used to create the session. | ||
| url: Url, | ||
| /// the protocol negotiated during the handshake | ||
| protocol: Option<String>, |
There was a problem hiding this comment.
before we didn't insist this to be a string, but for testing this was easiest. But we could make this a HeaderValue of course ...
There was a problem hiding this comment.
I'm fine leading it a String, no need to get too pedantic.
There was a problem hiding this comment.
I just looked up the latest spec and it states that the field must be Strings:
In both cases, the only valid value type is a String.
I had worked mostly against draft 9, which was the first to introduce the field, which didn't have that requirement yet. But with this in the spec, I think it's a good idea to switch to strict typing as well :) .
There was a problem hiding this comment.
quiche is just updated to fix compilation errors.
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
My qpack implementation supports literals, so it'll work just fine for sub-protocol negotiation. There's really no point implementing dynamic table support if we're only supporting a single request.
I'm okay with switching to a dedicated qpack crate, but can you make a separate PR for that? I'm sure it works but I'd rather limit the blast radius.
| fn uri_to_url(uri: Uri) -> Result<Url, ConnectError> { | ||
| let Parts { | ||
| scheme, | ||
| authority, | ||
| path_and_query, | ||
| .. | ||
| } = uri.into_parts(); | ||
| let scheme = scheme.unwrap_or(Scheme::HTTPS); | ||
| let Some(authority) = authority else { | ||
| return Err(ConnectError::WrongAuthority); | ||
| }; | ||
| let Some(path_and_query) = path_and_query else { | ||
| return Err(ConnectError::WrongPath); | ||
| }; | ||
| Ok( | ||
| Url::parse(&format!("{scheme}://{authority}{path_and_query}")) | ||
| .expect("Failed to parse URL"), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Yeah this is spooky, just return an error. Or better yet, remove the need to convert between both of these crates.
|
|
||
| // Errors that can occur during the connect request. | ||
| #[derive(Error, Debug, Clone)] | ||
| #[derive(Error, Debug)] |
There was a problem hiding this comment.
I think moq_lite assumes they are clone, so yeah this would be a breaking change.
| #[derive(Debug)] | ||
| pub struct ConnectRequest { | ||
| pub url: Url, | ||
| pub headers: HeaderMap, |
There was a problem hiding this comment.
Hmm, this might be an unpopular opinion, but I think we should strongly type standardized headers:
pub sub_protocols: Vec<String>There was a problem hiding this comment.
I am in favor for strongly typed, the spec wasn't clear on that in earlier drafts, but is now in the latest, so there is a fair reason to do that. But if we only did the sub_protocols here we'd limit the ability for implementations to add further headers, like the prioritization header (#3.4 in draft 14) of the spec or any additional headers, which are allowed and could be used to implement custom behaviors.
I guess the question here would be how specific we'd want the proto-crate to be. I'd err on the side of generic here though and keep it generic, but we could add specific helpers on a builder (.sub_protocols(protocols: Vec<String>) etc) for ease of use.
| #[derive(Debug)] | ||
| pub struct ConnectResponse { | ||
| pub status: http::status::StatusCode, | ||
| pub headers: HeaderMap, |
There was a problem hiding this comment.
Same deal, but this should be:
pub sub_protocol: Option<String>,And maybe we should error if no sub_protocol is selected but the sub_protocols request is not empty.
There was a problem hiding this comment.
complicated. according to the spec, the server "MAY" include that in the response, but it is not obliged to do so. So even if you requested, the server might respond with a 200 and not tell you anything about any protocol. That seems to be legal according to the spec.
While I would agree that this in principle feels like a non-alignment on the negotiation, error'ing here robs the implementation of reacting on the empty field with a default they might consider acceptable.
Using only sub-protocol again has the same drawback of blocking other an custom headers.
| let mut headers = self.headers.clone(); | ||
| headers.insert( | ||
| "sec-webtransport-http3-draft", | ||
| HeaderValue::from_static("draft02"), |
There was a problem hiding this comment.
heh yeah don't touch it unless you're willing to test all of the browsers.
| request: ConnectRequest, | ||
| pub request: ConnectRequest, | ||
| // The response that was sent by the server. | ||
| pub response: Option<ConnectResponse>, |
There was a problem hiding this comment.
IDK I feel like the application should keep track of the selected protocol. And it being an Option is gross.
| self.respond_with_headers(status, Default::default()).await | ||
| } | ||
|
|
||
| pub async fn respond_with_headers( |
There was a problem hiding this comment.
Again, I think we hold off on a generic _with_headers API for now. I don't want library consumers to have to read the WebTransport spec to figure out what headers are allowed/needed.
There was a problem hiding this comment.
what is the proposal then? I added the _with_headers only for the optional ability to set headers at all, there is still the regular new that behaves the same way as before.
Without the _with_headers implementers aren't able to set the subprotocol headers...
There was a problem hiding this comment.
What about:
pub async fn respond(&mut self, response: web_transport_proto::ConnectResponse) -> Result<(), ConnectError> {With protocol: Option<String> added to ConnectResponse?
| pub struct Connect { | ||
| // The request that was sent by the client. | ||
| request: ConnectRequest, | ||
| pub request: ConnectRequest, |
There was a problem hiding this comment.
I'm fine making this pub, although maybe via Deref so it's easier to access and read-only?
| /// The HTTP/3 ALPN is required when negotiating a QUIC connection. | ||
| pub const ALPN: &str = "h3"; | ||
|
|
||
| pub mod protocol_negotation { |
There was a problem hiding this comment.
Yeah exactly my point, all of this header stuff should be hidden in -proto behind type-safe fields.
| // The URL used to create the session. | ||
| url: Url, | ||
| /// the protocol negotiated during the handshake | ||
| protocol: Option<String>, |
There was a problem hiding this comment.
I'm fine leading it a String, no need to get too pedantic.
|
thanks for the feedback @kixelated - and fast as well! I responded to some of the comments inline. I'll address some of them in an update but some also need architecture decisions I can't make for you crates :) .
Main reason I did the qpack crate extension was that I wasn't really able to get that header - and potential other headers - into the existing implementation. I don't have any experience with qpack and its complex dynamic table system though, but thought that a more generic So I am not strongly pushing for using the crate instead of the local implementation. With sub protocols now being defined as String (or fail), I'll check again if I can get this to work without going to the external crate. If so, this would also eliminate the Splitting it up into two PRs would only work if this was on top of the other, as I wasn't able to get the protocol fields into the previous implementation before. But let me try again. |
|
So, just using |
|
Main reason I was going for the If that sounds fine to you, I can revert back to the previous qpack implementation and add the typed protocol fields instead, but we should mark somewhere that only the first/last (depending of the implementation) of these fields will be parsed/used... |
|
Yeah let's just break this PR into two parts:
pub struct ConnectRequest {
pub url: Url,
// technically this should be Option<Vec<String>> but who cares
pub protocols: Vec<String>,
}pub struct ConnectResponse {
pub status: http::status::StatusCode,
pub protocol: Option<String>
}
Also maybe switch from Url to Uri? I kind of hate the |
And yeah, |
|
replaced in favor of #140 . |
2 participants
Hey there,
for fun and no profit, I took it upon me to implement sub protocol negotiation refs #16
, implemented for the quinn crate and in accordance with draft 14 (the current) of the spec.
The implementation is split into three parts:
echoexample and a customping/ack) with a simple client that connects and sends a message and shows the response has been added, that shows how to do protocol negotiation with this API and offer several protocols over, one per web transport channel, within this framework.Additional implementation details are added as review comments inline.