From 7a38d68528b60822eb15ca4d99bd77fb589ee40a Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 13 Jul 2023 08:33:41 -0400 Subject: [PATCH 1/5] chanmgr: Add an experimental build_unmanaged_channel() method. This method will let the user construct a channel that isn't stored or monitored by the ChanMgr. --- crates/tor-chanmgr/src/lib.rs | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/crates/tor-chanmgr/src/lib.rs b/crates/tor-chanmgr/src/lib.rs index bcea9aedf..974a0e14e 100644 --- a/crates/tor-chanmgr/src/lib.rs +++ b/crates/tor-chanmgr/src/lib.rs @@ -296,9 +296,35 @@ impl ChanMgr { /// Obtain a channel builder which can be used to create connections to /// relays directly, bypassing many of the setup processes of [ChanMgr] #[cfg(feature = "experimental-api")] + // TODO REMOVE THIS if we keep build_unmanaged_channel; it's not actually + // usable without a BootstrapReporter pub fn builder(&self) -> Arc { Arc::new(self.mgr.channels.builder()) } + + /// Try to create a new, unmanaged channel to `target`. + /// + /// Unlike [`get_or_launch`](ChanMgr::get_or_launch), this function always + /// creates a new channel, never retries transient failure, and does not + /// register this channel with the `ChanMgr`. Generally you should not use + /// it. + /// + /// TODO: Before we merge this we should figure out if it handles timeouts + /// and whether it should? + #[cfg(feature = "experimental-api")] + pub async fn build_unmanaged_channel( + &self, + target: impl tor_linkspec::IntoOwnedChanTarget, + ) -> Result { + let target = target.to_owned(); + + self.mgr + .channels + .builder() + .connect_via_transport(&target, self.mgr.reporter.clone()) + .await + } + /// Watch for things that ought to change the configuration of all channels in the client /// /// Currently this handles enabling and disabling channel padding. From ed455023c2eddd2a54af6faa4144ab5d065a2d49 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 13 Jul 2023 09:24:13 -0400 Subject: [PATCH 2/5] chanmgr: Document makeup and timeout behavior of our factories Basically, it's all ChanBuilder at some point, and ChanBuilder has a timeout. --- crates/tor-chanmgr/src/lib.rs | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/crates/tor-chanmgr/src/lib.rs b/crates/tor-chanmgr/src/lib.rs index 974a0e14e..d14f38f3a 100644 --- a/crates/tor-chanmgr/src/lib.rs +++ b/crates/tor-chanmgr/src/lib.rs @@ -96,6 +96,23 @@ use tor_rtcompat::scheduler::{TaskHandle, TaskSchedule}; /// in the future.) pub struct ChanMgr { /// Internal channel manager object that does the actual work. + /// + /// ## How this is built + /// + /// This internal manager is parameterized over an + /// [`mgr::AbstractChannelFactory`], which here is instantiated with a [`factory::CompoundFactory`]. + /// The `CompoundFactory` itself holds: + /// * A `dyn` [`factory::AbstractPtMgr`] that can provide a `dyn` + /// [`factory::ChannelFactory`] for each supported pluggable transport. + /// This starts out as `None`, but can be replaced with [`ChanMgr::set_pt_mgr`]. + /// The `TorClient` code currently sets this using `tor_ptmgr::PtMgr`. + /// `PtrMgr` currently returns `ChannelFactory` implementations that are + /// built using [`transport::proxied::ExternalProxyPlugin`], which implements + /// [`transport::TransportImplHelper`], which in turn is wrapped into a + /// `ChanBuilder` to implement `ChannelFactory`. + /// * A `dyn` [`factory::ChannelFactory`] that it uses for everything else + /// it uses for everything else. We instantiate this with a + /// [`builder::ChanBuilder`] using a [`transport::default::DefaultTransport`]. mgr: mgr::AbstractChanMgr, /// Stream of [`ConnStatus`] events. @@ -288,6 +305,11 @@ impl ChanMgr { /// Replace the transport registry with one that may know about /// more transports. + /// + /// Note that the [`ChannelFactory`] instances returned by `ptrmgr` are + /// required to time-out channels that take too long to build. You'll get + /// this behavior by default if the factories implement [`ChannelFactory`] using + /// [`transport::proxied::ExternalProxyPlugin`], which `tor-ptrmgr` does. #[cfg(feature = "pt-client")] pub fn set_pt_mgr(&self, ptmgr: Arc) { self.mgr.with_mut_builder(|f| f.replace_ptmgr(ptmgr)); From 6b61eec987134f32a184133662ab34edd38c1e21 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 13 Jul 2023 09:31:28 -0400 Subject: [PATCH 3/5] chanmgr: Remove now-unused (and never usable) builder() method. --- crates/tor-chanmgr/src/lib.rs | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/crates/tor-chanmgr/src/lib.rs b/crates/tor-chanmgr/src/lib.rs index d14f38f3a..737bbbe05 100644 --- a/crates/tor-chanmgr/src/lib.rs +++ b/crates/tor-chanmgr/src/lib.rs @@ -315,24 +315,12 @@ impl ChanMgr { self.mgr.with_mut_builder(|f| f.replace_ptmgr(ptmgr)); } - /// Obtain a channel builder which can be used to create connections to - /// relays directly, bypassing many of the setup processes of [ChanMgr] - #[cfg(feature = "experimental-api")] - // TODO REMOVE THIS if we keep build_unmanaged_channel; it's not actually - // usable without a BootstrapReporter - pub fn builder(&self) -> Arc { - Arc::new(self.mgr.channels.builder()) - } - /// Try to create a new, unmanaged channel to `target`. /// /// Unlike [`get_or_launch`](ChanMgr::get_or_launch), this function always /// creates a new channel, never retries transient failure, and does not /// register this channel with the `ChanMgr`. Generally you should not use /// it. - /// - /// TODO: Before we merge this we should figure out if it handles timeouts - /// and whether it should? #[cfg(feature = "experimental-api")] pub async fn build_unmanaged_channel( &self, From 56d96e4253eca8bad7ebf535d0dc09329bfb7f91 Mon Sep 17 00:00:00 2001 From: gabi-250 Date: Thu, 13 Jul 2023 15:47:05 +0000 Subject: [PATCH 4/5] Resolve numerous typos in `ChanMgr::build_unmanaged_channel` code --- crates/tor-chanmgr/src/lib.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/tor-chanmgr/src/lib.rs b/crates/tor-chanmgr/src/lib.rs index 737bbbe05..b23bf4cbc 100644 --- a/crates/tor-chanmgr/src/lib.rs +++ b/crates/tor-chanmgr/src/lib.rs @@ -106,12 +106,12 @@ pub struct ChanMgr { /// [`factory::ChannelFactory`] for each supported pluggable transport. /// This starts out as `None`, but can be replaced with [`ChanMgr::set_pt_mgr`]. /// The `TorClient` code currently sets this using `tor_ptmgr::PtMgr`. - /// `PtrMgr` currently returns `ChannelFactory` implementations that are + /// `PtMgr` currently returns `ChannelFactory` implementations that are /// built using [`transport::proxied::ExternalProxyPlugin`], which implements /// [`transport::TransportImplHelper`], which in turn is wrapped into a /// `ChanBuilder` to implement `ChannelFactory`. /// * A `dyn` [`factory::ChannelFactory`] that it uses for everything else - /// it uses for everything else. We instantiate this with a + /// We instantiate this with a /// [`builder::ChanBuilder`] using a [`transport::default::DefaultTransport`]. mgr: mgr::AbstractChanMgr, @@ -306,10 +306,10 @@ impl ChanMgr { /// Replace the transport registry with one that may know about /// more transports. /// - /// Note that the [`ChannelFactory`] instances returned by `ptrmgr` are + /// Note that the [`ChannelFactory`] instances returned by `ptmgr` are /// required to time-out channels that take too long to build. You'll get /// this behavior by default if the factories implement [`ChannelFactory`] using - /// [`transport::proxied::ExternalProxyPlugin`], which `tor-ptrmgr` does. + /// [`transport::proxied::ExternalProxyPlugin`], which `tor-ptmgr` does. #[cfg(feature = "pt-client")] pub fn set_pt_mgr(&self, ptmgr: Arc) { self.mgr.with_mut_builder(|f| f.replace_ptmgr(ptmgr)); From 157d134a65c8e01c861ebb917ede3fea83e7c7bc Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 13 Jul 2023 11:51:12 -0400 Subject: [PATCH 5/5] Explain better why you would use build_unmanaged_channel --- crates/tor-chanmgr/src/lib.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/crates/tor-chanmgr/src/lib.rs b/crates/tor-chanmgr/src/lib.rs index b23bf4cbc..41608aa53 100644 --- a/crates/tor-chanmgr/src/lib.rs +++ b/crates/tor-chanmgr/src/lib.rs @@ -319,8 +319,13 @@ impl ChanMgr { /// /// Unlike [`get_or_launch`](ChanMgr::get_or_launch), this function always /// creates a new channel, never retries transient failure, and does not - /// register this channel with the `ChanMgr`. Generally you should not use - /// it. + /// register this channel with the `ChanMgr`. + /// + /// Generally you should not use this function; `get_or_launch` is usually a + /// better choice. This function is the right choice if, for whatever + /// reason, you need to manage the lifetime of the channel you create, and + /// make sure that no other code with access to this `ChanMgr` will be able + /// to use the channel. #[cfg(feature = "experimental-api")] pub async fn build_unmanaged_channel( &self,