From 4fcb550aa613882d1a46eb5a3946840c4cf12c22 Mon Sep 17 00:00:00 2001 From: Henry de Valence Date: Mon, 13 Jan 2020 09:54:27 -0800 Subject: [PATCH] Fix a deadlock in TokioComponent. The components are accessed by a lock on application state. When some command calls block_on to enter an async context, it obtained a write lock on the entire application state. This meant that if the application state were accessed later in an async context, a deadlock would occur. Instead the TokioComponent holds an Option now, so that before calling block_on, the caller can .take() the runtime and release the lock. Since we only ever enter an async context once, it's not a problem that the component is then missing its runtime, as once we are inside of a task we can access the runtime. --- zebrad/src/application.rs | 2 +- zebrad/src/commands/connect.rs | 17 +++++++++------- zebrad/src/commands/seed.rs | 9 +++++++-- zebrad/src/commands/start.rs | 5 ++++- zebrad/src/components/tokio.rs | 10 ++++++++-- zebrad/src/components/tracing.rs | 34 ++++++++++++++++++-------------- zebrad/src/prelude.rs | 7 ------- 7 files changed, 49 insertions(+), 35 deletions(-) diff --git a/zebrad/src/application.rs b/zebrad/src/application.rs index c6d5fbf6a..576cdc417 100644 --- a/zebrad/src/application.rs +++ b/zebrad/src/application.rs @@ -3,7 +3,7 @@ use crate::{commands::ZebradCmd, config::ZebradConfig}; use abscissa_core::{ application::{self, AppCell}, - config, trace, Application, Component, EntryPoint, FrameworkError, StandardPaths, + config, trace, Application, EntryPoint, FrameworkError, StandardPaths, }; /// Application state diff --git a/zebrad/src/commands/connect.rs b/zebrad/src/commands/connect.rs index c6b713d2e..038f6011c 100644 --- a/zebrad/src/commands/connect.rs +++ b/zebrad/src/commands/connect.rs @@ -26,13 +26,18 @@ impl Runnable for ConnectCmd { info!(connect.addr = ?self.addr); use crate::components::tokio::TokioComponent; - let _ = app_writer() + let rt = app_writer() .state_mut() .components .get_downcast_mut::() .expect("TokioComponent should be available") .rt - .block_on(self.connect()); + .take(); + + rt.expect("runtime should not already be taken") + .block_on(self.connect()) + // Surface any error that occurred executing the future. + .unwrap(); } } @@ -44,11 +49,9 @@ impl ConnectCmd { use tower::{buffer::Buffer, service_fn, Service, ServiceExt}; let node = Buffer::new( - service_fn(|req| { - async move { - info!(?req); - Ok::(Response::Ok) - } + service_fn(|req| async move { + info!(?req); + Ok::(Response::Ok) }), 1, ); diff --git a/zebrad/src/commands/seed.rs b/zebrad/src/commands/seed.rs index 3520c7317..1c12cbeb5 100644 --- a/zebrad/src/commands/seed.rs +++ b/zebrad/src/commands/seed.rs @@ -112,13 +112,18 @@ impl Runnable for SeedCmd { fn run(&self) { use crate::components::tokio::TokioComponent; - let _ = app_writer() + let rt = app_writer() .state_mut() .components .get_downcast_mut::() .expect("TokioComponent should be available") .rt - .block_on(self.seed()); + .take(); + + rt.expect("runtime should not already be taken") + .block_on(self.seed()) + // Surface any error that occurred executing the future. + .unwrap(); } } diff --git a/zebrad/src/commands/start.rs b/zebrad/src/commands/start.rs index ec8a39c9b..4f6ebd263 100644 --- a/zebrad/src/commands/start.rs +++ b/zebrad/src/commands/start.rs @@ -40,12 +40,15 @@ impl Runnable for StartCmd { use crate::components::tokio::TokioComponent; - app_writer() + let rt = app_writer() .state_mut() .components .get_downcast_mut::() .expect("TokioComponent should be available") .rt + .take(); + + rt.expect("runtime should not already be taken") .block_on(future::pending::<()>()); } } diff --git a/zebrad/src/components/tokio.rs b/zebrad/src/components/tokio.rs index 0487585e6..059368ade 100644 --- a/zebrad/src/components/tokio.rs +++ b/zebrad/src/components/tokio.rs @@ -5,15 +5,21 @@ use abscissa_core::{Component, FrameworkError}; use tokio::runtime::Runtime; /// An Abscissa component which owns a Tokio runtime. +/// +/// The runtime is stored as an `Option` so that when it's time to enter an async +/// context by calling `block_on` with a "root future", the runtime can be taken +/// independently of Abscissa's component locking system. Otherwise whatever +/// calls `block_on` holds an application lock for the entire lifetime of the +/// async context. #[derive(Component, Debug)] pub struct TokioComponent { - pub rt: Runtime, + pub rt: Option, } impl TokioComponent { pub fn new() -> Result { Ok(Self { - rt: Runtime::new().unwrap(), + rt: Some(Runtime::new().unwrap()), }) } } diff --git a/zebrad/src/components/tracing.rs b/zebrad/src/components/tracing.rs index 228d54723..260db8843 100644 --- a/zebrad/src/components/tracing.rs +++ b/zebrad/src/components/tracing.rs @@ -47,23 +47,27 @@ impl TracingEndpoint { .parse() .expect("Hardcoded address should be parseable"); - tokio_component.rt.spawn(async move { - // try_bind uses the tokio runtime, so we - // need to construct it inside the task. - let server = match Server::try_bind(&addr) { - Ok(s) => s, - Err(e) => { - error!("Could not open tracing endpoint listener"); - error!("Error: {}", e); - return; + tokio_component + .rt + .as_ref() + .expect("runtime should not be taken") + .spawn(async move { + // try_bind uses the tokio runtime, so we + // need to construct it inside the task. + let server = match Server::try_bind(&addr) { + Ok(s) => s, + Err(e) => { + error!("Could not open tracing endpoint listener"); + error!("Error: {}", e); + return; + } } - } - .serve(service); + .serve(service); - if let Err(e) = server.await { - error!("Server error: {}", e); - } - }); + if let Err(e) = server.await { + error!("Server error: {}", e); + } + }); Ok(()) } diff --git a/zebrad/src/prelude.rs b/zebrad/src/prelude.rs index 78a21bb3e..b4d6eb835 100644 --- a/zebrad/src/prelude.rs +++ b/zebrad/src/prelude.rs @@ -6,10 +6,3 @@ pub use crate::application::{app_config, app_reader, app_writer}; /// Commonly used Abscissa traits pub use abscissa_core::{Application, Command, Runnable}; - -/// Type alias to make working with tower traits easier. -/// -/// Note: the 'static lifetime bound means that the *type* cannot have any -/// non-'static lifetimes, (e.g., when a type contains a borrow and is -/// parameterized by 'a), *not* that the object itself has 'static lifetime. -pub(crate) use abscissa_core::error::BoxError;