Merge branch 'abstract-pt-mgr' into 'main'

Replace TransportRegistry with AbstractPtMgr

See merge request tpo/core/arti!887

crates/tor-chanmgr/src/err.rs

@@ -6,6 +6,7 @@ use std::sync::Arc;
 use futures::task::SpawnError;
 use thiserror::Error;
 
+use crate::factory::AbstractPtError;
 use tor_error::{internal, ErrorKind};
 use tor_linkspec::{ChanTarget, OwnedChanTarget, PtTargetAddr};
 use tor_proto::ClockSkew;
@@ -107,6 +108,15 @@ pub enum Error {
     #[error("Problem while connecting to Tor via a proxy")]
     Proxy(#[from] ProxyError),
 
+    /// An error occurred in a pluggable transport manager.
+    ///
+    /// We can't know the type, because any pluggable transport manager implementing
+    /// `AbstractPtMgr` can be used.
+    /// However, if you're using Arti in the standard configuration, this will be
+    /// `tor-ptmgr`'s `PtError`.
+    #[error("Pluggable transport error: {0}")]
+    Pt(#[source] Arc<dyn AbstractPtError>),
+
     /// An internal error of some kind that should never occur.
     #[error("Internal error")]
     Internal(#[from] tor_error::Bug),
@@ -146,6 +156,7 @@ impl tor_error::HasKind for Error {
             E::ChannelBuild { .. } => EK::TorAccessFailed,
             E::RequestCancelled => EK::TransientFailure,
             E::Proxy(e) => e.kind(),
+            E::Pt(e) => e.kind(),
         }
     }
 }
@@ -166,6 +177,7 @@ impl tor_error::HasRetryTime for Error {
 
             // Delegate.
             E::Proxy(e) => e.retry_time(),
+            E::Pt(e) => e.retry_time(),
 
             // This error reflects multiple attempts, but every failure is an IO
             // error, so we can also retry this after a delay.

crates/tor-chanmgr/src/factory.rs

@@ -1,15 +1,12 @@
 //! Traits and code to define different mechanisms for building Channels to
 //! different kinds of targets.
 
-pub(crate) mod registry;
-
 use std::sync::Arc;
 
 use async_trait::async_trait;
-use tor_linkspec::OwnedChanTarget;
+use tor_error::{HasKind, HasRetryTime};
+use tor_linkspec::{OwnedChanTarget, PtTransportName};
 use tor_proto::channel::Channel;
-
-pub use registry::TransportRegistry;
 use tracing::debug;
 
 /// An object that knows how to build `Channels` to `ChanTarget`s.
@@ -63,3 +60,19 @@ where
         self.connect_via_transport(target).await
     }
 }
+
+/// The error type returned by a pluggable transport manager.
+pub trait AbstractPtError: std::error::Error + HasKind + HasRetryTime + Send + Sync {}
+
+/// A pluggable transport manager.
+///
+/// We can't directly reference the `PtMgr` type from `tor-ptmgr`, because of dependency resolution
+/// constraints, so this defines the interface for what one should look like.
+#[async_trait]
+pub trait AbstractPtMgr {
+    /// Get a `ChannelFactory` for the provided `PtTransportName`.
+    async fn factory_for_transport(
+        &self,
+        transport: &PtTransportName,
+    ) -> Result<Option<Arc<dyn ChannelFactory + Sync>>, Arc<dyn AbstractPtError>>;
+}

crates/tor-chanmgr/src/factory/registry.rs

@@ -1,34 +0,0 @@
-//! Implement a registry for different kinds of transports.
-
-use async_trait::async_trait;
-use tor_linkspec::{HasChanMethod, OwnedChanTarget, TransportId};
-use tor_proto::channel::Channel;
-
-use crate::Error;
-
-use super::ChannelFactory;
-
-/// An object that knows about one or more [`ChannelFactory`]s.
-///
-/// It can be used itself as a `ChannelFactory`, to open connections to a given
-/// channel target depending on its configured [`TransportId`].
-//
-// TODO pt-client: Turn this into a concrete type?
-pub trait TransportRegistry {
-    /// Return a ChannelFactory that can make connections via a chosen
-    /// transport, if we know one.
-    //
-    // TODO pt-client: This might need to return an Arc instead of a reference
-    fn get_factory(&self, transport: &TransportId) -> Option<&(dyn ChannelFactory + Sync)>;
-}
-
-#[async_trait]
-impl<R: TransportRegistry + Sync> ChannelFactory for R {
-    async fn connect_via_transport(&self, target: &OwnedChanTarget) -> crate::Result<Channel> {
-        let method = target.chan_method();
-        let id = method.transport_id();
-        let factory = self.get_factory(&id).ok_or(Error::NoSuchTransport(id))?;
-
-        factory.connect_via_transport(target).await
-    }
-}

crates/tor-chanmgr/src/lib.rs

@@ -280,14 +280,14 @@ impl<R: Runtime> ChanMgr<R> {
         todo!("TODO pt-client: implement this.")
     }
 
+    /*
+    TODO pt-client: use AbstractPtMgr instead
     /// Replace the transport registry with one that may know about
     /// more transports.
     #[cfg(feature = "pt-client")]
     pub fn set_transport_registry(&self, _registry: impl factory::TransportRegistry) {
-        // TODO pt-client: See set_default_transport above.
-        #![allow(clippy::missing_panics_doc, clippy::needless_pass_by_value)]
-        todo!("TODO pt-client: implement this.")
     }
+     */
 
     /// Watch for things that ought to change the configuration of all channels in the client
     ///

crates/tor-ptmgr/src/lib.rs

@@ -58,6 +58,7 @@ use tor_rtcompat::Runtime;
 #[derive(Clone, Debug)]
 pub struct PtMgr<R> {
     /// An underlying `Runtime`, used to spawn background tasks.
+    #[allow(dead_code)]
     runtime: R,
 }
 
@@ -83,35 +84,3 @@ impl<R: Runtime> PtMgr<R> {
     // TODO pt-client: Possibly, this should have a separate function to launch
     // its background tasks.
 }
-
-#[cfg(feature = "tor-channel-factory")]
-#[allow(clippy::missing_panics_doc)]
-impl<R: Runtime> tor_chanmgr::factory::TransportRegistry for PtMgr<R> {
-    // There is going to be a lot happening "under the hood" here.
-    //
-    // When we are asked to get a ChannelFactory for a given
-    // connection, we will need to:
-    //    - launch the binary for that transport if it is not already running*.
-    //    - If we launched the binary, talk to it and see which ports it
-    //      is listening on.
-    //    - Return a ChannelFactory that connects via one of those ports,
-    //      using the appropriate version of SOCKS, passing K=V parameters
-    //      encoded properly.
-    //
-    // * As in other managers, we'll need to avoid trying to launch the same
-    //   transport twice if we get two concurrent requests.
-    //
-    // Later if the binary crashes, we should detect that.  We should relaunch
-    // it on demand.
-    //
-    // On reconfigure, we should shut down any no-longer-used transports.
-    //
-    // Maybe, we should shut down transports that haven't been used
-    // for a long time.
-
-    fn get_factory(&self, transport: &TransportId) -> Option<&(dyn ChannelFactory + Sync)> {
-        let _ = transport;
-        let _ = &self.runtime;
-        todo!("TODO pt-client")
-    }
-}