From 5d115f525397f3c7e4ce11916712cf43b856405c Mon Sep 17 00:00:00 2001 From: Filipe Rodrigues Date: Fri, 4 Feb 2022 21:43:50 +0000 Subject: [PATCH] Added `crossbeam::channel::Sender::Send` to the disallowed paths. --- clippy.toml | 1 + zsw/src/app.rs | 12 ++++++++---- zsw/src/app/settings_window.rs | 7 +++++-- zsw/src/img/loader.rs | 14 +++++++++----- zsw/src/paths/distributer.rs | 9 ++++++++- zsw/src/util/side_effect/extse.rs | 12 ++++++++++++ 6 files changed, 43 insertions(+), 12 deletions(-) diff --git a/clippy.toml b/clippy.toml index fa70f2f..4f242a8 100644 --- a/clippy.toml +++ b/clippy.toml @@ -1,3 +1,4 @@ disallowed-methods = [ {path = "crossbeam::channel::Receiver::recv", reason = "Might deadlock"}, + {path = "crossbeam::channel::Sender::send", reason = "Might deadlock"}, ] diff --git a/zsw/src/app.rs b/zsw/src/app.rs index 698e130..6a31fcb 100644 --- a/zsw/src/app.rs +++ b/zsw/src/app.rs @@ -107,10 +107,12 @@ pub fn run(args: &Args) -> Result<(), anyhow::Error> { let mut thread_spawner = util::ThreadSpawner::new(s); // Spawn the path distributer thread - thread_spawner.spawn_scoped("Path distributer", || paths_distributer.run())?; + // DEADLOCK: The image loader thread ensures no path receiver deadlocks + thread_spawner.spawn_scoped("Path distributer", || paths_distributer.run().allow::())?; // Spawn all image loaders - // DEADLOCK: We ensure the paths distributer doesn't deadlock + // DEADLOCK: The path distributer thread ensures the path distributer won't deadlock + // The renderer thread ensures no image receiver deadlocks let loader_threads = thread::available_parallelism().map_or(1, NonZeroUsize::get); let loader_fns = vec![image_loader; loader_threads] .into_iter() @@ -136,6 +138,7 @@ pub fn run(args: &Args) -> Result<(), anyhow::Error> { })?; // Spawn the renderer thread + // DEADLOCK: This thread ensures that no image receiver will deadlock thread_spawner.spawn_scoped("Renderer", || { renderer.run( &window, @@ -163,8 +166,9 @@ pub fn run(args: &Args) -> Result<(), anyhow::Error> { let (res, duration) = util::measure(|| { // Stop the renderer - // Note: Stopping the renderer will cause it to drop the image receiver, - // which will stop the image loaders and in turn the path loader. + // BLOCKING: Stopping the renderer will cause it to drop the image receivers, + // which will stop the image loaders, which will in turn stop + // the path loader. should_stop.store(true, atomic::Ordering::Relaxed); // Join all thread diff --git a/zsw/src/app/settings_window.rs b/zsw/src/app/settings_window.rs index 0b3237e..620632b 100644 --- a/zsw/src/app/settings_window.rs +++ b/zsw/src/app/settings_window.rs @@ -4,6 +4,8 @@ // `egui` returns a response on every operation, but we don't use them #![allow(unused_results)] +use crate::util::extse::CrossBeamChannelSenderSE; + // Imports use { crate::{paths, util::MightDeadlock, Egui, Panel, PanelState, Panels, Rect, Wgpu}, @@ -55,7 +57,7 @@ impl SettingsWindow { loop { // Get the surface size // TODO: This can deadlock if put inside the `egui.draw` closure. - // DEADLOCK: We aren't calling it from within `Wgpu::render`. + // DEADLOCK: Caller guarantees we aren't calling it from within `Wgpu::render`. let surface_size = wgpu.surface_size().allow::(); // Draw egui @@ -80,7 +82,8 @@ impl SettingsWindow { }; // Then send the paint jobs - if paint_jobs_tx.send(paint_jobs).is_err() { + // DEADLOCK: TODO + if paint_jobs_tx.send_se(paint_jobs).allow::().is_err() { log::info!("Renderer thread quit, quitting"); break; } diff --git a/zsw/src/img/loader.rs b/zsw/src/img/loader.rs index 9653f15..02d7854 100644 --- a/zsw/src/img/loader.rs +++ b/zsw/src/img/loader.rs @@ -11,7 +11,11 @@ use { super::Image, crate::{ paths, - util::{self, extse::CrossBeamChannelReceiverSE, MightDeadlock}, + util::{ + self, + extse::{CrossBeamChannelReceiverSE, CrossBeamChannelSenderSE}, + MightDeadlock, + }, }, anyhow::Context, zsw_side_effect_macros::side_effect, @@ -33,11 +37,10 @@ impl ImageLoader { /// Multiple image loaders may run at the same time /// /// # Deadlock - /// Deadlocks if the path distributer deadlocks in [`paths::Distributer::run`], - /// or if all receivers' deadlock in [`ImageReceiver::recv`]. + /// Deadlocks if the path distributer deadlocks, or if all receivers deadlock. #[side_effect(MightDeadlock)] pub fn run(self) -> Result<(), anyhow::Error> { - // DEADLOCK: Caller ensures the paths distributer doesn't deadlock + // DEADLOCK: Caller ensures the paths distributer isn't deadlocked while let Ok(path) = self.paths_rx.recv().allow::() { match util::measure(|| load::load_image(&path)) { // If we got it, send it @@ -45,8 +48,9 @@ impl ImageLoader { let format = util::image_format(&image); log::debug!(target: "zsw::perf", "Took {duration:?} to load {path:?} (format: {format})"); + // DEADLOCK: Caller guarantees a receiver isn't deadlocked let image = Image { path, image }; - if self.image_tx.send(image).is_err() { + if self.image_tx.send_se(image).allow::().is_err() { log::info!("No more receivers found, quitting"); break; } diff --git a/zsw/src/paths/distributer.rs b/zsw/src/paths/distributer.rs index 7f36aaa..19325de 100644 --- a/zsw/src/paths/distributer.rs +++ b/zsw/src/paths/distributer.rs @@ -3,6 +3,7 @@ // Imports use { super::Inner, + crate::util::{extse::CrossBeamChannelSenderSE, MightDeadlock}, parking_lot::Mutex, rand::prelude::SliceRandom, std::{ @@ -11,6 +12,7 @@ use { path::{Path, PathBuf}, sync::Arc, }, + zsw_side_effect_macros::side_effect, }; /// The distributer @@ -28,6 +30,10 @@ pub struct Distributer { impl Distributer { /// Runs the distributer until all receivers have quit + /// + /// # Deadlock + /// Deadlocks if all receivers deadlock. + #[side_effect(MightDeadlock)] pub fn run(&self) -> Result<(), anyhow::Error> { let mut cur_paths = vec![]; 'run: loop { @@ -58,7 +64,8 @@ impl Distributer { } // Else send the path - if self.tx.send(path).is_err() { + // DEADLOCK: Caller guarantees at least one receiver isn't deadlocked + if self.tx.send_se(path).allow::().is_err() { log::debug!("All receivers quit, quitting"); break 'run; } diff --git a/zsw/src/util/side_effect/extse.rs b/zsw/src/util/side_effect/extse.rs index 346118a..8808547 100644 --- a/zsw/src/util/side_effect/extse.rs +++ b/zsw/src/util/side_effect/extse.rs @@ -16,3 +16,15 @@ impl CrossBeamChannelReceiverSE for crossbeam::channel::Receiver { WithSideEffect::new(self.recv()) } } + +/// Side effect wrapper for [`crossbeam::channel::Sender`] +pub trait CrossBeamChannelSenderSE { + fn send_se(&self, msg: T) -> WithSideEffect>, MightDeadlock>; +} + +impl CrossBeamChannelSenderSE for crossbeam::channel::Sender { + fn send_se(&self, msg: T) -> WithSideEffect>, MightDeadlock> { + #[allow(clippy::disallowed_methods)] // We're wrapping it + WithSideEffect::new(self.send(msg)) + } +}