Skip to content

Commit

Permalink
Revert "Re-add invalidated remote/local candidates (algesten#508)"
Browse files Browse the repository at this point in the history 
This PR currently causes problems for firezone, hence it is reverted in our fork.
  • Loading branch information
@thomaseizinger
thomaseizinger committed May 21, 2024
1 parent efd420e commit 07c4fb0
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 151 deletions.
94 changes: 33 additions & 61 deletions src/ice/agent.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,7 @@ use super::pair::{CandidatePair, CheckState, PairId};
/// Handles the ICE protocol for a given peer.
///
/// Each connection between two peers corresponds to one [`IceAgent`] on either end.
/// To form connections to multiple peers, a peer needs to create a dedicated [`IceAgent`] for
/// each one.
/// To form connections to multiple peers, a peer needs to create a dedicated [`IceAgent`] for each one.
#[derive(Debug)]
pub struct IceAgent {
/// Last time handle_timeout run (paced by timing_advance).
Expand Down Expand Up @@ -481,52 +480,37 @@ impl IceAgent {

c.set_local_preference(pref);

// Tie this ufrag to this ICE-session.
c.set_ufrag(&self.local_credentials.ufrag);

// A candidate is redundant if and only if its transport address and base equal those
// of another candidate. The agent SHOULD eliminate the redundant
// candidate with the lower priority.
//
// NB this must be done _after_ set_local_preference(), since the prio() used in the
// elimination is calculated from that preference.
let maybe_redundant =
if let Some((idx, other)) =
self.local_candidates.iter_mut().enumerate().find(|(_, v)| {
v.addr() == c.addr() && v.base() == c.base() && v.proto() == c.proto()
});

let local_idx = if let Some((idx, other)) = maybe_redundant {
if other.discarded() && c.kind() == other.kind() && c.raddr() == other.raddr() {
debug!("Re-enable previously discarded local: {:?}", other);
other.set_discarded(false);
idx
} else {
if c.prio() < other.prio() {
// The new candidate is not better than what we already got.
debug!(
"Reject redundant candidate, current: {:?} rejected: {:?}",
other, c
);
return false;
}

// Stop using the current candidate in favor of the new one.
})
{
if c.prio() < other.prio() {
// The new candidate is not better than what we already got.
debug!(
"Replace redundant candidate, current: {:?} replaced with: {:?}",
"Reject redundant candidate, current: {:?} rejected: {:?}",
other, c
);
other.set_discarded(true);
self.discard_candidate_pairs_by_local(idx);

info!("Add local candidate: {:?}", c);
self.local_candidates.push(c);
self.local_candidates.len() - 1
return false;
}
} else {
info!("Add local candidate: {:?}", c);
self.local_candidates.push(c);
self.local_candidates.len() - 1
};

// Stop using the current candidate in favor of the new one.
debug!(
"Replace redundant candidate, current: {:?} replaced with: {:?}",
other, c
);
other.set_discarded(true);
self.discard_candidate_pairs_by_local(idx);
}

// Tie this ufrag to this ICE-session.
c.set_ufrag(&self.local_credentials.ufrag);

// These are the indexes of the remote candidates this candidate should be paired with.
let remote_idxs: Vec<_> = self
Expand All @@ -537,6 +521,12 @@ impl IceAgent {
.map(|(i, _)| i)
.collect();

info!("Add local candidate: {:?}", c);

self.local_candidates.push(c);

let local_idxs = [self.local_candidates.len() - 1];

// We always run in trickle ice mode.
//
// https://www.rfc-editor.org/rfc/rfc8838.html#section-10
Expand All @@ -546,7 +536,7 @@ impl IceAgent {
// TODO: The trickle ice spec is strange. What does it mean "has been trickled to the
// remote party"? Since we don't get a confirmation that the candidate has been received
// by the remote party, whether we form local pairs directly or later seems irrelevant.
self.form_pairs(&[local_idx], &remote_idxs);
self.form_pairs(&local_idxs, &remote_idxs);

true
}
Expand Down Expand Up @@ -602,25 +592,10 @@ impl IceAgent {
*existing = c;
idx
} else {
let maybe_discarded = self.remote_candidates.iter().position(|o| {
o.discarded()
&& c.addr() == o.addr()
&& c.base() == o.base()
&& c.proto() == o.proto()
&& c.kind() == o.kind()
&& c.raddr() == o.raddr()
});
info!("Add remote candidate: {:?}", c);

if let Some(idx) = maybe_discarded {
let other = &mut self.remote_candidates[idx];
debug!("Re-enable previously discarded remote: {:?}", other);
other.set_discarded(false);
idx
} else {
info!("Add remote candidate: {:?}", c);
self.remote_candidates.push(c);
self.remote_candidates.len() - 1
}
self.remote_candidates.push(c);
self.remote_candidates.len() - 1
};

// These are the indexes of the local candidates this candidate should be paired with.
Expand Down Expand Up @@ -937,8 +912,7 @@ impl IceAgent {

/// Provide the current time to the [`IceAgent`].
///
/// Typically, you will want to call [`IceAgent::poll_timeout`] and "wake-up"
/// the agent once that time is reached.
/// Typically, you will want to call [`IceAgent::poll_timeout`] and "wake-up" the agent once that time is reached.
pub fn handle_timeout(&mut self, now: Instant) {
// This happens exactly once because evaluate_state() below will
// switch away from New -> Checking.
Expand Down Expand Up @@ -1263,10 +1237,8 @@ impl IceAgent {
}) {
Some((i, _)) => i,
None => {
// Receiving traffic for an IP address that neither is a HOST nor RELAY
// is most likely a configuration fault where the user forgot to add a
// candidate for the local interface. We are network-connected application
// so we need to handle this gracefully: Log a message and discard the packet.
// Receiving traffic for an IP address that neither is a HOST nor RELAY is most likely a configuration fault where the user forgot to add a candidate for the local interface.
// We are network-connected application so we need to handle this gracefully: Log a message and discard the packet.

debug!(
"Discarding STUN request on unknown interface: {}",
Expand Down
90 changes: 0 additions & 90 deletions src/ice/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -362,96 +362,6 @@ mod test {
assert!(a2.time.duration_since(a2_time) < STUN_TIMEOUT);
}

#[test]
pub fn re_adding_invalidated_local_candidate() {
let mut a1 = TestAgent::new(info_span!("L"));
let mut a2 = TestAgent::new(info_span!("R"));

let c1 = host("1.1.1.1:1000", "udp");
a1.add_local_candidate(c1.clone());
a2.add_remote_candidate(c1.clone());
let c2 = host("2.2.2.2:1000", "udp");
a2.add_local_candidate(c2.clone());
a1.add_remote_candidate(c2);
a1.set_controlling(true);
a2.set_controlling(false);

loop {
if a1.state().is_connected() && a2.state().is_connected() {
break;
}
progress(&mut a1, &mut a2);
}

a1.agent.invalidate_candidate(&c1);

// Let time pass until it disconnects.
loop {
if a1.state().is_disconnected() && a2.state().is_disconnected() {
break;
}
progress(&mut a1, &mut a2);
}

// Add back the invalidated candidate
a1.add_local_candidate(c1);

// progress() fails after 100 number of polls.
a1.progress_count = 0;
a2.progress_count = 0;
loop {
if a1.state().is_connected() && a2.state().is_connected() {
break;
}
progress(&mut a1, &mut a2);
}
}

#[test]
pub fn re_adding_invalidated_remote_candidate() {
let mut a1 = TestAgent::new(info_span!("L"));
let mut a2 = TestAgent::new(info_span!("R"));

let c1 = host("1.1.1.1:1000", "udp");
a1.add_local_candidate(c1.clone());
a2.add_remote_candidate(c1);
let c2 = host("2.2.2.2:1000", "udp");
a2.add_local_candidate(c2.clone());
a1.add_remote_candidate(c2.clone());
a1.set_controlling(true);
a2.set_controlling(false);

loop {
if a1.state().is_connected() && a2.state().is_connected() {
break;
}
progress(&mut a1, &mut a2);
}

a1.agent.invalidate_candidate(&c2);

// Let time pass until it disconnects.
loop {
if a1.state().is_disconnected() && a2.state().is_disconnected() {
break;
}
progress(&mut a1, &mut a2);
}

// Add back the invalidated candidate
a1.add_remote_candidate(c2);

// progress() fails after 100 number of polls.
a1.progress_count = 0;
a2.progress_count = 0;
loop {
if a1.state().is_connected() && a2.state().is_connected() {
break;
}
progress(&mut a1, &mut a2);
}
}

#[test]
pub fn migrates_to_new_candidates_after_invalidation_without_timeout() {
let _guard = tracing_subscriber::fmt()
Expand Down

0 comments on commit 07c4fb0

Please sign in to comment.