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

fix: More migration tests and related fixes #2170

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
22 changes: 18 additions & 4 deletions neqo-transport/src/cid.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
rc::Rc,
};

use neqo_common::{hex, hex_with_len, qinfo, Decoder, Encoder};
use neqo_common::{hex, hex_with_len, qdebug, qinfo, Decoder, Encoder};
use neqo_crypto::{random, randomize};
use smallvec::{smallvec, SmallVec};

Expand Down Expand Up @@ -314,6 +314,10 @@
stats.new_connection_id += 1;
true
}

pub fn is_empty(&self) -> bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
pub fn is_empty(&self) -> bool {
pub fn is_zero_len(&self) -> bool {

RFC 9000 refers to this as "zero-length Connection ID", right? How about adopting that wording here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We seem to calling these "empty", e.g., CONNECTION_ID_SEQNO_EMPTY so I stuck with that. (You're right that that's different from the RFC.)

self.seqno == CONNECTION_ID_SEQNO_EMPTY || self.cid.is_empty()
}
}

impl ConnectionIdEntry<()> {
Expand Down Expand Up @@ -405,7 +409,7 @@
pub fn retire_prior_to(&mut self, retire_prior: u64) -> Vec<u64> {
let mut retired = Vec::new();
self.cids.retain(|e| {
larseggert marked this conversation as resolved.
Show resolved Hide resolved
if e.seqno < retire_prior {
if !e.is_empty() && e.seqno < retire_prior {
retired.push(e.seqno);
false
} else {
Expand Down Expand Up @@ -510,8 +514,18 @@
pub fn retire(&mut self, seqno: u64) {
// TODO(mt) - consider keeping connection IDs around for a short while.

self.connection_ids.retire(seqno);
self.lost_new_connection_id.retain(|cid| cid.seqno != seqno);
let empty_cid = seqno == CONNECTION_ID_SEQNO_EMPTY
|| self
.connection_ids
.cids
.iter()
.any(|c| c.seqno == seqno && c.cid.is_empty());
Comment on lines +517 to +522
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let empty_cid = seqno == CONNECTION_ID_SEQNO_EMPTY
|| self
.connection_ids
.cids
.iter()
.any(|c| c.seqno == seqno && c.cid.is_empty());
let empty_cid = self
.connection_ids
.cids
.iter()
.any(|c| c.seqno == seqno && c.cid.is_empty());

Do I understand correctly, that this would be functionally the same, but less efficient?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. (I'm not in love with the CONNECTION_ID_SEQNO_EMPTY hack, esp. since we only seem to use the magic value on one end of the connection.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that self.connection_ids is likely very small, is the optimization needed? In other words, do you expect a performance impact when accepting the suggestion above?

Copy link
Collaborator Author

@larseggert larseggert Oct 25, 2024

Choose a reason for hiding this comment

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

Not sure. I mostly kept it in so the CONNECTION_ID_SEQNO_EMPTY special case is more visible :-)

if empty_cid {
qdebug!("Connection ID {seqno} is zero-length, not retiring");

Check warning on line 524 in neqo-transport/src/cid.rs

View check run for this annotation

Codecov / codecov/patch

neqo-transport/src/cid.rs#L524

Added line #L524 was not covered by tests
} else {
self.connection_ids.retire(seqno);
self.lost_new_connection_id.retain(|cid| cid.seqno != seqno);
}
}

/// During the handshake, a server needs to regard the client's choice of destination
Expand Down
2 changes: 1 addition & 1 deletion neqo-transport/src/connection/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1536,7 +1536,7 @@ impl Connection {
/// This takes two times: when the datagram was received, and the current time.
fn input(&mut self, d: Datagram<impl AsRef<[u8]>>, received: Instant, now: Instant) {
// First determine the path.
let path = self.paths.find_path_with_rebinding(
let path = self.paths.find_path(
Copy link
Collaborator

Choose a reason for hiding this comment

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

For others, relevant discussion happening on #2176 (review).

d.destination(),
d.source(),
self.conn_params.get_cc_algorithm(),
Expand Down
16 changes: 3 additions & 13 deletions neqo-transport/src/connection/tests/handshake.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ use test_fixture::{
use super::{
super::{Connection, Output, State},
assert_error, connect, connect_force_idle, connect_with_rtt, default_client, default_server,
get_tokens, handshake, maybe_authenticate, resumed_server, send_something,
get_tokens, handshake, maybe_authenticate, resumed_server, send_something, zero_len_cid_client,
CountingConnectionIdGenerator, AT_LEAST_PTO, DEFAULT_RTT, DEFAULT_STREAM_DATA,
};
use crate::{
Expand All @@ -38,8 +38,7 @@ use crate::{
stats::FrameStats,
tparams::{self, TransportParameter, MIN_ACK_DELAY},
tracking::DEFAULT_ACK_DELAY,
CloseReason, ConnectionParameters, EmptyConnectionIdGenerator, Error, Pmtud, StreamType,
Version,
CloseReason, ConnectionParameters, Error, Pmtud, StreamType, Version,
};

const ECH_CONFIG_ID: u8 = 7;
Expand Down Expand Up @@ -263,16 +262,7 @@ fn crypto_frame_split() {
#[test]
fn chacha20poly1305() {
let mut server = default_server();
let mut client = Connection::new_client(
test_fixture::DEFAULT_SERVER_NAME,
test_fixture::DEFAULT_ALPN,
Rc::new(RefCell::new(EmptyConnectionIdGenerator::default())),
DEFAULT_ADDR,
DEFAULT_ADDR,
ConnectionParameters::default(),
now(),
)
.expect("create a default client");
let mut client = zero_len_cid_client(DEFAULT_ADDR, DEFAULT_ADDR);
client.set_ciphers(&[TLS_CHACHA20_POLY1305_SHA256]).unwrap();
connect_force_idle(&mut client, &mut server);
}
Expand Down
Loading
Loading