Merge branch 'share_state' (f3dc66d9) · Commits · eta / arti

crates/tor-circmgr/src/build.rs

+74 −65

Original line number	Diff line number	Diff line
		//! Facilities to build circuits directly, instead of via a circuit manager.

		use crate::path::{OwnedPath, TorPath};
		use crate::timeouts::{pareto::ParetoTimeoutEstimator, Action, TimeoutEstimator};
		use crate::timeouts::{self, Action};
		use crate::{Error, Result};
		use async_trait::async_trait;
		use futures::channel::oneshot;
		@@ -19,7 +19,6 @@ use tor_guardmgr::GuardStatus;
		use tor_linkspec::{ChanTarget, OwnedChanTarget, OwnedCircTarget};
		use tor_proto::circuit::{CircParameters, ClientCirc, PendingClientCirc};
		use tor_rtcompat::{Runtime, SleepProviderExt};
		use tracing::warn;

		mod guardstatus;

		@@ -130,30 +129,21 @@ impl Buildable for Arc<ClientCirc> {
		///
		/// In general, you should not need to construct or use this object yourself,
		/// unless you are choosing your own paths.
		struct Builder<
		R: Runtime,
		C: Buildable + Sync + Send + 'static,
		T: TimeoutEstimator + Send + Sync + 'static,
		> {
		struct Builder<R: Runtime, C: Buildable + Sync + Send + 'static> {
		/// The runtime used by this circuit builder.
		runtime: R,
		/// A channel manager that this circuit builder uses to make channels.
		chanmgr: Arc<ChanMgr<R>>,
		/// An estimator to determine the correct timeouts for circuit building.
		timeouts: T,
		timeouts: timeouts::Estimator,
		/// We don't actually hold any clientcircs, so we need to put this
		/// type here so the compiler won't freak out.
		_phantom: std::marker::PhantomData<C>,
		}

		impl<
		R: Runtime,
		C: Buildable + Sync + Send + 'static,
		T: TimeoutEstimator + Send + Sync + 'static,
		> Builder<R, C, T>
		{
		impl<R: Runtime, C: Buildable + Sync + Send + 'static> Builder<R, C> {
		/// Construct a new [`Builder`].
		fn new(runtime: R, chanmgr: Arc<ChanMgr<R>>, timeouts: T) -> Self {
		fn new(runtime: R, chanmgr: Arc<ChanMgr<R>>, timeouts: timeouts::Estimator) -> Self {
		Builder {
		runtime,
		chanmgr,
		@@ -271,7 +261,7 @@ impl<
		/// unless you are choosing your own paths.
		pub struct CircuitBuilder<R: Runtime> {
		/// The underlying [`Builder`] object
		builder: Arc<Builder<R, Arc<ClientCirc>, ParetoTimeoutEstimator>>,
		builder: Arc<Builder<R, Arc<ClientCirc>>>,
		/// Configuration for how to choose paths for circuits.
		path_config: crate::PathConfig,
		/// State-manager object to use in storing current state.
		@@ -292,14 +282,7 @@ impl<R: Runtime> CircuitBuilder<R> {
		storage: crate::TimeoutStateHandle,
		guardmgr: tor_guardmgr::GuardMgr<R>,
		) -> Self {
		let timeouts = match storage.load() {
		Ok(Some(v)) => ParetoTimeoutEstimator::from_state(v),
		Ok(None) => ParetoTimeoutEstimator::default(),
		Err(e) => {
		warn!("Unable to load timeout state: {}", e);
		ParetoTimeoutEstimator::default()
		}
		};
		let timeouts = timeouts::Estimator::from_storage(&storage);

		CircuitBuilder {
		builder: Arc::new(Builder::new(runtime, chanmgr, timeouts)),
		@@ -310,12 +293,31 @@ impl<R: Runtime> CircuitBuilder<R> {
		}

		/// Flush state to the state manager.
		pub fn save_state(&self) -> Result<()> {
		pub(crate) fn save_state(&self) -> Result<()> {
		// TODO: someday we'll want to only do this if there is something
		// changed.
		let _ignore = self.storage.try_lock()?; // XXXX don't ignore.
		let state = self.builder.timeouts.build_state();
		self.storage.store(&state)?;
		self.builder.timeouts.save_state(&self.storage)?;
		self.guardmgr.store_persistent_state()?;
		Ok(())
		}

		/// Replace our state with a new owning state, assuming we have
		/// storage permission.
		pub(crate) fn upgrade_to_owned_state(&self) -> Result<()> {
		self.builder
		.timeouts
		.upgrade_to_owning_storage(&self.storage);
		self.guardmgr.upgrade_to_owned_persistent_state()?;
		Ok(())
		}
		/// Reload persistent state from disk, if we don't have storage permission.
		pub(crate) fn reload_state(&self) -> Result<()> {
		if !self.storage.can_store() {
		self.builder
		.timeouts
		.reload_readonly_from_storage(&self.storage);
		}
		self.guardmgr.reload_persistent_state()?;
		Ok(())
		}

		@@ -323,7 +325,7 @@ impl<R: Runtime> CircuitBuilder<R> {
		///
		/// (NOTE: for now, this only affects circuit timeout estimation.)
		pub fn update_network_parameters(&self, p: &tor_netdir::params::NetParameters) {
		self.builder.timeouts.update_params(p.into());
		self.builder.timeouts.update_params(p);
		}

		/// DOCDOC
		@@ -422,6 +424,7 @@ where
		mod test {
		#![allow(clippy::unwrap_used)]
		use super::*;
		use crate::timeouts::TimeoutEstimator;
		use futures::channel::oneshot;
		use std::sync::atomic::{AtomicU64, Ordering::SeqCst};
		use std::sync::Mutex;
		@@ -603,24 +606,28 @@ mod test {
		}
		}
		impl TimeoutEstimator for Arc<Mutex<TimeoutRecorder>> {
		fn note_hop_completed(&self, hop: u8, delay: Duration, is_last: bool) {
		fn note_hop_completed(&mut self, hop: u8, delay: Duration, is_last: bool) {
		if !is_last {
		return;
		}

		let mut h = self.lock().unwrap();
		h.hist.push((true, hop, delay));
		let mut this = self.lock().unwrap();
		this.hist.push((true, hop, delay));
		}
		fn note_circ_timeout(&self, hop: u8, delay: Duration) {
		let mut h = self.lock().unwrap();
		h.hist.push((false, hop, delay));
		fn note_circ_timeout(&mut self, hop: u8, delay: Duration) {
		let mut this = self.lock().unwrap();
		this.hist.push((false, hop, delay));
		}
		fn timeouts(&self, _action: &Action) -> (Duration, Duration) {
		fn timeouts(&mut self, _action: &Action) -> (Duration, Duration) {
		(Duration::from_secs(3), Duration::from_secs(100))
		}
		fn learning_timeouts(&self) -> bool {
		false
		}
		fn update_params(&mut self, _params: &tor_netdir::params::NetParameters) {}

		fn build_state(&mut self) -> Option<crate::timeouts::pareto::ParetoTimeoutState> {
		None
		}
		}

		/// Testing only: create a bogus circuit target
		@@ -650,8 +657,11 @@ mod test {
		]);
		let chanmgr = Arc::new(ChanMgr::new(rt.clone()));
		let timeouts = Arc::new(Mutex::new(TimeoutRecorder::new()));
		let builder: Builder<_, Mutex<FakeCirc>, _> =
		Builder::new(rt.clone(), chanmgr, Arc::clone(&timeouts));
		let builder: Builder<_, Mutex<FakeCirc>> = Builder::new(
		rt.clone(),
		chanmgr,
		timeouts::Estimator::new(Arc::clone(&timeouts)),
		);
		let builder = Arc::new(builder);
		let rng =
		StdRng::from_rng(rand::thread_rng()).expect("couldn't construct temporary rng");
		@@ -678,15 +688,14 @@ mod test {
		);

		{
		let mut h = timeouts.lock().unwrap();
		assert_eq!(h.hist.len(), 2);
		assert!(h.hist[0].0); // completed
		assert_eq!(h.hist[0].1, 0); // last hop completed
		let timeouts = timeouts.lock().unwrap();
		assert_eq!(timeouts.hist.len(), 2);
		assert!(timeouts.hist[0].0); // completed
		assert_eq!(timeouts.hist[0].1, 0); // last hop completed
		// TODO: test time elapsed, once wait_for is more reliable.
		assert!(h.hist[1].0); // completed
		assert_eq!(h.hist[1].1, 2); // last hop completed
		assert!(timeouts.hist[1].0); // completed
		assert_eq!(timeouts.hist[1].1, 2); // last hop completed
		// TODO: test time elapsed, once wait_for is more reliable.
		h.hist.clear();
		}

		// Try a very long timeout.
		@@ -701,11 +710,10 @@ mod test {
		assert!(outcome.is_err());

		{
		let mut h = timeouts.lock().unwrap();
		assert_eq!(h.hist.len(), 1);
		assert!(!h.hist[0].0);
		assert_eq!(h.hist[0].1, 2);
		h.hist.clear();
		let timeouts = timeouts.lock().unwrap();
		assert_eq!(timeouts.hist.len(), 3);
		assert!(!timeouts.hist[2].0);
		assert_eq!(timeouts.hist[2].1, 2);
		}

		// Now try a recordable timeout.
		@@ -722,26 +730,27 @@ mod test {
		rt.advance(Duration::from_millis(100)).await;
		}
		{
		let h = timeouts.lock().unwrap();
		dbg!(&h.hist);
		let timeouts = timeouts.lock().unwrap();
		dbg!(&timeouts.hist);
		assert!(timeouts.hist.len() >= 4);
		// First we notice a circuit timeout after 2 hops
		assert!(!h.hist[0].0);
		assert_eq!(h.hist[0].1, 2);
		assert!(!timeouts.hist[3].0);
		assert_eq!(timeouts.hist[3].1, 2);
		// TODO: check timeout more closely.
		assert!(h.hist[0].2 < Duration::from_secs(100));
		assert!(h.hist[0].2 >= Duration::from_secs(3));
		assert!(timeouts.hist[3].2 < Duration::from_secs(100));
		assert!(timeouts.hist[3].2 >= Duration::from_secs(3));

		// This test is not reliable under test coverage; see arti#149.
		#[cfg(not(tarpaulin))]
		{
		assert_eq!(h.hist.len(), 2);
		assert_eq!(timeouts.hist.len(), 5);
		// Then we notice a circuit completing at its third hop.
		assert!(h.hist[1].0);
		assert_eq!(h.hist[1].1, 2);
		assert!(timeouts.hist[4].0);
		assert_eq!(timeouts.hist[4].1, 2);
		// TODO: check timeout more closely.
		assert!(h.hist[1].2 < Duration::from_secs(100));
		assert!(h.hist[1].2 >= Duration::from_secs(5));
		assert!(h.hist[0].2 < h.hist[1].2);
		assert!(timeouts.hist[4].2 < Duration::from_secs(100));
		assert!(timeouts.hist[4].2 >= Duration::from_secs(5));
		assert!(timeouts.hist[3].2 < timeouts.hist[4].2);
		}
		}
		HOP3_DELAY.store(300, SeqCst); // undo previous run.

crates/tor-circmgr/src/err.rs

+10 −1

Original line number	Diff line number	Diff line
		@@ -60,7 +60,7 @@ pub enum Error {

		/// Problem creating or updating a guard manager.
		#[error("Problem creating or updating guards list: {0}")]
		GuardMgr(#[from] tor_guardmgr::GuardMgrError),
		GuardMgr(#[source] tor_guardmgr::GuardMgrError),

		/// Problem selecting a guard relay.
		#[error("Unable to select a guard relay: {0}")]
		@@ -84,3 +84,12 @@ impl From<tor_rtcompat::TimeoutError> for Error {
		Error::CircTimeout
		}
		}

		impl From<tor_guardmgr::GuardMgrError> for Error {
		fn from(err: tor_guardmgr::GuardMgrError) -> Error {
		match err {
		tor_guardmgr::GuardMgrError::State(e) => Error::State(e),
		_ => Error::GuardMgr(err),
		}
		}
		}

crates/tor-circmgr/src/lib.rs

+28 −9

Original line number	Diff line number	Diff line
		@@ -174,10 +174,15 @@ impl<R: Runtime> CircMgr<R> {

		let guardmgr = tor_guardmgr::GuardMgr::new(runtime.clone(), storage.clone())?;

		let storage = storage.create_handle(PARETO_TIMEOUT_DATA_KEY);
		let storage_handle = storage.create_handle(PARETO_TIMEOUT_DATA_KEY);

		let builder =
		build::CircuitBuilder::new(runtime.clone(), chanmgr, path_config, storage, guardmgr);
		let builder = build::CircuitBuilder::new(
		runtime.clone(),
		chanmgr,
		path_config,
		storage_handle,
		guardmgr,
		);
		let mgr =
		mgr::AbstractCircMgr::new(builder, runtime.clone(), request_timing, circuit_timing);
		let circmgr = Arc::new(CircMgr { mgr: Arc::new(mgr) });
		@@ -190,13 +195,27 @@ impl<R: Runtime> CircMgr<R> {
		Ok(circmgr)
		}

		/// Flush state to the state manager, if there is any unsaved state.
		pub fn update_persistent_state(&self) -> Result<()> {
		/// Reload state from the state manager.
		///
		/// We only call this method if we _don't_ have the lock on the state
		/// files. If we have the lock, we only want to save.
		pub fn reload_persistent_state(&self) -> Result<()> {
		self.mgr.peek_builder().reload_state()?;
		Ok(())
		}

		/// Switch from having an unowned persistent state to having an owned one.
		///
		/// Requires that we hold the lock on the state files.
		pub fn upgrade_to_owned_persistent_state(&self) -> Result<()> {
		self.mgr.peek_builder().upgrade_to_owned_state()?;
		Ok(())
		}

		/// Flush state to the state manager, if there is any unsaved state and
		/// we have the lock.
		pub fn store_persistent_state(&self) -> Result<()> {
		self.mgr.peek_builder().save_state()?;
		self.mgr
		.peek_builder()
		.guardmgr()
		.update_persistent_state()?;
		Ok(())
		}

crates/tor-circmgr/src/timeouts.rs

+17 −3

Original line number	Diff line number	Diff line
		@@ -10,7 +10,11 @@

		use std::time::Duration;

		pub(crate) mod estimator;
		pub(crate) mod pareto;
		pub(crate) mod readonly;

		pub(crate) use estimator::Estimator;

		/// An object that calculates circuit timeout thresholds from the history
		/// of circuit build times.
		@@ -23,7 +27,7 @@ pub(crate) trait TimeoutEstimator {
		/// circuit.
		///
		/// If this is the last hop of the circuit, then `is_last` is true.
		fn note_hop_completed(&self, hop: u8, delay: Duration, is_last: bool);
		fn note_hop_completed(&mut self, hop: u8, delay: Duration, is_last: bool);

		/// Record that a circuit failed to complete because it took too long.
		///
		@@ -32,7 +36,7 @@ pub(crate) trait TimeoutEstimator {
		///
		/// The `delay` number is the amount of time after we first launched the
		/// circuit.
		fn note_circ_timeout(&self, hop: u8, delay: Duration);
		fn note_circ_timeout(&mut self, hop: u8, delay: Duration);

		/// Return the current estimation for how long we should wait for a given
		/// [`Action`] to complete.
		@@ -43,11 +47,21 @@ pub(crate) trait TimeoutEstimator {
		/// building it in order see how long it takes. After `abandon`
		/// has elapsed since circuit launch, the circuit should be
		/// abandoned completely.
		fn timeouts(&self, action: &Action) -> (Duration, Duration);
		fn timeouts(&mut self, action: &Action) -> (Duration, Duration);

		/// Return true if we're currently trying to learn more timeouts
		/// by launching testing circuits.
		fn learning_timeouts(&self) -> bool;

		/// Replace the network parameters used by this estimator (if any)
		/// with ones derived from `params`.
		fn update_params(&mut self, params: &tor_netdir::params::NetParameters);

		/// Construct a new ParetoTimeoutState to represent the current state
		/// of this estimator, if it is possible to store the state to disk.
		///
		/// TODO: change the type used for the state.
		fn build_state(&mut self) -> Option<pareto::ParetoTimeoutState>;
		}

		/// A possible action for which we can try to estimate a timeout.

crates/tor-circmgr/src/timeouts/estimator.rs

0 → 100644

+150 −0

Original line number	Diff line number	Diff line
		//! Declarations for a [`TimeoutEstimator`] type that can change implementation.

		use crate::timeouts::{
		pareto::{ParetoTimeoutEstimator, ParetoTimeoutState},
		readonly::ReadonlyTimeoutEstimator,
		Action, TimeoutEstimator,
		};
		use crate::TimeoutStateHandle;
		use std::sync::Mutex;
		use std::time::Duration;
		use tor_netdir::params::NetParameters;
		use tracing::{debug, warn};

		/// A timeout estimator that can change its inner implementation and share its
		/// implementation among multiple threads.
		pub(crate) struct Estimator {
		/// The estimator we're currently using.
		inner: Mutex<Box<dyn TimeoutEstimator + Send + 'static>>,
		}

		impl Estimator {
		/// Construct a new estimator from some variant.
		pub(crate) fn new(est: impl TimeoutEstimator + Send + 'static) -> Self {
		Self {
		inner: Mutex::new(Box::new(est)),
		}
		}

		/// Create this estimator based on the values stored in `storage`, and whether
		/// this storage is read-only.
		pub(crate) fn from_storage(storage: &TimeoutStateHandle) -> Self {
		let (_, est) = estimator_from_storage(storage);
		Self {
		inner: Mutex::new(est),
		}
		}

		/// Assuming that we can read and write to `storage`, replace our state with
		/// a new state that estimates timeouts.
		pub(crate) fn upgrade_to_owning_storage(&self, storage: &TimeoutStateHandle) {
		let (readonly, est) = estimator_from_storage(storage);
		if readonly {
		warn!("Unable to upgrade to owned persistent storage.");
		return;
		}
		*self.inner.lock().expect("Timeout estimator lock poisoned") = est;
		}

		/// Replace the contents of this estimator with a read-only state estimator
		/// based on the contents of `storage`.
		pub(crate) fn reload_readonly_from_storage(&self, storage: &TimeoutStateHandle) {
		if let Ok(Some(v)) = storage.load() {
		let est = ReadonlyTimeoutEstimator::from_state(&v);
		*self.inner.lock().expect("Timeout estimator lock poisoned") = Box::new(est);
		} else {
		debug!("Unable to reload timeout state.")
		}
		}

		/// Record that a given circuit hop has completed.
		///
		/// The `hop` number is a zero-indexed value for which hop just completed.
		///
		/// The `delay` value is the amount of time after we first launched the
		/// circuit.
		///
		/// If this is the last hop of the circuit, then `is_last` is true.
		pub(crate) fn note_hop_completed(&self, hop: u8, delay: Duration, is_last: bool) {
		let mut inner = self.inner.lock().expect("Timeout estimator lock poisoned.");

		inner.note_hop_completed(hop, delay, is_last);
		}

		/// Record that a circuit failed to complete because it took too long.
		///
		/// The `hop` number is a the number of hops that were successfully
		/// completed.
		///
		/// The `delay` number is the amount of time after we first launched the
		/// circuit.
		pub(crate) fn note_circ_timeout(&self, hop: u8, delay: Duration) {
		let mut inner = self.inner.lock().expect("Timeout estimator lock poisoned.");
		inner.note_circ_timeout(hop, delay);
		}

		/// Return the current estimation for how long we should wait for a given
		/// [`Action`] to complete.
		///
		/// This function should return a 2-tuple of `(timeout, abandon)`
		/// durations. After `timeout` has elapsed since circuit launch,
		/// the circuit should no longer be used, but we should still keep
		/// building it in order see how long it takes. After `abandon`
		/// has elapsed since circuit launch, the circuit should be
		/// abandoned completely.
		pub(crate) fn timeouts(&self, action: &Action) -> (Duration, Duration) {
		let mut inner = self.inner.lock().expect("Timeout estimator lock poisoned.");

		inner.timeouts(action)
		}

		/// Return true if we're currently trying to learn more timeouts
		/// by launching testing circuits.
		pub(crate) fn learning_timeouts(&self) -> bool {
		let inner = self.inner.lock().expect("Timeout estimator lock poisoned.");
		inner.learning_timeouts()
		}

		/// Replace the network parameters used by this estimator (if any)
		/// with ones derived from `params`.
		pub(crate) fn update_params(&self, params: &NetParameters) {
		let mut inner = self.inner.lock().expect("Timeout estimator lock poisoned.");
		inner.update_params(params);
		}

		/// Store any state associated with this timeout esimator into `storage`.
		pub(crate) fn save_state(&self, storage: &TimeoutStateHandle) -> crate::Result<()> {
		let state = {
		let mut inner = self.inner.lock().expect("Timeout estimator lock poisoned.");
		inner.build_state()
		};
		if let Some(state) = state {
		storage.store(&state)?;
		}
		Ok(())
		}
		}

		/// Try to construct a new boxed TimeoutEstimator based on the contents of
		/// storage, and whether it is read-only.
		///
		/// Returns true on a read-only state.
		fn estimator_from_storage(
		storage: &TimeoutStateHandle,
		) -> (bool, Box<dyn TimeoutEstimator + Send + 'static>) {
		let state = match storage.load() {
		Ok(Some(v)) => v,
		Ok(None) => ParetoTimeoutState::default(),
		Err(e) => {
		warn!("Unable to load timeout state: {}", e);
		return (true, Box::new(ReadonlyTimeoutEstimator::new()));
		}
		};

		if storage.can_store() {
		// We own the lock, so we're going to use a full estimator.
		(false, Box::new(ParetoTimeoutEstimator::from_state(state)))
		} else {
		(true, Box::new(ReadonlyTimeoutEstimator::from_state(&state)))
		}
		}