From c92b791b15c517fd37187f75a8d53df7ba4f63e2 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Fri, 27 Sep 2024 09:15:31 +1000 Subject: [PATCH 1/4] Always apply latest timing config --- src/ice/agent.rs | 14 +++++++------- src/ice/pair.rs | 25 +++++++++++-------------- 2 files changed, 18 insertions(+), 21 deletions(-) diff --git a/src/ice/agent.rs b/src/ice/agent.rs index 19ab6289..7e8d06dd 100644 --- a/src/ice/agent.rs +++ b/src/ice/agent.rs @@ -694,7 +694,7 @@ impl IceAgent { let prio = CandidatePair::calculate_prio(self.controlling, remote.prio(), local.prio()); let mut pair = - CandidatePair::new(*local_idx, *remote_idx, prio, self.timing_config); + CandidatePair::new(*local_idx, *remote_idx, prio, &self.timing_config); trace!("Form pair local: {:?} remote: {:?}", local, remote); @@ -1022,7 +1022,7 @@ impl IceAgent { let keep = if self.ice_lite { p.has_recent_remote_binding_request(now) } else { - p.is_still_possible(now) + p.is_still_possible(now, &self.timing_config) }; if !keep { debug!("Remove failed pair: {:?}", p); @@ -1060,7 +1060,7 @@ impl IceAgent { .candidate_pairs .iter_mut() .enumerate() - .map(|(i, c)| (i, c.next_binding_attempt(now))) + .map(|(i, c)| (i, c.next_binding_attempt(now, &self.timing_config))) .min_by_key(|(_, t)| *t); if let Some((idx, deadline)) = next { @@ -1111,7 +1111,7 @@ impl IceAgent { } else { self.candidate_pairs .iter_mut() - .map(|c| c.next_binding_attempt(last_now)) + .map(|c| c.next_binding_attempt(last_now, &self.timing_config)) .min() }; @@ -1347,7 +1347,7 @@ impl IceAgent { // * Its state is set to Waiting. (this is the default) // * The pair is inserted into the checklist based on its priority. // * The pair is enqueued into the triggered-check queue. - let pair = CandidatePair::new(local_idx, remote_idx, prio, self.timing_config); + let pair = CandidatePair::new(local_idx, remote_idx, prio, &self.timing_config); debug!("Created new pair for STUN request: {:?}", pair); @@ -1421,7 +1421,7 @@ impl IceAgent { // Only the controlling side sends USE-CANDIDATE. let use_candidate = self.controlling && pair.is_nominated(); - let trans_id = pair.new_attempt(now); + let trans_id = pair.new_attempt(now, &self.timing_config); self.stats.bind_request_sent += 1; @@ -1637,7 +1637,7 @@ impl IceAgent { for p in &self.candidate_pairs { if p.is_nominated() { any_nomination = true; - } else if p.is_still_possible(now) { + } else if p.is_still_possible(now, &self.timing_config) { any_still_possible = true; } } diff --git a/src/ice/pair.rs b/src/ice/pair.rs index 62b1fc1f..bbd97010 100644 --- a/src/ice/pair.rs +++ b/src/ice/pair.rs @@ -46,8 +46,6 @@ pub struct CandidatePair { /// State of nomination for this candidate pair. nomination_state: NominationState, - - timing_config: StunTiming, } #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] @@ -114,13 +112,12 @@ impl Default for PairId { } impl CandidatePair { - pub fn new(local_idx: usize, remote_idx: usize, prio: u64, timing_config: StunTiming) -> Self { + pub fn new(local_idx: usize, remote_idx: usize, prio: u64, timing_config: &StunTiming) -> Self { CandidatePair { local_idx, remote_idx, prio, binding_attempts: VecDeque::with_capacity(timing_config.max_retransmits() + 1), - timing_config, id: Default::default(), valid_idx: Default::default(), state: Default::default(), @@ -221,7 +218,7 @@ impl CandidatePair { /// Records a new binding request attempt. /// /// Returns the transaction id to use in the STUN message. - pub fn new_attempt(&mut self, now: Instant) -> TransId { + pub fn new_attempt(&mut self, now: Instant, timing_config: &StunTiming) -> TransId { // calculate a new time self.cached_next_attempt_time = None; @@ -240,7 +237,7 @@ impl CandidatePair { self.binding_attempts.push_back(attempt); // Never keep more than STUN_MAX_RETRANS attempts. - while self.binding_attempts.len() > self.timing_config.max_retransmits() { + while self.binding_attempts.len() > timing_config.max_retransmits() { self.binding_attempts.pop_front(); } @@ -322,7 +319,7 @@ impl CandidatePair { /// When we should do the next retry. /// /// Returns `None` if we are not to attempt this pair anymore. - pub fn next_binding_attempt(&mut self, now: Instant) -> Instant { + pub fn next_binding_attempt(&mut self, now: Instant, timing_config: &StunTiming) -> Instant { if let Some(cached) = self.cached_next_attempt_time { return cached; } @@ -335,19 +332,19 @@ impl CandidatePair { // checking more often. let unanswered_count = self .unanswered() - .filter(|(_, since)| now - *since > self.timing_config.max_rto() / 2) + .filter(|(_, since)| now - *since > timing_config.max_rto() / 2) .map(|(count, _)| count); let send_count = unanswered_count.unwrap_or(self.binding_attempts.len()); - last + self.timing_config.stun_resend_delay(send_count) + last + timing_config.stun_resend_delay(send_count) } else { // No previous attempt, do next retry straight away. now }; // At least do a check at this time. - let min = now + self.timing_config.max_rto(); + let min = now + timing_config.max_rto(); let at_least = next.min(min); @@ -360,19 +357,19 @@ impl CandidatePair { /// Tells if this candidate pair is still possible to use for connectivity. /// /// Returns `false` if the candidate has failed. - pub fn is_still_possible(&self, now: Instant) -> bool { + pub fn is_still_possible(&self, now: Instant, timing_config: &StunTiming) -> bool { let attempts = self.binding_attempts.len(); let unanswered = self.unanswered().map(|b| b.0).unwrap_or(0); - if attempts < self.timing_config.max_retransmits() - || unanswered < self.timing_config.max_retransmits() + if attempts < timing_config.max_retransmits() + || unanswered < timing_config.max_retransmits() { true } else { // check to see if we are still waiting for the last attempt // this unwrap is fine because unanswered count > 0 let last = self.last_attempt_time().unwrap(); - let cutoff = last + self.timing_config.stun_last_resend_delay(); + let cutoff = last + timing_config.stun_last_resend_delay(); now < cutoff } } From cd9f682623a58c1c0d8f4f91b9c2ed7d518100e2 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Fri, 27 Sep 2024 16:15:25 +1000 Subject: [PATCH 2/4] Don't pass `TimingConfig` in ctor --- src/ice/agent.rs | 5 ++--- src/ice/pair.rs | 6 +++--- src/io/mod.rs | 5 ++++- src/io/stun.rs | 4 +++- 4 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/ice/agent.rs b/src/ice/agent.rs index 7e8d06dd..d3c64cc2 100644 --- a/src/ice/agent.rs +++ b/src/ice/agent.rs @@ -693,8 +693,7 @@ impl IceAgent { let prio = CandidatePair::calculate_prio(self.controlling, remote.prio(), local.prio()); - let mut pair = - CandidatePair::new(*local_idx, *remote_idx, prio, &self.timing_config); + let mut pair = CandidatePair::new(*local_idx, *remote_idx, prio); trace!("Form pair local: {:?} remote: {:?}", local, remote); @@ -1347,7 +1346,7 @@ impl IceAgent { // * Its state is set to Waiting. (this is the default) // * The pair is inserted into the checklist based on its priority. // * The pair is enqueued into the triggered-check queue. - let pair = CandidatePair::new(local_idx, remote_idx, prio, &self.timing_config); + let pair = CandidatePair::new(local_idx, remote_idx, prio); debug!("Created new pair for STUN request: {:?}", pair); diff --git a/src/ice/pair.rs b/src/ice/pair.rs index bbd97010..87eb99ee 100644 --- a/src/ice/pair.rs +++ b/src/ice/pair.rs @@ -2,7 +2,7 @@ use std::collections::VecDeque; use std::fmt; use std::time::{Duration, Instant}; -use crate::io::{Id, StunTiming, TransId}; +use crate::io::{Id, StunTiming, TransId, DEFAULT_MAX_RETRANSMITS}; use crate::Candidate; // When running ice-lite we need a cutoff when we consider the remote definitely gone. @@ -112,12 +112,12 @@ impl Default for PairId { } impl CandidatePair { - pub fn new(local_idx: usize, remote_idx: usize, prio: u64, timing_config: &StunTiming) -> Self { + pub fn new(local_idx: usize, remote_idx: usize, prio: u64) -> Self { CandidatePair { local_idx, remote_idx, prio, - binding_attempts: VecDeque::with_capacity(timing_config.max_retransmits() + 1), + binding_attempts: VecDeque::with_capacity(DEFAULT_MAX_RETRANSMITS * 2), id: Default::default(), valid_idx: Default::default(), state: Default::default(), diff --git a/src/io/mod.rs b/src/io/mod.rs index fd931ab1..d59f7dc7 100644 --- a/src/io/mod.rs +++ b/src/io/mod.rs @@ -12,7 +12,10 @@ use thiserror::Error; mod stun; pub use stun::StunMessage; -pub(crate) use stun::{Class as StunClass, Method as StunMethod, StunError, StunTiming, TransId}; +pub(crate) use stun::{ + Class as StunClass, Method as StunMethod, StunError, StunTiming, TransId, + DEFAULT_MAX_RETRANSMITS, +}; mod id; // this is only exported from this crate to avoid needing diff --git a/src/io/stun.rs b/src/io/stun.rs index 30cff7c4..bd58199d 100644 --- a/src/io/stun.rs +++ b/src/io/stun.rs @@ -8,6 +8,8 @@ use crc::{Crc, CRC_32_ISO_HDLC}; use serde::{Deserialize, Serialize}; use thiserror::Error; +pub(crate) const DEFAULT_MAX_RETRANSMITS: usize = 9; + #[derive(Debug, Clone, Copy)] pub struct StunTiming { pub(crate) initial_rto: Duration, @@ -59,7 +61,7 @@ impl Default for StunTiming { fn default() -> Self { Self { initial_rto: Duration::from_millis(250), - max_retransmits: 9, + max_retransmits: DEFAULT_MAX_RETRANSMITS, max_rto: Duration::from_millis(3000), // libwebrtc uses 8000 here but we want faster detection of gone peers. } } From 08005e4291ad75d9eca2d62d03914dfece2b1ca2 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Fri, 27 Sep 2024 16:16:33 +1000 Subject: [PATCH 3/4] Remove `Clone` and `Copy` --- src/io/stun.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/io/stun.rs b/src/io/stun.rs index bd58199d..2014ddf0 100644 --- a/src/io/stun.rs +++ b/src/io/stun.rs @@ -10,7 +10,7 @@ use thiserror::Error; pub(crate) const DEFAULT_MAX_RETRANSMITS: usize = 9; -#[derive(Debug, Clone, Copy)] +#[derive(Debug)] // Purposely not `Clone` / `Copy` to ensure we always use the latest one everywhere. pub struct StunTiming { pub(crate) initial_rto: Duration, pub(crate) max_retransmits: usize, From c32514166b8eefb27b5cf6673b2238e11aef7199 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Fri, 27 Sep 2024 16:18:57 +1000 Subject: [PATCH 4/4] Fix stale config --- src/ice/pair.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/ice/pair.rs b/src/ice/pair.rs index 87eb99ee..64e1a5d5 100644 --- a/src/ice/pair.rs +++ b/src/ice/pair.rs @@ -31,7 +31,8 @@ pub struct CandidatePair { /// Record of the latest STUN messages we've tried using this pair. /// - /// This list will never grow beyond STUN_MAX_RETRANS + 1 + /// This list will usually not grow beyond [`DEFAULT_MAX_RETRANSMITS`] * 2 + /// unless the user configures a very large retransmission counter. binding_attempts: VecDeque, /// The next time we are to do a binding attempt, cached, since we @@ -236,7 +237,7 @@ impl CandidatePair { self.binding_attempts.push_back(attempt); - // Never keep more than STUN_MAX_RETRANS attempts. + // Never keep more than the maximum allowed retransmits. while self.binding_attempts.len() > timing_config.max_retransmits() { self.binding_attempts.pop_front(); }