From d592e86f9c48e6a9d577c23c7a28c1720028680c Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 18 Nov 2021 09:44:42 -0500 Subject: [PATCH] Fold "circuit_timing" and "request_timing" into a single section. --- crates/arti-client/src/config.rs | 2 +- crates/arti-config/src/arti_defaults.toml | 6 +-- crates/arti-config/src/options.rs | 6 +-- crates/tor-circmgr/src/config.rs | 62 ++++++++--------------- crates/tor-circmgr/src/lib.rs | 6 +-- crates/tor-circmgr/src/mgr.rs | 37 +++----------- 6 files changed, 33 insertions(+), 86 deletions(-) diff --git a/crates/arti-client/src/config.rs b/crates/arti-client/src/config.rs index 43f6f296c..0a3caf2fd 100644 --- a/crates/arti-client/src/config.rs +++ b/crates/arti-client/src/config.rs @@ -13,7 +13,7 @@ pub use tor_config::ConfigBuildError; pub mod circ { pub use tor_circmgr::{ CircMgrConfig, CircMgrConfigBuilder, CircuitTiming, CircuitTimingBuilder, PathConfig, - PathConfigBuilder, RequestTiming, RequestTimingBuilder, + PathConfigBuilder, }; } diff --git a/crates/arti-config/src/arti_defaults.toml b/crates/arti-config/src/arti_defaults.toml index 25836f24d..592178738 100644 --- a/crates/arti-config/src/arti_defaults.toml +++ b/crates/arti-config/src/arti_defaults.toml @@ -82,16 +82,14 @@ microdesc_parallelism = 4 # IPv4 addresses begin with the same 16 bits. enforce_distance = { "subnets_family_v4" = 16, "subnets_family_v6" = 32 } -# Rules for how long circuits should survive. +# Rules for how long circuits should survive, and how long pending +# requests should wait for a circuit. [circuit_timing] # Once a circuit has been used for a request, we stop giving it out for # other requests after this time. max_dirtiness = "10 minutes" -# Rules for how long pending requests should wait for a circuit. -[request_timing] - # When a circuit is requested, we keep trying to build circuits for up # to this long before the request gives up. request_timeout = "60 sec" diff --git a/crates/arti-config/src/options.rs b/crates/arti-config/src/options.rs index 14c5a0225..68eb022eb 100644 --- a/crates/arti-config/src/options.rs +++ b/crates/arti-config/src/options.rs @@ -71,10 +71,7 @@ pub struct ArtiConfig { /// Information about how to build paths through the network. path_rules: arti_client::config::circ::PathConfig, - /// Information about how to retry requests for circuits. - request_timing: arti_client::config::circ::RequestTiming, - - /// Information about how to expire circuits. + /// Information about how to retry and expire circuits and request for circuits. circuit_timing: arti_client::config::circ::CircuitTiming, /// Information about client address configuration parameters. @@ -128,7 +125,6 @@ impl ArtiConfig { let mut builder = CircMgrConfigBuilder::default(); builder .path_config(self.path_rules.clone()) - .request_timing(self.request_timing.clone()) .circuit_timing(self.circuit_timing.clone()) .build() } diff --git a/crates/tor-circmgr/src/config.rs b/crates/tor-circmgr/src/config.rs index 19041fc4d..ed8e807a3 100644 --- a/crates/tor-circmgr/src/config.rs +++ b/crates/tor-circmgr/src/config.rs @@ -11,44 +11,6 @@ use serde::Deserialize; use std::time::Duration; -/// Configuration for circuit timeouts and retries. -/// -/// This type is immutable once constructed. To create an object of this type, -/// use [`RequestTimingBuilder`], or deserialize it from a string. -#[derive(Debug, Clone, Builder, Deserialize)] -#[builder(build_fn(error = "ConfigBuildError"))] -pub struct RequestTiming { - /// When a circuit is requested, we stop retrying new circuits - /// after this much time. - // TODO: Impose a maximum or minimum? - #[builder(default = "Duration::from_secs(60)")] - #[serde(with = "humantime_serde")] - pub(crate) request_timeout: Duration, - - /// When a circuit is requested, we stop retrying new circuits after - /// this many attempts. - // TODO: Impose a maximum or minimum? - #[builder(default = "32")] - pub(crate) request_max_retries: u32, - - /// When waiting for requested circuits, wait at least this long - /// before using a suitable-looking circuit launched by some other - /// request. - #[builder(default = "Duration::from_millis(50)")] - #[serde(with = "humantime_serde")] - pub(crate) request_loyalty: Duration, -} - -// NOTE: it seems that `unwrap` may be safe because of builder defaults -// check `derive_builder` documentation for details -// https://docs.rs/derive_builder/0.10.2/derive_builder/#default-values -#[allow(clippy::unwrap_used)] -impl Default for RequestTiming { - fn default() -> Self { - RequestTimingBuilder::default().build().unwrap() - } -} - /// Rules for building paths over the network. /// /// This type is immutable once constructed. To build one, use @@ -74,6 +36,26 @@ pub struct CircuitTiming { #[builder(default = "Duration::from_secs(60 * 10)")] #[serde(with = "humantime_serde")] pub(crate) max_dirtiness: Duration, + + /// When a circuit is requested, we stop retrying new circuits + /// after this much time. + // TODO: Impose a maximum or minimum? + #[builder(default = "Duration::from_secs(60)")] + #[serde(with = "humantime_serde")] + pub(crate) request_timeout: Duration, + + /// When a circuit is requested, we stop retrying new circuits after + /// this many attempts. + // TODO: Impose a maximum or minimum? + #[builder(default = "32")] + pub(crate) request_max_retries: u32, + + /// When waiting for requested circuits, wait at least this long + /// before using a suitable-looking circuit launched by some other + /// request. + #[builder(default = "Duration::from_millis(50)")] + #[serde(with = "humantime_serde")] + pub(crate) request_loyalty: Duration, } // NOTE: it seems that `unwrap` may be safe because of builder defaults @@ -104,10 +86,6 @@ pub struct CircMgrConfig { #[builder(default)] pub(crate) path_config: PathConfig, - /// Timing and retry information related to requests for circuits. - #[builder(default)] - pub(crate) request_timing: RequestTiming, - /// Timing and retry information related to circuits themselves. #[builder(default)] pub(crate) circuit_timing: CircuitTiming, diff --git a/crates/tor-circmgr/src/lib.rs b/crates/tor-circmgr/src/lib.rs index 240f19722..0ef4dbbb3 100644 --- a/crates/tor-circmgr/src/lib.rs +++ b/crates/tor-circmgr/src/lib.rs @@ -74,7 +74,7 @@ pub use usage::{IsolationToken, StreamIsolation, StreamIsolationBuilder, TargetP pub use config::{ CircMgrConfig, CircMgrConfigBuilder, CircuitTiming, CircuitTimingBuilder, PathConfig, - PathConfigBuilder, RequestTiming, RequestTimingBuilder, + PathConfigBuilder, }; use usage::TargetCircUsage; @@ -164,7 +164,6 @@ impl CircMgr { { let CircMgrConfig { path_config, - request_timing, circuit_timing, } = config; @@ -179,8 +178,7 @@ impl CircMgr { storage_handle, guardmgr, ); - let mgr = - mgr::AbstractCircMgr::new(builder, runtime.clone(), request_timing, circuit_timing); + let mgr = mgr::AbstractCircMgr::new(builder, runtime.clone(), circuit_timing); let circmgr = Arc::new(CircMgr { mgr: Arc::new(mgr) }); runtime.spawn(continually_expire_circuits( diff --git a/crates/tor-circmgr/src/mgr.rs b/crates/tor-circmgr/src/mgr.rs index f22251246..308ad7cd6 100644 --- a/crates/tor-circmgr/src/mgr.rs +++ b/crates/tor-circmgr/src/mgr.rs @@ -22,7 +22,7 @@ // - Error from prepare_action() // - Error reported by restrict_mut? -use crate::config::{CircuitTiming, RequestTiming}; +use crate::config::CircuitTiming; use crate::{DirInfo, Error, Result}; use retry_error::RetryError; @@ -622,9 +622,7 @@ pub(crate) struct AbstractCircMgr { /// pending circuits. circs: sync::Mutex>, - /// Configured timing and retry rules for attaching requests to circuits. - request_timing: RequestTiming, - /// Configured information about when to expire circuits. + /// Configured information about when to expire circuits and requests. circuit_timing: CircuitTiming, /// Minimum lifetime of an unused circuit. @@ -645,12 +643,7 @@ enum Action { impl AbstractCircMgr { /// Construct a new AbstractCircMgr. - pub(crate) fn new( - builder: B, - runtime: R, - request_timing: RequestTiming, - circuit_timing: CircuitTiming, - ) -> Self { + pub(crate) fn new(builder: B, runtime: R, circuit_timing: CircuitTiming) -> Self { let circs = sync::Mutex::new(CircList::new()); let dflt_params = tor_netdir::params::NetParameters::default(); let unused_timing = (&dflt_params).into(); @@ -658,7 +651,6 @@ impl AbstractCircMgr { builder, runtime, circs, - request_timing, circuit_timing, unused_timing: sync::Mutex::new(unused_timing), } @@ -683,9 +675,9 @@ impl AbstractCircMgr { usage: &::Usage, dir: DirInfo<'_>, ) -> Result> { - let wait_for_circ = self.request_timing.request_timeout; + let wait_for_circ = self.circuit_timing.request_timeout; let timeout_at = self.runtime.now() + wait_for_circ; - let max_tries = self.request_timing.request_max_retries; + let max_tries = self.circuit_timing.request_max_retries; let mut retry_err = RetryError /* ::> */::in_attempt_to("find or build a circuit"); @@ -1035,7 +1027,7 @@ impl AbstractCircMgr { // delay will give the circuits that were originally // specifically intended for a request a little more time // to finish, before we offer it this circuit instead. - let briefly = self.request_timing.request_loyalty; + let briefly = self.circuit_timing.request_loyalty; let sl = runtime_copy.sleep(briefly); runtime_copy.allow_one_advance(briefly); sl.await; @@ -1345,7 +1337,6 @@ mod test { let mgr = Arc::new(AbstractCircMgr::new( builder, rt.clone(), - RequestTiming::default(), CircuitTiming::default(), )); @@ -1428,7 +1419,6 @@ mod test { let mgr = Arc::new(AbstractCircMgr::new( builder, rt.clone(), - RequestTiming::default(), CircuitTiming::default(), )); let c1 = mgr @@ -1461,7 +1451,6 @@ mod test { let mgr = Arc::new(AbstractCircMgr::new( builder, rt.clone(), - RequestTiming::default(), CircuitTiming::default(), )); let c1 = mgr @@ -1487,7 +1476,6 @@ mod test { let mgr = Arc::new(AbstractCircMgr::new( builder, rt.clone(), - RequestTiming::default(), CircuitTiming::default(), )); let c1 = rt.wait_for(mgr.get_or_launch(&ports, di())).await; @@ -1509,7 +1497,6 @@ mod test { let mgr = Arc::new(AbstractCircMgr::new( builder, rt.clone(), - RequestTiming::default(), CircuitTiming::default(), )); let c1 = rt.wait_for(mgr.get_or_launch(&ports, di())).await; @@ -1536,7 +1523,6 @@ mod test { let mgr = Arc::new(AbstractCircMgr::new( builder, rt.clone(), - RequestTiming::default(), CircuitTiming::default(), )); let c1 = rt.wait_for(mgr.get_or_launch(&ports, di())).await; @@ -1560,7 +1546,6 @@ mod test { let mgr = Arc::new(AbstractCircMgr::new( builder, rt.clone(), - RequestTiming::default(), CircuitTiming::default(), )); @@ -1589,7 +1574,6 @@ mod test { let mgr = Arc::new(AbstractCircMgr::new( builder, rt.clone(), - RequestTiming::default(), CircuitTiming::default(), )); @@ -1657,7 +1641,6 @@ mod test { let mgr = Arc::new(AbstractCircMgr::new( builder, rt.clone(), - RequestTiming::default(), CircuitTiming::default(), )); // Note that ports2 will be wider than ports1, so the second @@ -1692,7 +1675,6 @@ mod test { let mgr = Arc::new(AbstractCircMgr::new( builder, rt.clone(), - RequestTiming::default(), CircuitTiming::default(), )); @@ -1738,12 +1720,7 @@ mod test { .build() .unwrap(); - let mgr = Arc::new(AbstractCircMgr::new( - builder, - rt.clone(), - RequestTiming::default(), - circuit_timing, - )); + let mgr = Arc::new(AbstractCircMgr::new(builder, rt.clone(), circuit_timing)); let imap = FakeSpec::new(vec![993_u16]); let pop = FakeSpec::new(vec![995_u16]);