From c65121c43a6078dc361aeb1b5bd46caf3e8ac231 Mon Sep 17 00:00:00 2001 From: Clement Tsang <34804052+ClementTsang@users.noreply.github.com> Date: Sun, 11 Aug 2024 06:04:44 +0000 Subject: [PATCH] refactor: clean up some config and panic code (#1556) * clean up some code * refactor cancellation system to a separate cancellation token struct and clean up panic code --- src/constants.rs | 40 ++++++--------- src/data_conversion.rs | 4 +- src/main.rs | 87 +++++++++++++-------------------- src/utils/cancellation_token.rs | 61 +++++++++++++++++++++++ 4 files changed, 111 insertions(+), 81 deletions(-) create mode 100644 src/utils/cancellation_token.rs diff --git a/src/constants.rs b/src/constants.rs index e31b8b7e..d3053f70 100644 --- a/src/constants.rs +++ b/src/constants.rs @@ -23,7 +23,7 @@ pub const TIME_LABEL_HEIGHT_LIMIT: u16 = 7; pub const SIDE_BORDERS: Borders = Borders::LEFT.union(Borders::RIGHT); // Help text -pub const HELP_CONTENTS_TEXT: [&str; 10] = [ +const HELP_CONTENTS_TEXT: [&str; 10] = [ "Either scroll or press the number key to go to the corresponding help menu section:", "1 - General", "2 - CPU widget", @@ -38,7 +38,7 @@ pub const HELP_CONTENTS_TEXT: [&str; 10] = [ // TODO [Help]: Search in help? // TODO [Help]: Move to using tables for easier formatting? -pub const GENERAL_HELP_TEXT: [&str; 32] = [ +pub(crate) const GENERAL_HELP_TEXT: [&str; 32] = [ "1 - General", "q, Ctrl-c Quit", "Esc Close dialog windows, search, widgets, or exit expanded mode", @@ -73,12 +73,12 @@ pub const GENERAL_HELP_TEXT: [&str; 32] = [ "Mouse click Selects the clicked widget, table entry, dialog option, or tab", ]; -pub const CPU_HELP_TEXT: [&str; 2] = [ +const CPU_HELP_TEXT: [&str; 2] = [ "2 - CPU widget", "Mouse scroll Scrolling over an CPU core/average shows only that entry on the chart", ]; -pub const PROCESS_HELP_TEXT: [&str; 17] = [ +const PROCESS_HELP_TEXT: [&str; 17] = [ "3 - Process widget", "dd, F9 Kill the selected process", "c Sort by CPU usage, press again to reverse", @@ -98,7 +98,7 @@ pub const PROCESS_HELP_TEXT: [&str; 17] = [ "M Sort by GPU memory usage, press again to reverse", ]; -pub const SEARCH_HELP_TEXT: [&str; 51] = [ +const SEARCH_HELP_TEXT: [&str; 51] = [ "4 - Process search widget", "Esc Close the search widget (retains the filter)", "Ctrl-a Skip to the start of the search query", @@ -152,7 +152,7 @@ pub const SEARCH_HELP_TEXT: [&str; 51] = [ "TiB ex: read > 1 tib", ]; -pub const SORT_HELP_TEXT: [&str; 6] = [ +const SORT_HELP_TEXT: [&str; 6] = [ "5 - Sort widget", "Down, 'j' Scroll down in list", "Up, 'k' Scroll up in list", @@ -161,13 +161,13 @@ pub const SORT_HELP_TEXT: [&str; 6] = [ "Enter Sort by current selected column", ]; -pub const TEMP_HELP_WIDGET: [&str; 3] = [ +const TEMP_HELP_WIDGET: [&str; 3] = [ "6 - Temperature widget", "'s' Sort by sensor name, press again to reverse", "'t' Sort by temperature, press again to reverse", ]; -pub const DISK_HELP_WIDGET: [&str; 9] = [ +const DISK_HELP_WIDGET: [&str; 9] = [ "7 - Disk widget", "'d' Sort by disk name, press again to reverse", "'m' Sort by disk mount, press again to reverse", @@ -179,18 +179,18 @@ pub const DISK_HELP_WIDGET: [&str; 9] = [ "'w' Sort by disk write activity, press again to reverse", ]; -pub const BATTERY_HELP_TEXT: [&str; 3] = [ +const BATTERY_HELP_TEXT: [&str; 3] = [ "8 - Battery widget", "Left Go to previous battery", "Right Go to next battery", ]; -pub const BASIC_MEM_HELP_TEXT: [&str; 2] = [ +const BASIC_MEM_HELP_TEXT: [&str; 2] = [ "9 - Basic memory widget", "% Toggle between values and percentages for memory usage", ]; -pub const HELP_TEXT: [&[&str]; HELP_CONTENTS_TEXT.len()] = [ +pub(crate) const HELP_TEXT: [&[&str]; HELP_CONTENTS_TEXT.len()] = [ &HELP_CONTENTS_TEXT, &GENERAL_HELP_TEXT, &CPU_HELP_TEXT, @@ -203,8 +203,7 @@ pub const HELP_TEXT: [&[&str]; HELP_CONTENTS_TEXT.len()] = [ &BASIC_MEM_HELP_TEXT, ]; -// Default layouts -pub const DEFAULT_LAYOUT: &str = r#" +pub(crate) const DEFAULT_LAYOUT: &str = r#" [[row]] ratio=30 [[row.child]] @@ -229,7 +228,7 @@ pub const DEFAULT_LAYOUT: &str = r#" default=true "#; -pub const DEFAULT_BATTERY_LAYOUT: &str = r#" +pub(crate) const DEFAULT_BATTERY_LAYOUT: &str = r#" [[row]] ratio=30 [[row.child]] @@ -258,10 +257,8 @@ pub const DEFAULT_BATTERY_LAYOUT: &str = r#" default=true "#; -// Config and flags - // TODO: Eventually deprecate this, or grab from a file. -pub const CONFIG_TEXT: &str = r#"# This is a default config file for bottom. All of the settings are commented +pub(crate) const CONFIG_TEXT: &str = r#"# This is a default config file for bottom. All of the settings are commented # out by default; if you wish to change them uncomment and modify as you see # fit. @@ -475,15 +472,6 @@ pub const CONFIG_TEXT: &str = r#"# This is a default config file for bottom. All # default=true "#; -pub const CONFIG_TOP_HEAD: &str = r##"# This is bottom's config file. -# Values in this config file will change when changed in the interface. -# You can also manually change these values. -# Be aware that contents of this file will be overwritten if something is -# changed in the application; you can disable writing via the -# --no_write flag or no_write config option. - -"##; - #[cfg(test)] mod test { use super::*; diff --git a/src/data_conversion.rs b/src/data_conversion.rs index d72fae24..25df88b5 100644 --- a/src/data_conversion.rs +++ b/src/data_conversion.rs @@ -74,8 +74,8 @@ pub struct ConvertedData { pub cache_labels: Option<(String, String)>, pub swap_labels: Option<(String, String)>, - pub mem_data: Vec, /* TODO: Switch this and all data points over to a better data - * structure... */ + // TODO: Switch this and all data points over to a better data structure. + pub mem_data: Vec, #[cfg(not(target_os = "windows"))] pub cache_data: Vec, pub swap_data: Vec, diff --git a/src/main.rs b/src/main.rs index bc319518..be079976 100644 --- a/src/main.rs +++ b/src/main.rs @@ -9,6 +9,7 @@ pub mod app; pub mod utils { + pub mod cancellation_token; pub mod data_prefixes; pub mod data_units; pub mod general; @@ -29,7 +30,7 @@ use std::{ panic::{self, PanicInfo}, sync::{ mpsc::{self, Receiver, Sender}, - Arc, Condvar, Mutex, + Arc, }, thread::{self, JoinHandle}, time::{Duration, Instant}, @@ -49,6 +50,7 @@ use data_conversion::*; use event::{handle_key_event_or_break, handle_mouse_event, BottomEvent, CollectionThreadEvent}; use options::{args, get_or_create_config, init_app}; use tui::{backend::CrosstermBackend, Terminal}; +use utils::cancellation_token::CancellationToken; #[allow(unused_imports)] use utils::logging::*; @@ -131,23 +133,27 @@ fn panic_hook(panic_info: &PanicInfo<'_>) { )), ); } + + // TODO: Might be cleaner in the future to use a cancellation token, but that causes some fun issues with + // lifetimes; for now if it panics then shut down the main program entirely ASAP. + std::process::exit(1); } /// Create a thread to poll for user inputs and forward them to the main thread. fn create_input_thread( - sender: Sender, termination_ctrl_lock: Arc>, + sender: Sender, cancellation_token: Arc, ) -> JoinHandle<()> { thread::spawn(move || { let mut mouse_timer = Instant::now(); loop { - if let Ok(is_terminated) = termination_ctrl_lock.try_lock() { - // We don't block. - if *is_terminated { - drop(is_terminated); + // We don't block. + if let Some(is_terminated) = cancellation_token.try_check() { + if is_terminated { break; } } + if let Ok(poll) = poll(Duration::from_millis(20)) { if poll { if let Ok(event) = read() { @@ -206,8 +212,8 @@ fn create_input_thread( /// Create a thread to handle data collection. fn create_collection_thread( sender: Sender, control_receiver: Receiver, - termination_lock: Arc>, termination_cvar: Arc, - app_config_fields: &AppConfigFields, filters: DataFilters, used_widget_set: UsedWidgets, + cancellation_token: Arc, app_config_fields: &AppConfigFields, + filters: DataFilters, used_widget_set: UsedWidgets, ) -> JoinHandle<()> { let temp_type = app_config_fields.temperature_type; let use_current_cpu_total = app_config_fields.use_current_cpu_total; @@ -228,9 +234,8 @@ fn create_collection_thread( loop { // Check once at the very top... don't block though. - if let Ok(is_terminated) = termination_lock.try_lock() { - if *is_terminated { - drop(is_terminated); + if let Some(is_terminated) = cancellation_token.try_check() { + if is_terminated { break; } } @@ -247,9 +252,8 @@ fn create_collection_thread( data_state.update_data(); // Yet another check to bail if needed... do not block! - if let Ok(is_terminated) = termination_lock.try_lock() { - if *is_terminated { - drop(is_terminated); + if let Some(is_terminated) = cancellation_token.try_check() { + if is_terminated { break; } } @@ -260,15 +264,9 @@ fn create_collection_thread( break; } - // This is actually used as a "sleep" that can be interrupted by another thread. - if let Ok((is_terminated, _)) = termination_cvar.wait_timeout( - termination_lock.lock().unwrap(), - Duration::from_millis(update_time), - ) { - if *is_terminated { - drop(is_terminated); - break; - } + // Sleep while allowing for interruptions... + if cancellation_token.sleep_with_cancellation(Duration::from_millis(update_time)) { + break; } } }) @@ -308,8 +306,6 @@ fn generate_schema() -> anyhow::Result<()> { } fn main() -> anyhow::Result<()> { - // TODO: If there is any panic in any thread, send a cancellation token (or similar) to shut down everything. - // let _profiler = dhat::Profiler::new_heap(); let args = args::get_args(); @@ -341,12 +337,7 @@ fn main() -> anyhow::Result<()> { // Check if the current environment is in a terminal. check_if_terminal(); - // Create termination mutex and cvar. We use this setup because we need to sleep - // at some points in the update thread, but we want to be able to interrupt - // the "sleep" if a termination occurs. - let termination_lock = Arc::new(Mutex::new(false)); - let termination_cvar = Arc::new(Condvar::new()); - + let cancellation_token = Arc::new(CancellationToken::default()); let (sender, receiver) = mpsc::channel(); // Set up the event loop thread; we set this up early to speed up @@ -355,37 +346,27 @@ fn main() -> anyhow::Result<()> { let _collection_thread = create_collection_thread( sender.clone(), collection_thread_ctrl_receiver, - termination_lock.clone(), - termination_cvar.clone(), + cancellation_token.clone(), &app.app_config_fields, app.filters.clone(), app.used_widgets, ); // Set up the input handling loop thread. - let _input_thread = create_input_thread(sender.clone(), termination_lock.clone()); + let _input_thread = create_input_thread(sender.clone(), cancellation_token.clone()); // Set up the cleaning loop thread. let _cleaning_thread = { - let lock = termination_lock.clone(); - let cvar = termination_cvar.clone(); + let cancellation_token = cancellation_token.clone(); let cleaning_sender = sender.clone(); let offset_wait_time = app.app_config_fields.retention_ms + 60000; - thread::spawn(move || { - loop { - let result = cvar.wait_timeout( - lock.lock().unwrap(), - Duration::from_millis(offset_wait_time), - ); - if let Ok(result) = result { - if *(result.0) { - break; - } - } - if cleaning_sender.send(BottomEvent::Clean).is_err() { - // debug!("Failed to send cleaning sender..."); - break; - } + thread::spawn(move || loop { + if cancellation_token.sleep_with_cancellation(Duration::from_millis(offset_wait_time)) { + break; + } + + if cleaning_sender.send(BottomEvent::Clean).is_err() { + break; } }) }; @@ -585,8 +566,8 @@ fn main() -> anyhow::Result<()> { } // I think doing it in this order is safe... - *termination_lock.lock().unwrap() = true; - termination_cvar.notify_all(); + // TODO: maybe move the cancellation token to the ctrl-c handler? + cancellation_token.cancel(); cleanup_terminal(&mut terminal)?; Ok(()) diff --git a/src/utils/cancellation_token.rs b/src/utils/cancellation_token.rs new file mode 100644 index 00000000..981a5016 --- /dev/null +++ b/src/utils/cancellation_token.rs @@ -0,0 +1,61 @@ +use std::{ + sync::{Condvar, Mutex}, + time::Duration, +}; + +/// A cancellation token. +pub(crate) struct CancellationToken { + // The "check" for the cancellation token. Setting this to true will mark the cancellation token as "cancelled". + mutex: Mutex, + cvar: Condvar, +} + +impl Default for CancellationToken { + fn default() -> Self { + Self { + mutex: Mutex::new(false), + cvar: Condvar::new(), + } + } +} + +impl CancellationToken { + /// Mark the [`CancellationToken`] as cancelled. + /// + /// This is idempotent, and once cancelled, will stay cancelled. Sending it + /// again will not do anything. + pub fn cancel(&self) { + let mut guard = self + .mutex + .lock() + .expect("cancellation token lock should not be poisoned"); + + if !*guard { + *guard = true; + self.cvar.notify_all(); + } + } + + /// Try and check the [`CancellationToken`]'s status. Note that + /// this will not block. + pub fn try_check(&self) -> Option { + self.mutex.try_lock().ok().map(|guard| *guard) + } + + /// Allows a thread to sleep while still being interruptible with by the token. + /// + /// Returns the condition state after either sleeping or being woken up. + pub fn sleep_with_cancellation(&self, duration: Duration) -> bool { + let guard = self + .mutex + .lock() + .expect("cancellation token lock should not be poisoned"); + + let (result, _) = self + .cvar + .wait_timeout(guard, duration) + .expect("cancellation token lock should not be poisoned"); + + *result + } +}