Skip to content
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

HTTP/3 connection params #151

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 16 additions & 27 deletions h3/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,14 @@ use crate::{
connection::{self, ConnectionInner, ConnectionState, SharedStateRef},
error::{Code, Error, ErrorLevel},
frame::FrameStream,
proto::{frame::Frame, headers::Header, varint::VarInt},
params::Params,
proto::{frame::Frame, headers::Header},
qpack, quic, stream,
};

/// Start building a new HTTP/3 client
/// Start building a new HTTP/3 client with default params
pub fn builder() -> Builder {
Builder::new()
Builder::new(Default::default())
}

/// Create a new HTTP/3 client with default settings
Expand All @@ -37,7 +38,7 @@ where
//# address and UDP port, where the IP address and port might be derived
//# from a URI, a selected alternative service ([ALTSVC]), a configured
//# proxy, or name resolution of any of these.
Builder::new().build(conn).await
Builder::new(Default::default()).build(conn).await
}

/// HTTP/3 request sender
Expand Down Expand Up @@ -469,34 +470,22 @@ where
/// # O: quic::OpenStreams<B>,
/// # B: bytes::Buf,
/// # {
/// let h3_conn = h3::client::builder()
/// .max_field_section_size(8192)
/// let params = h3::params::Params::default()
/// .max_field_section_size(8192);
/// let h3_conn = h3::client::Builder::new(params)
/// .build(quic)
/// .await
/// .expect("Failed to build connection");
/// # }
/// ```
pub struct Builder {
max_field_section_size: u64,
send_grease: bool,
params: Params,
}

impl Builder {
pub(super) fn new() -> Self {
Builder {
max_field_section_size: VarInt::MAX.0,
send_grease: true,
}
}

/// Set the maximum header size this client is willing to accept
///
/// See [header size constraints] section of the specification for details.
///
/// [header size constraints]: https://www.rfc-editor.org/rfc/rfc9114.html#name-header-size-constraints
pub fn max_field_section_size(&mut self, value: u64) -> &mut Self {
self.max_field_section_size = value;
self
/// Create a HTTP/3 client builder
pub fn new(params: Params) -> Self {
Self { params }
}

/// Create a new HTTP/3 client from a `quic` connection
Expand All @@ -518,20 +507,20 @@ impl Builder {
Connection {
inner: ConnectionInner::new(
quic,
self.max_field_section_size,
self.params.max_field_section_size,
conn_state.clone(),
self.send_grease,
self.params.grease,
)
.await?,
},
SendRequest {
open,
conn_state,
conn_waker,
max_field_section_size: self.max_field_section_size,
max_field_section_size: self.params.max_field_section_size,
sender_count: Arc::new(AtomicUsize::new(1)),
_buf: PhantomData,
send_grease_frame: self.send_grease,
send_grease_frame: self.params.grease,
},
))
}
Expand Down
1 change: 1 addition & 0 deletions h3/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

pub mod client;
pub mod error;
pub mod params;
pub mod quic;
pub mod server;

Expand Down
45 changes: 45 additions & 0 deletions h3/src/params.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
//! HTTP/3 connection parameters

/// HTTP/3 connection parameters builder
pub struct Params {
pub(crate) enable_webtransport: bool,
pub(crate) grease: bool,
pub(crate) max_field_section_size: u64,
}

impl Default for Params {
fn default() -> Self {
Self {
enable_webtransport: false,
grease: true,
max_field_section_size: Self::DEFAULT_MAX_FIELD_SECTION_SIZE,
}
}
}

impl Params {
/// Default max header size
pub const DEFAULT_MAX_FIELD_SECTION_SIZE: u64 = (1 << 62) - 1;

/// Set whether WebTransport is supported
pub fn enable_webtransport(mut self, val: bool) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be better to put the WebTransport stuff behind a Feature Flag. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I thought about that too. But what if it's just for params and settings?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would put WebTransport completely behind a feature flag or maybe even in a separate crate. Iirc there was some discussion about this in #71.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the benefits of putting stuff behind a feature gate sometimes (smaller binary, faster compilation, etc). I totally agree we should feature-gate or separate major implementations of WT.

But configs and settings are lightweight and hard to separate. The benefit of putting those behind a feature gate seems limited. I'm 100% sure about this though. What do you think? @seanmonstar

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about it more from a User perspective. Because it may lead to confusion when you are able to set a config which will do nothing without the feature.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it makes sense. And adding that may be too soon..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not had the time to read the WebTransport spec, so not sure what more is needed, but: does adding this at least allow for WebTransport to be proxied? If it's needed for a proxy to simply pass the data frames along, then seems fair to include it as part of the crate, perhaps?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think so. The setting would be required to be sent by both sides when establishing connection to create a WT session.

self.enable_webtransport = val;
self
}

/// Set wether to send GREASE
pub fn grease(mut self, val: bool) -> Self {
self.grease = val;
self
}

/// Set the maximum header size the endpoint is willing to accept
///
/// See [header size constraints] section of the specification for details.
///
/// [header size constraints]: https://www.rfc-editor.org/rfc/rfc9114.html#name-header-size-constraints
pub fn max_field_section_size(mut self, val: u64) -> Self {
self.max_field_section_size = val;
self
}
}
50 changes: 16 additions & 34 deletions h3/src/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,8 @@ use crate::{
connection::{self, ConnectionInner, ConnectionState, SharedStateRef},
error::{Code, Error, ErrorLevel},
frame::FrameStream,
proto::{frame::Frame, headers::Header, varint::VarInt},
params::Params,
proto::{frame::Frame, headers::Header},
qpack,
quic::{self, RecvStream as _, SendStream as _},
stream,
Expand All @@ -79,7 +80,7 @@ use tracing::{error, trace, warn};
/// This function creates a [`Builder`] that carries settings that can
/// be shared between server connections.
pub fn builder() -> Builder {
Builder::new()
Builder::new(Default::default())
}

/// Server connection driver
Expand Down Expand Up @@ -492,43 +493,24 @@ where
/// C: h3::quic::Connection<B>,
/// B: bytes::Buf,
/// {
/// let mut server_builder = h3::server::builder();
/// // Set the maximum header size
/// server_builder.max_field_section_size(1000);
/// // do not send grease types
/// server_builder.send_grease(false);
/// let params = h3::params::Params::default()
/// // Set the maximum header size
/// .max_field_section_size(1000)
/// // do not send grease types
/// .grease(false);
/// let mut server_builder = h3::server::Builder::new(params);
/// // Build the Connection
/// let mut h3_conn = server_builder.build(conn);
/// }
/// ```
pub struct Builder {
pub(super) max_field_section_size: u64,
pub(super) send_grease: bool,
pub(super) params: Params,
}

impl Builder {
/// Creates a new [`Builder`] with default settings.
pub(super) fn new() -> Self {
Builder {
max_field_section_size: VarInt::MAX.0,
send_grease: true,
}
}
/// Set the maximum header size this client is willing to accept
///
/// See [header size constraints] section of the specification for details.
///
/// [header size constraints]: https://www.rfc-editor.org/rfc/rfc9114.html#name-header-size-constraints
pub fn max_field_section_size(&mut self, value: u64) -> &mut Self {
self.max_field_section_size = value;
self
}

/// Send grease values to the Client.
/// See [setting](https://www.rfc-editor.org/rfc/rfc9114.html#settings-parameters), [frame](https://www.rfc-editor.org/rfc/rfc9114.html#frame-reserved) and [stream](https://www.rfc-editor.org/rfc/rfc9114.html#stream-grease) for more information.
pub fn send_grease(&mut self, value: bool) -> &mut Self {
self.send_grease = value;
self
/// Create a new HTTP/3 server [`Builder`]
pub fn new(params: Params) -> Self {
Self { params }
}
}

Expand All @@ -545,12 +527,12 @@ impl Builder {
Ok(Connection {
inner: ConnectionInner::new(
conn,
self.max_field_section_size,
self.params.max_field_section_size,
SharedStateRef::default(),
self.send_grease,
self.params.grease,
)
.await?,
max_field_section_size: self.max_field_section_size,
max_field_section_size: self.params.max_field_section_size,
request_end_send: sender,
request_end_recv: receiver,
ongoing_streams: HashSet::new(),
Expand Down
12 changes: 5 additions & 7 deletions h3/src/tests/connection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use crate::{
client::{self, SendRequest},
connection::ConnectionState,
error::{Code, Error, Kind},
params::Params,
proto::{
coding::Encode as _,
frame::{Frame, Settings},
Expand Down Expand Up @@ -161,11 +162,8 @@ async fn settings_exchange_client() {

let server_fut = async {
let conn = server.next().await;
let mut incoming = server::builder()
.max_field_section_size(12)
.build(conn)
.await
.unwrap();
let params = Params::default().max_field_section_size(12);
let mut incoming = server::Builder::new(params).build(conn).await.unwrap();
incoming.accept().await.unwrap()
};

Expand All @@ -179,8 +177,8 @@ async fn settings_exchange_server() {
let mut server = pair.server();

let client_fut = async {
let (mut conn, _client) = client::builder()
.max_field_section_size(12)
let params = Params::default().max_field_section_size(12);
let (mut conn, _client) = client::Builder::new(params)
.build::<_, _, Bytes>(pair.client().await)
.await
.expect("client init");
Expand Down
37 changes: 13 additions & 24 deletions h3/src/tests/request.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use crate::{
client,
connection::ConnectionState,
error::{Code, Error, Kind},
params::Params,
proto::{
coding::Encode,
frame::{self, Frame, FrameType},
Expand Down Expand Up @@ -282,11 +283,8 @@ async fn header_too_big_response_from_server() {
//= type=test
//# An HTTP/3 implementation MAY impose a limit on the maximum size of
//# the message header it will accept on an individual HTTP message.
let mut incoming_req = server::builder()
.max_field_section_size(12)
.build(conn)
.await
.unwrap();
let params = Params::default().max_field_section_size(12);
let mut incoming_req = server::Builder::new(params).build(conn).await.unwrap();

let err_kind = incoming_req.accept().await.map(|_| ()).unwrap_err().kind();
assert_matches!(
Expand Down Expand Up @@ -340,11 +338,8 @@ async fn header_too_big_response_from_server_trailers() {
//= type=test
//# An HTTP/3 implementation MAY impose a limit on the maximum size of
//# the message header it will accept on an individual HTTP message.
let mut incoming_req = server::builder()
.max_field_section_size(207)
.build(conn)
.await
.unwrap();
let params = Params::default().max_field_section_size(207);
let mut incoming_req = server::Builder::new(params).build(conn).await.unwrap();

let (_request, mut request_stream) = incoming_req.accept().await.expect("accept").unwrap();
let _ = request_stream
Expand Down Expand Up @@ -408,11 +403,8 @@ async fn header_too_big_client_error() {
//= type=test
//# An HTTP/3 implementation MAY impose a limit on the maximum size of
//# the message header it will accept on an individual HTTP message.
server::builder()
.max_field_section_size(12)
.build(conn)
.await
.unwrap();
let params = Params::default().max_field_section_size(12);
server::Builder::new(params).build(conn).await.unwrap();
};

tokio::join!(server_fut, client_fut);
Expand Down Expand Up @@ -471,11 +463,8 @@ async fn header_too_big_client_error_trailer() {
//= type=test
//# An HTTP/3 implementation MAY impose a limit on the maximum size of
//# the message header it will accept on an individual HTTP message.
let mut incoming_req = server::builder()
.max_field_section_size(207)
.build(conn)
.await
.unwrap();
let params = Params::default().max_field_section_size(207);
let mut incoming_req = server::Builder::new(params).build(conn).await.unwrap();

let (_request, mut request_stream) = incoming_req.accept().await.expect("accept").unwrap();
let _ = request_stream
Expand Down Expand Up @@ -504,8 +493,8 @@ async fn header_too_big_discard_from_client() {
//# process it.

// Do not poll driver so client doesn't know about server's max_field section size setting
let (_conn, mut client) = client::builder()
.max_field_section_size(12)
let params = Params::default().max_field_section_size(12);
let (_conn, mut client) = client::Builder::new(params)
.build::<_, _, Bytes>(pair.client().await)
.await
.expect("client init");
Expand Down Expand Up @@ -589,8 +578,8 @@ async fn header_too_big_discard_from_client_trailers() {
//# process it.

// Do not poll driver so client doesn't know about server's max_field section size setting
let (mut driver, mut client) = client::builder()
.max_field_section_size(200)
let params = Params::default().max_field_section_size(200);
let (mut driver, mut client) = client::Builder::new(params)
.build::<_, _, Bytes>(pair.client().await)
.await
.expect("client init");
Expand Down