Skip to content

fix: Iterate once in add_datagram #3239

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

Closed
larseggert wants to merge 1 commit into from
Closed

fix: Iterate once in add_datagram #3239

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
47 changes: 21 additions & 26 deletions neqo-transport/src/events.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,33 +163,28 @@ impl ConnectionEvents {
self.remove(|evt| matches!(evt, ConnectionEvent::RecvStreamReadable { stream_id: x } if *x == stream_id.as_u64()));
}

// The number of datagrams in the events queue is limited to max_queued_datagrams.
// This function ensure this and deletes the oldest datagrams (head-drop) if needed.
fn check_datagram_queued(&self, max_queued_datagrams: usize, stats: &mut Stats) {
let mut queue = self.events.borrow_mut();
let count = queue
.iter()
.filter(|evt| matches!(evt, ConnectionEvent::Datagram(_)))
.count();
if count < max_queued_datagrams {
// Below the limit. No action needed.
return;
}
let first = queue
.iter_mut()
.find(|evt| matches!(evt, ConnectionEvent::Datagram(_)))
.expect("Checked above");
// Remove the oldest (head-drop), replacing it with an
// IncomingDatagramDropped placeholder.
*first = ConnectionEvent::IncomingDatagramDropped;
stats.incoming_datagram_dropped += 1;
}

pub fn add_datagram(&self, max_queued_datagrams: usize, data: &[u8], stats: &mut Stats) {
self.check_datagram_queued(max_queued_datagrams, stats);
self.events
.borrow_mut()
.push_back(ConnectionEvent::Datagram(data.to_vec()));
let mut q = self.events.borrow_mut();
// The number of datagrams in the events queue is limited to max_queued_datagrams.
// Delete the oldest datagram (head-drop) if needed.
let mut first_idx = None;
let mut datagrams_remaining = max_queued_datagrams;
for (i, evt) in q.iter().enumerate() {
if matches!(evt, ConnectionEvent::Datagram(_)) {
first_idx.get_or_insert(i);
datagrams_remaining = datagrams_remaining.saturating_sub(1);
if datagrams_remaining == 0 {
// Replace the oldest datagram with an `IncomingDatagramDropped` placeholder.
let Some(first_idx) = first_idx else {
unreachable!("first_idx is Some when datagrams_remaining is 0");
};
Comment on lines +178 to +180
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

The unreachable! can never be reached because first_idx is guaranteed to be Some when datagrams_remaining == 0. The logic ensures we only reach this point after finding at least one datagram (which sets first_idx). Consider using unwrap() with a comment explaining the invariant, or restructure to avoid the pattern match entirely since you already know first_idx.is_some() at this point.

Suggested change
let Some(first_idx) = first_idx else {
unreachable!("first_idx is Some when datagrams_remaining is 0");
};
// Invariant: first_idx is always Some here because datagrams_remaining only reaches 0 after finding at least one datagram.
let first_idx = first_idx.unwrap();

Copilot uses AI. Check for mistakes.
q[first_idx] = ConnectionEvent::IncomingDatagramDropped;
stats.incoming_datagram_dropped += 1;
break;
}
}
}
q.push_back(ConnectionEvent::Datagram(data.to_vec()));
}

pub fn datagram_outcome(
Expand Down
Loading