diff --git a/src/net/hosts.rs b/src/net/hosts.rs index 079090193..cbc388f8e 100644 --- a/src/net/hosts.rs +++ b/src/net/hosts.rs @@ -61,7 +61,6 @@ use crate::{ /// or Connected. The state is `None` when the corresponding host has been removed from the /// HostRegistry. /// -/// TODO: clean up documentation surrounding the use of HostState::Free and unregister(). /// TODO: Use HostState::Free `age` variable to implement a pruning logic that deletes peers from /// the registry once they have bypassed a certain age threshold. @@ -81,30 +80,31 @@ pub type HostsPtr = Arc; pub(in crate::net) type HostRegistry = Mutex>; /// HostState is a set of mutually exclusive states that can be Insert, -/// Refine, Move, Connect, Suspend or Connected. The state is `None` when the -/// corresponding host has been removed from the HostRegistry. +/// Refine, Move, Connect, Suspend or Connected or Free. /// ``` /// +------+ -/// | None | +/// | free | /// +------+ /// ^ /// | +/// v /// +------+ +---------+ /// +------> | move | ---> | suspend | /// | +------+ +---------+ -/// | | | -/// | | v +--------+ -/// +---------+ | +--------+ | insert | -/// | connect | | | refine | +--------+ +/// | | | +--------+ +/// | | v | insert | +/// +---------+ | +--------+ +--------+ +/// | connect | | | refine | ^ /// +---------+ | +--------+ | /// | v | v /// | +-----------+ | +------+ -/// +---> | connected | <-------+-------> | None | +/// +---> | connected | <-------+-------> | free | /// +-----------+ +------+ +/// ^ /// | /// v /// +------+ -/// | None | +/// | free | /// +------+ /// /// ``` @@ -151,7 +151,8 @@ pub(in crate::net) enum HostState { impl HostState { // Try to change state to Insert. Only possible if we are not yet - // tracking this host in the HostRegistry. + // tracking this host in the HostRegistry, or if this host is marked + // as Free. fn try_insert(&self) -> Result { let start = self.to_string(); let end = HostState::Insert.to_string(); @@ -166,9 +167,8 @@ impl HostState { } } - // Try to change state to Refine. Only possible if we are not yet - // tracking this host in the HostRegistry or if the host is marked as - // Suspend i.e. we have failed to connect to it. + // Try to change state to Refine. Only possible if the peer is marked + // as Free, or Suspend i.e. we have failed to connect to it. fn try_refine(&self) -> Result { let start = self.to_string(); let end = HostState::Refine.to_string(); @@ -183,8 +183,8 @@ impl HostState { } } - // Try to change state to Connect. Only possible if we are not yet - // tracking this host in the HostRegistry. + // Try to change state to Connect. Only possible if this peer is marked + // as Free. fn try_connect(&self) -> Result { let start = self.to_string(); let end = HostState::Connect.to_string(); @@ -200,10 +200,11 @@ impl HostState { } // Try to change state to Connected. Possible if this peer's state - // is currently Connect or Refine, or Move. Refine is necessary since the + // is currently Connect, Refine, Move, or Free. Refine is necessary since the // refinery process requires us to establish a connection to a peer. // Move is necessary due to the upgrade to Gold sequence in - // `session::perform_handshake_protocols`. + // `session::perform_handshake_protocols`. Free is necessary since + // this could be a peer we previously recognize from inbound sessions. fn try_connected(&self, channel: ChannelPtr) -> Result { let start = self.to_string(); let end = HostState::Connected(channel.clone()).to_string(); @@ -219,8 +220,10 @@ impl HostState { } // Try to change state to Move. Possibly if this host is currently - // Connect i.e. it is being connected to, or if we are currently Connected - // to this peer (due to host Downgrade sequence in `session::remove_sub_on_stop`) + // Connect i.e. it is being connected to, if we are currently Connected + // to this peer (due to host Downgrade sequence in `session::remove_sub_on_stop`), + // or if this node is Free (since we might recognize this peer from a previous + // inbound session). fn try_move(&self) -> Result { let start = self.to_string(); let end = HostState::Move.to_string(); @@ -253,6 +256,12 @@ impl HostState { } } + // Free up this host to be used by the HostRegistry. The most permissive + // state that allows every state transition with the exception of Suspend + // since Suspend is specific to outbound session and has a strict state of + // transitions associated with it. + // This is preferable to simply deleting hosts from the HostRegistry since + // it is less likely to result in race conditions. fn try_free(&self, age: u64) -> Result { match self { HostState::Insert => Ok(HostState::Free(age)), @@ -858,8 +867,6 @@ impl Hosts { self.container.sort_by_last_seen(color.clone() as usize); self.container.resize(color.clone()); - // Free up this peer for usage by other parts of the code base. - // This is a safe since the hostlist modification is now complete. self.unregister(addr); } @@ -944,12 +951,11 @@ impl Hosts { None } - /// Remove a host from the HostRegistry. Must be called after move(), when the refinery - /// process fails, or when a channel stops. Prevents hosts from getting trapped in the - /// HostState logical machinery. + /// Mark as host as Free which frees it up for most future operations. pub(in crate::net) fn unregister(&self, addr: &Url) { let age = UNIX_EPOCH.elapsed().unwrap().as_secs(); self.try_register(addr.clone(), HostState::Free(age)).unwrap(); + debug!(target: "net::hosts::unregister()", "Unregistered: {}", &addr); } /// Returns the list of connected channels. @@ -1269,10 +1275,8 @@ impl Hosts { /// Note that this method puts a given Url into the "Move" state but does not reset the /// state afterwards. This is because the next state will differ depending on its usage. /// The state transition from `Move` to `Connected` or `Suspend` are both valid operations. - /// In some cases, `unregister()` can be called after `move_host()` but it must be done with - /// care due to the potential for race conditions. - /// - /// TODO: replace unregister() with a tombstone() call. + /// In some cases, `unregister()` can be called after `move_host()` to explicitly mark + /// the host state as `Free`. pub(in crate::net) fn move_host( &self, addr: &Url, diff --git a/src/net/session/manual_session.rs b/src/net/session/manual_session.rs index 123c7ad4e..dd13791f2 100644 --- a/src/net/session/manual_session.rs +++ b/src/net/session/manual_session.rs @@ -195,8 +195,7 @@ impl Slot { self.addr, e, ); - // Stop tracking this peer, to avoid it getting stuck in the Connect - // state. This is safe since we have failed to connect at this point. + // Free up this addr for future operations. self.p2p().hosts().unregister(&self.addr); } } diff --git a/src/net/session/mod.rs b/src/net/session/mod.rs index cae9d8f19..484d9649a 100644 --- a/src/net/session/mod.rs +++ b/src/net/session/mod.rs @@ -84,10 +84,10 @@ pub async fn remove_sub_on_stop( hosts.move_host(addr, last_seen, HostColor::Grey).unwrap(); } - // For all sessions that are not refine sessions, remove this channel - // from the HostRegistry. `unregister()` frees up this addr for any - // future operation. We don't call this on refine sessions since the - // unregister() call happens in the refinery directly. + // For all sessions that are not refine sessions, mark this addr as + // Free. `unregister()` frees up this addr for any future operation. We + // don't call this on refine sessions since the unregister() call + // happens in the refinery directly. if type_id & SESSION_REFINE == 0 { hosts.unregister(channel.address()); } diff --git a/src/net/session/refine_session.rs b/src/net/session/refine_session.rs index 467cf0ec9..08120ef4e 100644 --- a/src/net/session/refine_session.rs +++ b/src/net/session/refine_session.rs @@ -248,7 +248,7 @@ impl GreylistRefinery { warn!(target: "net::refinery", "No connections for {}s. GreylistRefinery paused.", offline_timer.as_secs()); - // It is neccessary to clear suspended hosts at this point, otherwise these + // It is neccessary to Free suspended hosts at this point, otherwise these // hosts cannot be connected to in Outbound Session. Failure to do this could // result in the refinery being paused forver (since connections could never be // made).