From 8271037d374e642563e074622f006986200e3762 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 16 Nov 2022 11:32:49 -0500 Subject: [PATCH 1/5] ChanMgr: Rename ChannelMap to MgrState We're doing this because the type now holds "all the mutable state in a ChanMgr", not just the map. This is a pure renaming; no documentation has been updated. Part of #606. --- crates/tor-chanmgr/src/mgr.rs | 4 ++-- crates/tor-chanmgr/src/mgr/map.rs | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/crates/tor-chanmgr/src/mgr.rs b/crates/tor-chanmgr/src/mgr.rs index 2c51fafc6..6689c3848 100644 --- a/crates/tor-chanmgr/src/mgr.rs +++ b/crates/tor-chanmgr/src/mgr.rs @@ -82,7 +82,7 @@ pub(crate) struct AbstractChanMgr { connector: CF, /// A map from ed25519 identity to channel, or to pending channel status. - pub(crate) channels: map::ChannelMap, + pub(crate) channels: map::MgrState, } /// Type alias for a future that we wait on to see when a pending @@ -103,7 +103,7 @@ impl AbstractChanMgr { ) -> Self { AbstractChanMgr { connector, - channels: map::ChannelMap::new(config.clone(), dormancy, netparams), + channels: map::MgrState::new(config.clone(), dormancy, netparams), } } diff --git a/crates/tor-chanmgr/src/mgr/map.rs b/crates/tor-chanmgr/src/mgr/map.rs index 44c441deb..6b09e9f8c 100644 --- a/crates/tor-chanmgr/src/mgr/map.rs +++ b/crates/tor-chanmgr/src/mgr/map.rs @@ -31,7 +31,7 @@ mod padding_test; /// `Mutex>` to limit the amount of code that can see and /// lock the Mutex here. (We're using a blocking mutex close to async /// code, so we need to be careful.) -pub(crate) struct ChannelMap { +pub(crate) struct MgrState { /// The data, within a lock inner: std::sync::Mutex>, } @@ -221,7 +221,7 @@ impl ChannelState { } } -impl ChannelMap { +impl MgrState { /// Create a new empty ChannelMap. pub(crate) fn new( config: ChannelConfig, @@ -234,7 +234,7 @@ impl ChannelMap { .unwrap_or_else(|e: tor_error::Bug| panic!("bug detected on startup: {:?}", e)); let _: Option<_> = update; // there are no channels yet, that would need to be told - ChannelMap { + MgrState { inner: std::sync::Mutex::new(Inner { channels: ByRelayIds::new(), config, @@ -517,8 +517,8 @@ mod test { use tor_llcrypto::pk::ed25519::Ed25519Identity; use tor_proto::channel::params::ChannelPaddingInstructionsUpdates; - fn new_test_channel_map() -> ChannelMap { - ChannelMap::new( + fn new_test_channel_map() -> MgrState { + MgrState::new( ChannelConfig::default(), Default::default(), &Default::default(), From 5804f0f350c27779333e2d66e9efe027445a093e Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 16 Nov 2022 11:46:11 -0500 Subject: [PATCH 2/5] ChanMgr: Rename map.rs to state.rs This is another pure renaming. --- crates/tor-chanmgr/src/mgr.rs | 18 +++++++++--------- .../tor-chanmgr/src/mgr/{map.rs => state.rs} | 0 .../src/mgr/{map => state}/padding_test.rs | 0 3 files changed, 9 insertions(+), 9 deletions(-) rename crates/tor-chanmgr/src/mgr/{map.rs => state.rs} (100%) rename crates/tor-chanmgr/src/mgr/{map => state}/padding_test.rs (100%) diff --git a/crates/tor-chanmgr/src/mgr.rs b/crates/tor-chanmgr/src/mgr.rs index 6689c3848..f04ae50a3 100644 --- a/crates/tor-chanmgr/src/mgr.rs +++ b/crates/tor-chanmgr/src/mgr.rs @@ -1,6 +1,6 @@ //! Abstract implementation of a channel manager -use crate::mgr::map::{OpenEntry, PendingEntry}; +use crate::mgr::state::{OpenEntry, PendingEntry}; use crate::{ChanProvenance, ChannelConfig, ChannelUsage, Dormancy, Error, Result}; use async_trait::async_trait; @@ -15,7 +15,7 @@ use tor_linkspec::{HasRelayIds, RelayIds}; use tor_netdir::params::NetParameters; use tor_proto::channel::params::ChannelPaddingInstructionsUpdates; -mod map; +mod state; /// Trait to describe as much of a /// [`Channel`](tor_proto::channel::Channel) as `AbstractChanMgr` @@ -82,7 +82,7 @@ pub(crate) struct AbstractChanMgr { connector: CF, /// A map from ed25519 identity to channel, or to pending channel status. - pub(crate) channels: map::MgrState, + pub(crate) channels: state::MgrState, } /// Type alias for a future that we wait on to see when a pending @@ -103,7 +103,7 @@ impl AbstractChanMgr { ) -> Self { AbstractChanMgr { connector, - channels: map::MgrState::new(config.clone(), dormancy, netparams), + channels: state::MgrState::new(config.clone(), dormancy, netparams), } } @@ -115,11 +115,11 @@ impl AbstractChanMgr { /// Helper: return the objects used to inform pending tasks /// about a newly open or failed channel. - fn setup_launch(&self, ids: RelayIds) -> (map::ChannelState, Sending) { + fn setup_launch(&self, ids: RelayIds) -> (state::ChannelState, Sending) { let (snd, rcv) = oneshot::channel(); let pending = rcv.shared(); ( - map::ChannelState::Building(map::PendingEntry { ids, pending }), + state::ChannelState::Building(state::PendingEntry { ids, pending }), snd, ) } @@ -256,7 +256,7 @@ impl AbstractChanMgr { target: &CF::BuildSpec, final_attempt: bool, ) -> Result>> { - use map::ChannelState::*; + use state::ChannelState::*; self.channels.with_channels(|channel_map| { match channel_map.by_all_ids(target) { Some(Open(OpenEntry { channel, .. })) => { @@ -354,7 +354,7 @@ impl AbstractChanMgr { target: &CF::BuildSpec, outcome: Result, ) -> Result> { - use map::ChannelState::*; + use state::ChannelState::*; match outcome { Ok(chan) => { // The channel got built: remember it, tell the @@ -474,7 +474,7 @@ impl AbstractChanMgr { where T: Into>, { - use map::ChannelState::*; + use state::ChannelState::*; self.channels .with_channels(|channel_map| match channel_map.by_id(ident) { Some(Open(ref ent)) if ent.channel.is_usable() => Some(ent.channel.clone()), diff --git a/crates/tor-chanmgr/src/mgr/map.rs b/crates/tor-chanmgr/src/mgr/state.rs similarity index 100% rename from crates/tor-chanmgr/src/mgr/map.rs rename to crates/tor-chanmgr/src/mgr/state.rs diff --git a/crates/tor-chanmgr/src/mgr/map/padding_test.rs b/crates/tor-chanmgr/src/mgr/state/padding_test.rs similarity index 100% rename from crates/tor-chanmgr/src/mgr/map/padding_test.rs rename to crates/tor-chanmgr/src/mgr/state/padding_test.rs From 53c401af651e35050b1fc741b2c80791284ba993 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 16 Nov 2022 12:02:02 -0500 Subject: [PATCH 3/5] Fix up documentation that referred to a ChannelMap. --- crates/tor-chanmgr/src/mgr.rs | 5 ++++- crates/tor-chanmgr/src/mgr/state.rs | 30 +++++++++++++---------------- 2 files changed, 17 insertions(+), 18 deletions(-) diff --git a/crates/tor-chanmgr/src/mgr.rs b/crates/tor-chanmgr/src/mgr.rs index f04ae50a3..c720abe11 100644 --- a/crates/tor-chanmgr/src/mgr.rs +++ b/crates/tor-chanmgr/src/mgr.rs @@ -81,7 +81,10 @@ pub(crate) struct AbstractChanMgr { /// A 'connector' object that we use to create channels. connector: CF, - /// A map from ed25519 identity to channel, or to pending channel status. + /// All internal state held by this channel manager. + /// + /// The most important part is the map from relay identity to channel, or + /// to pending channel status. pub(crate) channels: state::MgrState, } diff --git a/crates/tor-chanmgr/src/mgr/state.rs b/crates/tor-chanmgr/src/mgr/state.rs index 6b09e9f8c..b93083b19 100644 --- a/crates/tor-chanmgr/src/mgr/state.rs +++ b/crates/tor-chanmgr/src/mgr/state.rs @@ -25,29 +25,25 @@ use void::{ResultVoidExt as _, Void}; #[cfg(test)] mod padding_test; -/// A map from channel id to channel state, plus necessary auxiliary state +/// All mutable state held by an `AbstractChannelMgr`. /// -/// We make this a separate type instead of just using -/// `Mutex>` to limit the amount of code that can see and +/// One reason that this is an isolated type is that we want to +/// to limit the amount of code that can see and /// lock the Mutex here. (We're using a blocking mutex close to async /// code, so we need to be careful.) pub(crate) struct MgrState { /// The data, within a lock + /// + /// (Danger: this uses a blocking mutex close to async code. This mutex + /// must never be held while an await is happening.) inner: std::sync::Mutex>, } /// A map from channel id to channel state, plus necessary auxiliary state - inside lock struct Inner { /// A map from identity to channel, or to pending channel status. - /// - /// (Danger: this uses a blocking mutex close to async code. This mutex - /// must never be held while an await is happening.) channels: ByRelayIds>, - // TODO: The subsequent fields really do not belong in structure called - // `ChannelMap`. These, plus the actual map, should probably be in a - // structure called "MgrState", and that structure should be the thing that - // is put behind a Mutex. /// Parameters for channels that we create, and that all existing channels are using /// /// Will be updated by a background task, which also notifies all existing @@ -63,7 +59,7 @@ struct Inner { /// Dormancy /// /// The last dormancy information we have been told about and passed on to our channels. - /// Updated via `ChanMgr::set_dormancy` and hence `ChannelMap::reconfigure_general`, + /// Updated via `MgrState::set_dormancy` and hence `MgrState::reconfigure_general`, /// which then uses it to calculate how to reconfigure the channels. dormancy: Dormancy, } @@ -222,7 +218,7 @@ impl ChannelState { } impl MgrState { - /// Create a new empty ChannelMap. + /// Create a new empty `MgrState`. pub(crate) fn new( config: ChannelConfig, dormancy: Dormancy, @@ -244,9 +240,9 @@ impl MgrState { } } - /// Run a function on the `ByRelayIds` that implements this map. + /// Run a function on the `ByRelayIds` that implements the map in this `MgrState`. /// - /// This function grabs a mutex over the map: do not provide slow function. + /// This function grabs a mutex: do not provide a slow function. /// /// We provide this function rather than exposing the channels set directly, /// to make sure that the calling code doesn't await while holding the lock. @@ -258,9 +254,9 @@ impl MgrState { Ok(func(&mut inner.channels)) } - /// Run a function on the `ByRelayIds` that implements this map. + /// Run a function on the `ByRelayIds` that implements the map in this `MgrState`. /// - /// This function grabs a mutex over the map: do not provide slow function. + /// This function grabs a mutex: do not provide a slow function. /// /// We provide this function rather than exposing the channels set directly, /// to make sure that the calling code doesn't await while holding the lock. @@ -279,7 +275,7 @@ impl MgrState { Ok(func(channels, channels_params)) } - /// Remove every unusable state from the map. + /// Remove every unusable state from the map in this state.. #[cfg(test)] pub(crate) fn remove_unusable(&self) -> Result<()> { let mut inner = self.inner.lock()?; From b466e242352bd445fc68cf62883de06625c9be3e Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 16 Nov 2022 12:05:20 -0500 Subject: [PATCH 4/5] chanmgr::mgr::*: misc spelling fixes and normali[sz]ations --- crates/tor-chanmgr/src/mgr.rs | 2 +- crates/tor-chanmgr/src/mgr/state.rs | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/tor-chanmgr/src/mgr.rs b/crates/tor-chanmgr/src/mgr.rs index c720abe11..ec1ab3915 100644 --- a/crates/tor-chanmgr/src/mgr.rs +++ b/crates/tor-chanmgr/src/mgr.rs @@ -31,7 +31,7 @@ pub(crate) trait AbstractChannel: Clone + HasRelayIds { /// Return None if the channel is currently in use. fn duration_unused(&self) -> Option; - /// Reparameterise this channel according to the provided `ChannelPaddingInstructionsUpdates` + /// Reparameterize this channel according to the provided `ChannelPaddingInstructionsUpdates` /// /// The changed parameters may not be implemented "immediately", /// but this will be done "reasonably soon". diff --git a/crates/tor-chanmgr/src/mgr/state.rs b/crates/tor-chanmgr/src/mgr/state.rs index b93083b19..eb92c9b9b 100644 --- a/crates/tor-chanmgr/src/mgr/state.rs +++ b/crates/tor-chanmgr/src/mgr/state.rs @@ -139,7 +139,7 @@ impl ChannelState { /// Type of the `nf_ito_*` netdir parameters, convenience alias type NfIto = IntegerMilliseconds>; -/// Extract from a `NetDarameters` which we need, conveniently organised for our processing +/// Extract from a `NetParameters` which we need, conveniently organized for our processing /// /// This type serves two functions at once: /// @@ -288,7 +288,7 @@ impl MgrState { /// Reconfigure all channels as necessary /// - /// (By reparameterising channels as needed) + /// (By reparameterizing channels as needed) /// This function will handle /// - netdir update /// - a reconfiguration @@ -314,7 +314,7 @@ impl MgrState { let mut inner = self .inner .lock() - .map_err(|_| internal!("poisonned channel manager"))?; + .map_err(|_| internal!("poisoned channel manager"))?; let mut inner = &mut *inner; if let Some(new_config) = new_config { @@ -374,7 +374,7 @@ impl MgrState { /// /// This is called in two places: /// -/// 1. During chanmgr creation, it is called once to analyse the initial state +/// 1. During chanmgr creation, it is called once to analyze the initial state /// and construct a corresponding ChannelPaddingInstructions. /// /// 2. During reconfiguration. @@ -625,7 +625,7 @@ mod test { } #[test] - fn reparameterise_via_netdir() -> Result<()> { + fn reparameterize_via_netdir() -> Result<()> { let map = new_test_channel_map(); // Set some non-default parameters so that we can tell when an update happens From 7d4691e402bfa2ffa0f91e53809b15d048a1b613 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 16 Nov 2022 12:07:47 -0500 Subject: [PATCH 5/5] chanmgr: rename new_test_channel_map to new_test_state. --- crates/tor-chanmgr/src/mgr/state.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/tor-chanmgr/src/mgr/state.rs b/crates/tor-chanmgr/src/mgr/state.rs index eb92c9b9b..26f63ba7f 100644 --- a/crates/tor-chanmgr/src/mgr/state.rs +++ b/crates/tor-chanmgr/src/mgr/state.rs @@ -513,7 +513,7 @@ mod test { use tor_llcrypto::pk::ed25519::Ed25519Identity; use tor_proto::channel::params::ChannelPaddingInstructionsUpdates; - fn new_test_channel_map() -> MgrState { + fn new_test_state() -> MgrState { MgrState::new( ChannelConfig::default(), Default::default(), @@ -603,7 +603,7 @@ mod test { #[test] fn rmv_unusable() -> Result<()> { - let map = new_test_channel_map(); + let map = new_test_state(); map.with_channels(|map| { map.insert(closed("machen")); @@ -626,7 +626,7 @@ mod test { #[test] fn reparameterize_via_netdir() -> Result<()> { - let map = new_test_channel_map(); + let map = new_test_state(); // Set some non-default parameters so that we can tell when an update happens let _ = map @@ -683,7 +683,7 @@ mod test { #[test] fn expire_channels() -> Result<()> { - let map = new_test_channel_map(); + let map = new_test_state(); // Channel that has been unused beyond max duration allowed is expired map.with_channels(|map| { @@ -700,7 +700,7 @@ mod test { assert!(map.by_ed25519(&str_to_ed("w")).is_none()); })?; - let map = new_test_channel_map(); + let map = new_test_state(); // Channel that has been unused for shorter than max unused duration map.with_channels(|map| {