-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: WebTransport on top QUIC transport #5564
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
base: master
Are you sure you want to change the base?
Conversation
@jxs Hello! |
# Conflicts: # transports/quic/src/transport.rs
Thank you @dgarus ! I hope to give this a review soon. |
@thomaseizinger |
You should get some basic coverage by adding
|
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.
Some first comments.
I like that in this approach the webtransport is integrated in the existing quic crate and that we can to support both on one listener!
Sorry of some of my comments have already been discussed in previous iterations on the PR.
let mut h3_conn = h3::server::builder() | ||
.enable_webtransport(true) | ||
.enable_connect(true) | ||
.enable_datagram(false) | ||
.max_webtransport_sessions(1) | ||
.send_grease(true) | ||
.build(h3_quinn::Connection::new(connection.clone())) | ||
.await?; |
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.
Does that mean we create a new h3
server
for each connection? Sorry if that's a noob question but if this is an incoming connection on a listener, couldn't we just have one server per listener?
request.uri().path(), | ||
))); | ||
} | ||
if request.uri().query() == Some(webtransport::NOISE_QUERY) { |
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.
if request.uri().query() == Some(webtransport::NOISE_QUERY) { | |
if request.uri().query() != Some(webtransport::NOISE_QUERY) { |
It should be the NOISE_QUERY
, right?
if Some(&Protocol::WEB_TRANSPORT) == proto { | ||
let method = request.method(); |
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.
Nit: just early return here an Err if proto != Some(&Protocol::WEB_TRANSPORT)
like you also do it when checking the method, path, etc.
// indicating no more streams to be received | ||
return Err(WebtransportConnectingError::NoMoreStreams); |
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.
What does "NoMoreStreams" mean? When does it happen?
# Conflicts: # Cargo.lock # transports/quic/Cargo.toml # transports/quic/src/config.rs # transports/quic/src/connection/connecting.rs # transports/quic/src/transport.rs # transports/tls/src/certificate.rs # transports/tls/src/lib.rs
Hi! |
# Conflicts: # Cargo.lock
Yes, we are planning to remove support for Also related: #3515 (comment) |
Ok, I got it, you're right.
No, just a bit less code base, that is all. |
# Conflicts: # Cargo.lock
@elenaf9 |
related to #2993
@thomaseizinger
Hi! There were so many changes in the master branch, so it was easier for me to create a new PR from scratch. I tried to consider all your previous comments and our discussions.
Here are the main points:
For simplicity, I made a single cert.