Skip to content

remove 2 debug_asserts #3478

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.

Sign up for GitHub

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

Open
jesup wants to merge 2 commits into
base: users/jesup/datagram_max_size
Choose a base branch
from
Open

remove 2 debug_asserts #3478

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f4944a7
subtract out some overheads from the max datagram size
jesup Feb 26, 2026
b5a260e
remove debug_assert that could cause debug builds to fail in a race c…
jesup Mar 16, 2026
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
12 changes: 10 additions & 2 deletions neqo-http3/src/connection_client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -894,9 +894,17 @@ impl Http3Client {
///
/// This cannot panic. The max varint length is 8.
pub fn webtransport_max_datagram_size(&self, session_id: StreamId) -> Res<u64> {
let session_id_len = u64::try_from(Encoder::varint_len(session_id.as_u64()))
.map_err(|_| Error::Internal)?;
// Reserve space for ACK frames and packet number growth beyond what is
// seen at session start (when max_datagram_size is sampled). A worst-case
// ACK frame is ~50 bytes (RecvdPackets::USEFUL_ACK_LEN) and packet number
// varints can grow by up to 3 bytes over the life of a connection, giving
// ~53 bytes of realistic overhead; 64 provides a small margin.
const DATAGRAM_OVERHEAD: u64 = 64;
Ok(self.conn.max_datagram_size()?
- u64::try_from(Encoder::varint_len(session_id.as_u64()))
.map_err(|_| Error::Internal)?)
.saturating_sub(session_id_len)
.saturating_sub(DATAGRAM_OVERHEAD))
}

pub fn webtransport_set_datagram_high_water_mark(
Expand Down
1 change: 0 additions & 1 deletion neqo-http3/src/features/extended_connect/session.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,7 +444,6 @@ impl Session {
qtrace!("[{self}] send_datagram state={:?}", self.state);
if self.state != State::Active {
qdebug!("[{self}]: cannot send datagram in {:?} state.", self.state);
debug_assert!(false);
Copy link
Copy Markdown
Member

@mxinden mxinden Mar 16, 2026

Choose a reason for hiding this comment

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

👍

return Err(Error::Unavailable);
}
if conn.remote_datagram_size() == 0 && self.protocol.datagram_capsule_support() {
Expand Down
8 changes: 6 additions & 2 deletions neqo-http3/src/features/extended_connect/tests/webtransport/datagrams.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ use crate::{
};

const DGRAM: &[u8] = &[0, 100];
// Overhead subtracted by webtransport_max_datagram_size on the client side.
const CLIENT_DATAGRAM_OVERHEAD: u64 = 64;

#[test]
fn no_datagrams() {
Expand Down Expand Up @@ -63,7 +65,8 @@ fn do_datagram_test(wt: &mut WtTest, wt_session: &WebTransportRequest) {
assert_eq!(
wt.max_datagram_size(wt_session.stream_id()),
Ok(DATAGRAM_SIZE
- u64::try_from(Encoder::varint_len(wt_session.stream_id().as_u64())).unwrap())
- u64::try_from(Encoder::varint_len(wt_session.stream_id().as_u64())).unwrap()
- CLIENT_DATAGRAM_OVERHEAD)
);

assert_eq!(wt_session.send_datagram(DGRAM, None, now(), 0, 0), Ok(()));
Expand Down Expand Up @@ -99,7 +102,8 @@ fn datagrams_server_only() {
assert_eq!(
wt.max_datagram_size(wt_session.stream_id()),
Ok(DATAGRAM_SIZE
- u64::try_from(Encoder::varint_len(wt_session.stream_id().as_u64())).unwrap())
- u64::try_from(Encoder::varint_len(wt_session.stream_id().as_u64())).unwrap()
- CLIENT_DATAGRAM_OVERHEAD)
);

assert_eq!(
Expand Down
8 changes: 3 additions & 5 deletions neqo-http3/src/features/extended_connect/webtransport_session.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,11 +419,9 @@ impl Protocol for Session {
_buf: &[u8],
_now: Instant,
) -> Res<()> {
debug_assert!(
false,
"[{self}] WebTransport does not support datagram capsules."
);
Ok(())
// WebTransport uses QUIC datagrams, not HTTP DATAGRAM Capsules.
// datagram_capsule_support() returns false so this should never be called.
Err(Error::Unavailable)
Comment on lines -422 to +424
Copy link
Copy Markdown
Member

@mxinden mxinden Mar 16, 2026

Choose a reason for hiding this comment

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

Can you explain how you are hitting this debug_assert?

webtransport::Session::datagram_capsule_support always returns false.

neqo/neqo-http3/src/features/extended_connect/webtransport_session.rs

Lines 402 to 413 in 6978589

fn datagram_capsule_support(&self) -> bool {
// HTTP/3 WebTransport requires QUIC datagram support. In other words,
// HTTP/3 WebTransport never falls back to HTTP datagram capsules.
//
// > WebTransport over HTTP/3 also requires support for QUIC datagrams.
// > To indicate support, both the client and the server send a
// > max_datagram_frame_size transport parameter with a value greater than
// > 0 (see Section 3 of [QUIC-DATAGRAM]).
//
// <https://www.ietf.org/archive/id/draft-ietf-webtrans-http3-14.html#section-3.1>
false
}

Thus webtransport::Session::write_datagram_capsule should enver be called.

neqo/neqo-http3/src/features/extended_connect/session.rs

Lines 449 to 457 in 6978589

if conn.remote_datagram_size() == 0 && self.protocol.datagram_capsule_support() {
qtrace!("[{self}] remote_datagram_size is 0, trying HTTP DATAGRAM Capsule");
self.protocol.write_datagram_capsule(
&mut self.control_stream_send,
conn,
buf,
now,
)?;
return Ok((true, None));

Copy link
Copy Markdown
Member Author

@jesup jesup Mar 17, 2026

Choose a reason for hiding this comment

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

Not hitting it. This shouldn't be possible at the moment, but if some future change (accidentally) changed that, returning an error would be better I think

Copy link
Copy Markdown
Member

@mxinden mxinden Mar 25, 2026

Choose a reason for hiding this comment

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

but if some future change (accidentally) changed that

In that case it would panic in debug mode, thus alerting us of the bug, and return an error in release mode, thus not causing harm. In other words, please keep this as is.

}

fn set_datagram_high_water_mark(&mut self, mark: f64) {
Expand Down
1 change: 0 additions & 1 deletion neqo-http3/tests/connect_udp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,6 @@ fn connect_via_proxy() {
}

#[test]
#[cfg_attr(debug_assertions, should_panic(expected = "assertion failed: false"))]
fn send_dgram_on_non_active_session() {
let (mut client, _proxy, connect_udp_session_id) = initiate_new_session();

Expand Down