Skip to content

Commit 7fb80b9

Browse files
authored
feat: prune old, inactive paths (#3666)
## Description Whenever we insert a new path, trigger pruning paths. We currently only prune IP paths, and pruning paths only occurs if we have more than 30 IP paths. We will prune any paths that did not successfully holepunch. If there are still over 30 IP paths left, then we order the "inactive" paths (paths that have been closed, but at one point holepunched), and prune the paths that were closed earliest. ## Notes and Questions - Added constants: - `MAX_IP_PATHS` = 30 - maximum IP paths per endpoint - `MAX_INACTIVE_IP_PATHS` = 10 - maximum inactive IP paths to keep - New `PathState` field: - `status` - tracks the `PathStatus` of the path - New `PathStatus` enum: - `PathStatus::Open` - is an open path - `PathStatus::Inactive(Instant)` - was opened once, but currently inactive - `PathStatus::Unusable` - we attempted to use it, but it never connected - `PathStatus::Unknown` - we don't know the status yet - New methods on `RemotePathState`: - `abandoned_path` - marks a path as abandoned with timestamp, triggered when we get the `PathEvent::Abandoned` event - `prune_paths` - triggers path pruning, occurs whenever we insert a path to the `RemotePathState` - changed `insert` to `insert_open_path` - New `prune_ip_paths` function with all the prune logic: - Only prunes if IP paths exceed `MAX_IP_PATHS` - Never prunes active paths or paths of unknown status - Always prunes failed holepunch attempts (PathStatus::Unusable) - Keeps 10 most recently inactive paths that were previously successful - Special case: if all paths failed, keeps `MAX_IP_PATHS` instead of pruning everything - Added tests for edge cases and the typical case
1 parent 51ba699 commit 7fb80b9

File tree

3 files changed

+448
-32
lines changed

3 files changed

+448
-32
lines changed

iroh/src/magicsock/remote_map.rs

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -185,11 +185,6 @@ pub enum Source {
185185
_0: Private,
186186
},
187187
/// We established a connection on this address.
188-
///
189-
/// Currently this means the path was in uses as [`PathId::ZERO`] when the a connection
190-
/// was added to the `RemoteStateActor`.
191-
///
192-
/// [`PathId::ZERO`]: quinn_proto::PathId::ZERO
193188
#[strum(serialize = "Connection")]
194189
Connection {
195190
/// private marker

iroh/src/magicsock/remote_map/remote_state.rs

Lines changed: 10 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -50,16 +50,6 @@ const HOLEPUNCH_ATTEMPTS_INTERVAL: Duration = Duration::from_secs(5);
5050
mod guarded_channel;
5151
mod path_state;
5252

53-
// TODO: use this
54-
// /// Number of addresses that are not active that we keep around per endpoint.
55-
// ///
56-
// /// See [`RemoteState::prune_direct_addresses`].
57-
// pub(super) const MAX_INACTIVE_DIRECT_ADDRESSES: usize = 20;
58-
59-
// TODO: use this
60-
// /// How long since an endpoint path was last alive before it might be pruned.
61-
// const LAST_ALIVE_PRUNE_DURATION: Duration = Duration::from_secs(120);
62-
6353
// TODO: use this
6454
// /// The latency at or under which we don't try to upgrade to a better path.
6555
// const GOOD_ENOUGH_LATENCY: Duration = Duration::from_millis(5);
@@ -461,7 +451,7 @@ impl RemoteStateActor {
461451
path.set_status(status).ok();
462452
conn_state.add_open_path(path_remote.clone(), PathId::ZERO);
463453
self.paths
464-
.insert(path_remote, Source::Connection { _0: Private });
454+
.insert_open_path(path_remote.clone(), Source::Connection { _0: Private });
465455
self.select_path();
466456

467457
if path_remote_is_ip {
@@ -779,15 +769,17 @@ impl RemoteStateActor {
779769
);
780770
conn_state.add_open_path(path_remote.clone(), path_id);
781771
self.paths
782-
.insert(path_remote, Source::Connection { _0: Private });
772+
.insert_open_path(path_remote.clone(), Source::Connection { _0: Private });
783773
}
784774

785775
self.select_path();
786776
}
787777
PathEvent::Abandoned { id, path_stats } => {
788778
trace!(?path_stats, "path abandoned");
789779
// This is the last event for this path.
790-
conn_state.remove_path(&id);
780+
if let Some(addr) = conn_state.remove_path(&id) {
781+
self.paths.abandoned_path(&addr);
782+
}
791783
}
792784
PathEvent::Closed { id, .. } | PathEvent::LocallyClosed { id, .. } => {
793785
let Some(path_remote) = conn_state.paths.get(&id).cloned() else {
@@ -1073,11 +1065,13 @@ impl ConnectionState {
10731065
}
10741066

10751067
/// Completely removes a path from this connection.
1076-
fn remove_path(&mut self, path_id: &PathId) {
1077-
if let Some(addr) = self.paths.remove(path_id) {
1078-
self.path_ids.remove(&addr);
1068+
fn remove_path(&mut self, path_id: &PathId) -> Option<transports::Addr> {
1069+
let addr = self.paths.remove(path_id);
1070+
if let Some(ref addr) = addr {
1071+
self.path_ids.remove(addr);
10791072
}
10801073
self.open_paths.remove(path_id);
1074+
addr
10811075
}
10821076

10831077
/// Removes the path from the open paths.

0 commit comments

Comments
 (0)