From 9de8b95b4c106d6edaf64f6523a21708f46cc9a3 Mon Sep 17 00:00:00 2001 From: Finomnis Date: Sat, 21 Oct 2023 15:08:07 +0200 Subject: [PATCH 01/14] Fix coverage of errors.rs --- src/errors/tests.rs | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/src/errors/tests.rs b/src/errors/tests.rs index ac86457..32fd12f 100644 --- a/src/errors/tests.rs +++ b/src/errors/tests.rs @@ -2,7 +2,13 @@ use crate::BoxedError; use super::*; -fn examine_report(report: miette::Report) { +fn examine_report( + error: impl miette::Diagnostic + std::error::Error + std::fmt::Debug + Sync + Send + 'static, +) { + println!("{}", error); + println!("{:?}", error); + // Convert to report + let report: miette::Report = error.into(); println!("{}", report); println!("{:?}", report); // Convert to std::error::Error @@ -13,14 +19,21 @@ fn examine_report(report: miette::Report) { #[test] fn errors_can_be_converted_to_diagnostic() { - examine_report(GracefulShutdownError::ShutdownTimeout::(Box::new([])).into()); - examine_report(GracefulShutdownError::SubsystemsFailed::(Box::new([])).into()); - examine_report(SubsystemJoinError::SubsystemsFailed::(Arc::new([])).into()); - examine_report(SubsystemError::Panicked::("".into()).into()); - examine_report( - SubsystemError::Failed::("".into(), SubsystemFailure("".into())).into(), - ); - examine_report(CancelledByShutdown.into()); + examine_report(GracefulShutdownError::ShutdownTimeout::( + Box::new([]), + )); + examine_report(GracefulShutdownError::SubsystemsFailed::( + Box::new([]), + )); + examine_report(SubsystemJoinError::SubsystemsFailed::( + Arc::new([]), + )); + examine_report(SubsystemError::Panicked::("".into())); + examine_report(SubsystemError::Failed::( + "".into(), + SubsystemFailure("".into()), + )); + examine_report(CancelledByShutdown); } #[test] From e0ddd1bb6a26af1746018fb360bf51c5adf7da16 Mon Sep 17 00:00:00 2001 From: Finomnis Date: Sat, 21 Oct 2023 15:39:12 +0200 Subject: [PATCH 02/14] Another attempt to improve coverage of errors.rs --- src/errors.rs | 1 + src/errors/tests.rs | 2 ++ 2 files changed, 3 insertions(+) diff --git a/src/errors.rs b/src/errors.rs index 693ec98..7008560 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -124,6 +124,7 @@ impl SubsystemError { /// [`cancel_on_shutdown()`](crate::FutureExt::cancel_on_shutdown). #[derive(Error, Debug, Diagnostic)] #[error("A shutdown request caused this task to be cancelled")] +#[diagnostic(code(graceful_shutdown::future::cancelled_by_shutdown))] pub struct CancelledByShutdown; #[cfg(test)] diff --git a/src/errors/tests.rs b/src/errors/tests.rs index 32fd12f..52c3678 100644 --- a/src/errors/tests.rs +++ b/src/errors/tests.rs @@ -7,6 +7,8 @@ fn examine_report( ) { println!("{}", error); println!("{:?}", error); + println!("{:?}", error.source()); + println!("{}", error.code().unwrap()); // Convert to report let report: miette::Report = error.into(); println!("{}", report); From b4eda82e3f5eca8d9e6a48716d05b3dac4b0896d Mon Sep 17 00:00:00 2001 From: Finomnis Date: Sun, 22 Oct 2023 11:47:44 +0200 Subject: [PATCH 03/14] Add coverage tests for runner.rs --- src/runner.rs | 18 +++++++-------- tests/integration_test_2.rs | 44 +++++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 10 deletions(-) create mode 100644 tests/integration_test_2.rs diff --git a/src/runner.rs b/src/runner.rs index 1354057..837465c 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -75,15 +75,10 @@ async fn run_subsystem( Ok(Ok(())) => None, Ok(Err(e)) => Some(SubsystemError::Failed(name, SubsystemFailure(e))), Err(e) => { - if e.is_panic() { - Some(SubsystemError::Panicked(name)) - } else { - // Don't do anything in case of a cancellation; - // cancellations can't be forwarded (because the - // current function we are in will be cancelled - // simultaneously) - None - } + // We can assume that this is a panic, because a cancellation + // can never happen as long as we still hold `guard`. + assert!(e.is_panic()); + Some(SubsystemError::Panicked(name)) } }; @@ -95,7 +90,10 @@ async fn run_subsystem( // It is still important that the handle does not leak out of the subsystem. let subsystem_handle = match redirected_subsystem_handle.try_recv() { Ok(s) => s, - Err(_) => panic!("The SubsystemHandle object must not be leaked out of the subsystem!"), + Err(_) => { + tracing::error!("The SubsystemHandle object must not be leaked out of the subsystem!"); + panic!("The SubsystemHandle object must not be leaked out of the subsystem!"); + } }; // Raise potential errors diff --git a/tests/integration_test_2.rs b/tests/integration_test_2.rs new file mode 100644 index 0000000..455dbdc --- /dev/null +++ b/tests/integration_test_2.rs @@ -0,0 +1,44 @@ +use tokio::time::{sleep, Duration}; +use tokio_graceful_shutdown::{SubsystemBuilder, SubsystemHandle, Toplevel}; +use tracing_test::traced_test; + +pub mod common; + +use std::{ + error::Error, + sync::{Arc, Mutex}, +}; + +/// Wrapper function to simplify lambdas +type BoxedError = Box; +type BoxedResult = Result<(), BoxedError>; + +#[tokio::test] +#[traced_test] +async fn leak_subsystem_handle() { + let subsys_ext: Arc>> = Default::default(); + let subsys_ext2 = Arc::clone(&subsys_ext); + + let subsystem = move |subsys: SubsystemHandle| async move { + subsys.on_shutdown_requested().await; + + *subsys_ext2.lock().unwrap() = Some(subsys); + + BoxedResult::Ok(()) + }; + + let toplevel = Toplevel::new(move |s| async move { + s.start(SubsystemBuilder::new("subsys", subsystem)); + + sleep(Duration::from_millis(100)).await; + s.request_shutdown(); + }); + + let result = toplevel + .handle_shutdown_requests(Duration::from_millis(100)) + .await; + assert!(result.is_err()); + assert!(logs_contain( + "The SubsystemHandle object must not be leaked out of the subsystem!" + )); +} From 2aa6334f737df85ac06455a59ee2ad65ef887fb6 Mon Sep 17 00:00:00 2001 From: Finomnis Date: Sun, 22 Oct 2023 13:55:01 +0200 Subject: [PATCH 04/14] Minor rework in toplevel --- src/toplevel.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/toplevel.rs b/src/toplevel.rs index bcd52a0..71609f0 100644 --- a/src/toplevel.rs +++ b/src/toplevel.rs @@ -181,7 +181,12 @@ impl Toplevel { ); match tokio::time::timeout(shutdown_timeout, self.toplevel_subsys.join()).await { - Ok(Ok(())) => { + Ok(result) => { + // An `Err` here would indicate a programming error, + // because the toplevel subsys doesn't catch any errors; + // it only forwards them. + assert!(result.is_ok()); + let errors = collect_errors(); if errors.is_empty() { tracing::info!("Shutdown finished."); @@ -191,10 +196,6 @@ impl Toplevel { Err(GracefulShutdownError::SubsystemsFailed(errors)) } } - Ok(Err(_)) => { - // This can't happen because the toplevel subsys doesn't catch any errors; it only forwards them. - unreachable!(); - } Err(_) => { tracing::error!("Shutdown timed out!"); Err(GracefulShutdownError::ShutdownTimeout(collect_errors())) From 2255099c232c37f0c83a29ca7c3cc53e94e7cb9f Mon Sep 17 00:00:00 2001 From: Finomnis Date: Sun, 22 Oct 2023 14:27:19 +0200 Subject: [PATCH 05/14] Refactor dropped error --- src/errors.rs | 9 +++++++++ src/subsystem/subsystem_handle.rs | 6 ++---- src/toplevel.rs | 6 ++---- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/errors.rs b/src/errors.rs index 7008560..cc59540 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -4,6 +4,7 @@ use std::sync::Arc; use miette::Diagnostic; use thiserror::Error; +use tokio::sync::mpsc; use crate::ErrTypeTraits; @@ -127,5 +128,13 @@ impl SubsystemError { #[diagnostic(code(graceful_shutdown::future::cancelled_by_shutdown))] pub struct CancelledByShutdown; +pub(crate) fn handle_dropped_error( + result: Result<(), mpsc::error::SendError>, +) { + if let Err(mpsc::error::SendError(e)) = result { + tracing::warn!("An error got dropped: {e:?}"); + }; +} + #[cfg(test)] mod tests; diff --git a/src/subsystem/subsystem_handle.rs b/src/subsystem/subsystem_handle.rs index 52a5f2f..702dacc 100644 --- a/src/subsystem/subsystem_handle.rs +++ b/src/subsystem/subsystem_handle.rs @@ -9,7 +9,7 @@ use tokio::sync::{mpsc, oneshot}; use tokio_util::sync::CancellationToken; use crate::{ - errors::SubsystemError, + errors::{handle_dropped_error, SubsystemError}, runner::{AliveGuard, SubsystemRunner}, utils::{remote_drop_collection::RemotelyDroppableItems, JoinerToken}, BoxedError, ErrTypeTraits, ErrorAction, NestedSubsystem, SubsystemBuilder, @@ -124,9 +124,7 @@ impl SubsystemHandle { match error_action { ErrorAction::Forward => Some(e), ErrorAction::CatchAndLocalShutdown => { - if let Err(mpsc::error::SendError(e)) = error_sender.send(e) { - tracing::warn!("An error got dropped: {e:?}"); - }; + handle_dropped_error(error_sender.send(e)); cancellation_token.cancel(); None } diff --git a/src/toplevel.rs b/src/toplevel.rs index 71609f0..3476ae7 100644 --- a/src/toplevel.rs +++ b/src/toplevel.rs @@ -5,7 +5,7 @@ use tokio::sync::mpsc; use tokio_util::sync::CancellationToken; use crate::{ - errors::{GracefulShutdownError, SubsystemError}, + errors::{handle_dropped_error, GracefulShutdownError, SubsystemError}, signal_handling::wait_for_signal, subsystem::{self, ErrorActions}, BoxedError, ErrTypeTraits, ErrorAction, NestedSubsystem, SubsystemHandle, @@ -74,9 +74,7 @@ impl Toplevel { } }; - if let Err(mpsc::error::SendError(e)) = error_sender.send(e) { - tracing::warn!("An error got dropped: {e:?}"); - }; + handle_dropped_error(error_sender.send(e)); }); let toplevel_subsys = root_handle.start_with_abs_name( From 8775c33c7e0d0c00eb8212465324d248111a2283 Mon Sep 17 00:00:00 2001 From: Finomnis Date: Sun, 22 Oct 2023 14:46:33 +0200 Subject: [PATCH 06/14] Remove defensive coding error handling from coverage check --- src/errors.rs | 12 +++++++++--- src/lib.rs | 2 ++ 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/errors.rs b/src/errors.rs index cc59540..5b5db4a 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -10,15 +10,15 @@ use crate::ErrTypeTraits; /// This enum contains all the possible errors that could be returned /// by [`handle_shutdown_requests()`](crate::Toplevel::handle_shutdown_requests). -#[derive(Error, Debug, Diagnostic)] +#[derive(Debug, Error, Diagnostic)] pub enum GracefulShutdownError { /// At least one subsystem caused an error. - #[error("at least one subsystem returned an error")] #[diagnostic(code(graceful_shutdown::failed))] + #[error("at least one subsystem returned an error")] SubsystemsFailed(#[related] Box<[SubsystemError]>), /// The shutdown did not finish within the given timeout. - #[error("shutdown timed out")] #[diagnostic(code(graceful_shutdown::timeout))] + #[error("shutdown timed out")] ShutdownTimeout(#[related] Box<[SubsystemError]>), } @@ -128,6 +128,12 @@ impl SubsystemError { #[diagnostic(code(graceful_shutdown::future::cancelled_by_shutdown))] pub struct CancelledByShutdown; +// This function contains code that stems from the principle +// of defensive coding - meaning, handle potential errors +// gracefully, even if they should not happen. +// Therefore, not entering this if-block should not cause +// a reduced code coverage. +#[cfg_attr(coverage_nightly, coverage(off))] pub(crate) fn handle_dropped_error( result: Result<(), mpsc::error::SendError>, ) { diff --git a/src/lib.rs b/src/lib.rs index e1d4a89..f520e50 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -91,6 +91,8 @@ test(no_crate_inject, attr(deny(warnings))), test(attr(allow(dead_code))) )] +// Allows functions to be ignored by the coverage algorithm +#![cfg_attr(coverage_nightly, feature(coverage_attribute))] type BoxedError = Box; From ff403a9ea70f0baa7d79677d903a4a82ccf47c50 Mon Sep 17 00:00:00 2001 From: Finomnis Date: Sun, 22 Oct 2023 15:18:08 +0200 Subject: [PATCH 07/14] Fix uncovered handle_dropped_error function --- src/errors.rs | 7 +++---- src/errors/tests.rs | 12 ++++++++++++ src/utils/remote_drop_collection/tests.rs | 14 ++++++++++++++ 3 files changed, 29 insertions(+), 4 deletions(-) diff --git a/src/errors.rs b/src/errors.rs index 5b5db4a..7ce2cb9 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -131,15 +131,14 @@ pub struct CancelledByShutdown; // This function contains code that stems from the principle // of defensive coding - meaning, handle potential errors // gracefully, even if they should not happen. -// Therefore, not entering this if-block should not cause -// a reduced code coverage. -#[cfg_attr(coverage_nightly, coverage(off))] +// Therefore it is in this special function, so we don't +// get coverage problems. pub(crate) fn handle_dropped_error( result: Result<(), mpsc::error::SendError>, ) { if let Err(mpsc::error::SendError(e)) = result { tracing::warn!("An error got dropped: {e:?}"); - }; + } } #[cfg(test)] diff --git a/src/errors/tests.rs b/src/errors/tests.rs index 52c3678..6b69a82 100644 --- a/src/errors/tests.rs +++ b/src/errors/tests.rs @@ -1,3 +1,5 @@ +use tracing_test::traced_test; + use crate::BoxedError; use super::*; @@ -76,3 +78,13 @@ fn extract_contained_error_from_convert_subsystem_failure() { assert_eq!(msg, *failure); assert_eq!(msg, failure.into_error()); } + +#[test] +#[traced_test] +fn handle_dropped_errors() { + handle_dropped_error(Err(mpsc::error::SendError(BoxedError::from(String::from( + "ABC", + ))))); + + assert!(logs_contain("An error got dropped: \"ABC\"")); +} diff --git a/src/utils/remote_drop_collection/tests.rs b/src/utils/remote_drop_collection/tests.rs index 45a194b..ef770be 100644 --- a/src/utils/remote_drop_collection/tests.rs +++ b/src/utils/remote_drop_collection/tests.rs @@ -1,6 +1,20 @@ use super::*; use crate::{utils::JoinerToken, BoxedError}; +#[test] +fn single_item() { + let items = RemotelyDroppableItems::new(); + + let (count1, _) = JoinerToken::::new(|_| None); + assert_eq!(0, count1.count()); + + let token1 = items.insert(count1.child_token(|_| None)); + assert_eq!(1, count1.count()); + + drop(token1); + assert_eq!(0, count1.count()); +} + #[test] fn insert_and_drop() { let items = RemotelyDroppableItems::new(); From c93011cb5a6412c0d7fa1383460bf24da052f401 Mon Sep 17 00:00:00 2001 From: Finomnis Date: Sun, 22 Oct 2023 15:28:18 +0200 Subject: [PATCH 08/14] Fix coverage of remote_drop_collection --- src/utils/remote_drop_collection.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/utils/remote_drop_collection.rs b/src/utils/remote_drop_collection.rs index ad88612..ca1961a 100644 --- a/src/utils/remote_drop_collection.rs +++ b/src/utils/remote_drop_collection.rs @@ -62,10 +62,10 @@ impl Drop for RemoteDrop { // Important: lock first, then read the offset. let mut data = data.lock().unwrap(); - if let Some(offset) = self.offset.upgrade() { + self.offset.upgrade().map(|offset| { let offset = offset.load(Ordering::Acquire); - if let Some(last_item) = data.pop() { + data.pop().map(|last_item| { if offset != data.len() { // There must have been at least two items, and we are not at the end. // So swap first before dropping. @@ -73,8 +73,8 @@ impl Drop for RemoteDrop { last_item.offset.store(offset, Ordering::Release); data[offset] = last_item; } - } - } + }); + }); } } } From b666d26a80d9a558a1e77177c61f5fa4f8a14392 Mon Sep 17 00:00:00 2001 From: Finomnis Date: Sun, 22 Oct 2023 15:48:09 +0200 Subject: [PATCH 09/14] Rework remote_drop_collection --- src/utils/remote_drop_collection.rs | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/src/utils/remote_drop_collection.rs b/src/utils/remote_drop_collection.rs index ca1961a..c149496 100644 --- a/src/utils/remote_drop_collection.rs +++ b/src/utils/remote_drop_collection.rs @@ -62,19 +62,23 @@ impl Drop for RemoteDrop { // Important: lock first, then read the offset. let mut data = data.lock().unwrap(); - self.offset.upgrade().map(|offset| { - let offset = offset.load(Ordering::Acquire); + let offset = self + .offset + .upgrade() + .expect("Trying to delete non-existent item! Please report this.") + .load(Ordering::Acquire); - data.pop().map(|last_item| { - if offset != data.len() { - // There must have been at least two items, and we are not at the end. - // So swap first before dropping. + let last_item = data + .pop() + .expect("Trying to delete non-existent item! Please report this."); - last_item.offset.store(offset, Ordering::Release); - data[offset] = last_item; - } - }); - }); + if offset != data.len() { + // There must have been at least two items, and we are not at the end. + // So swap first before dropping. + + last_item.offset.store(offset, Ordering::Release); + data[offset] = last_item; + } } } } From 7a1cb772d06a01c41740f6988b12c1747d119f52 Mon Sep 17 00:00:00 2001 From: Finomnis Date: Sun, 22 Oct 2023 16:02:01 +0200 Subject: [PATCH 10/14] Rework joiner_token unhandled stopreason handling --- src/errors.rs | 13 +++++++++++++ src/errors/tests.rs | 10 ++++++++++ src/utils/joiner_token.rs | 9 +++++---- 3 files changed, 28 insertions(+), 4 deletions(-) diff --git a/src/errors.rs b/src/errors.rs index 7ce2cb9..54f1ff1 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -141,5 +141,18 @@ pub(crate) fn handle_dropped_error( } } +// This function contains code that stems from the principle +// of defensive coding - meaning, handle potential errors +// gracefully, even if they should not happen. +// Therefore it is in this special function, so we don't +// get coverage problems. +pub(crate) fn handle_unhandled_stopreason( + maybe_stop_reason: Option>, +) { + if let Some(stop_reason) = maybe_stop_reason { + tracing::warn!("Unhandled stop reason: {:?}", stop_reason); + } +} + #[cfg(test)] mod tests; diff --git a/src/errors/tests.rs b/src/errors/tests.rs index 6b69a82..79efd75 100644 --- a/src/errors/tests.rs +++ b/src/errors/tests.rs @@ -88,3 +88,13 @@ fn handle_dropped_errors() { assert!(logs_contain("An error got dropped: \"ABC\"")); } + +#[test] +#[traced_test] +fn handle_unhandled_stopreasons() { + handle_unhandled_stopreason(Some(SubsystemError::::Panicked(Arc::from( + "def", + )))); + + assert!(logs_contain("Unhandled stop reason: Panicked(\"def\")")); +} diff --git a/src/utils/joiner_token.rs b/src/utils/joiner_token.rs index 6b672ea..a60ecc5 100644 --- a/src/utils/joiner_token.rs +++ b/src/utils/joiner_token.rs @@ -2,7 +2,10 @@ use std::{fmt::Debug, sync::Arc}; use tokio::sync::watch; -use crate::{errors::SubsystemError, ErrTypeTraits}; +use crate::{ + errors::{handle_unhandled_stopreason, SubsystemError}, + ErrTypeTraits, +}; struct Inner { counter: watch::Sender<(bool, u32)>, @@ -126,9 +129,7 @@ impl JoinerToken { maybe_parent = parent.parent.as_ref(); } - if let Some(stop_reason) = maybe_stop_reason { - tracing::warn!("Unhandled stop reason: {:?}", stop_reason); - } + handle_unhandled_stopreason(maybe_stop_reason); } pub(crate) fn downgrade(self) -> JoinerTokenRef { From dc2bc9e1b1c36bcf3d2580ca4670ad7445e85a78 Mon Sep 17 00:00:00 2001 From: Finomnis Date: Sun, 22 Oct 2023 16:13:46 +0200 Subject: [PATCH 11/14] Add wait_for_children test --- tests/integration_test_2.rs | 55 +++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/tests/integration_test_2.rs b/tests/integration_test_2.rs index 455dbdc..9748f58 100644 --- a/tests/integration_test_2.rs +++ b/tests/integration_test_2.rs @@ -9,6 +9,8 @@ use std::{ sync::{Arc, Mutex}, }; +use crate::common::Event; + /// Wrapper function to simplify lambdas type BoxedError = Box; type BoxedResult = Result<(), BoxedError>; @@ -42,3 +44,56 @@ async fn leak_subsystem_handle() { "The SubsystemHandle object must not be leaked out of the subsystem!" )); } + +#[tokio::test] +#[traced_test] +async fn wait_for_children() { + let (nested1_started, set_nested1_started) = Event::create(); + let (nested1_finished, set_nested1_finished) = Event::create(); + let (nested2_started, set_nested2_started) = Event::create(); + let (nested2_finished, set_nested2_finished) = Event::create(); + + let nested_subsys2 = move |subsys: SubsystemHandle| async move { + set_nested2_started(); + subsys.on_shutdown_requested().await; + sleep(Duration::from_millis(100)).await; + set_nested2_finished(); + BoxedResult::Ok(()) + }; + + let nested_subsys1 = move |subsys: SubsystemHandle| async move { + subsys.start(SubsystemBuilder::new("nested2", nested_subsys2)); + set_nested1_started(); + subsys.on_shutdown_requested().await; + sleep(Duration::from_millis(100)).await; + set_nested1_finished(); + BoxedResult::Ok(()) + }; + + let subsys1 = move |mut subsys: SubsystemHandle| async move { + subsys.start(SubsystemBuilder::new("nested1", nested_subsys1)); + + sleep(Duration::from_millis(100)).await; + + subsys.request_shutdown(); + + assert!(nested1_started.get()); + assert!(!nested1_finished.get()); + assert!(nested2_started.get()); + assert!(!nested2_finished.get()); + + subsys.wait_for_children().await; + + assert!(nested1_finished.get()); + assert!(nested2_finished.get()); + + BoxedResult::Ok(()) + }; + + Toplevel::new(|s| async move { + s.start(SubsystemBuilder::new("subsys", subsys1)); + }) + .handle_shutdown_requests(Duration::from_millis(500)) + .await + .unwrap(); +} From 7735446447dadbcfb63706fa7f3902c956eb9d95 Mon Sep 17 00:00:00 2001 From: Finomnis Date: Sun, 22 Oct 2023 16:22:02 +0200 Subject: [PATCH 12/14] Add request_local_shutdown test, make wait_for_children non-mut --- src/subsystem/subsystem_handle.rs | 2 +- src/utils/joiner_token.rs | 4 +- src/utils/joiner_token/tests.rs | 2 +- tests/integration_test_2.rs | 63 ++++++++++++++++++++++++++++++- 4 files changed, 65 insertions(+), 6 deletions(-) diff --git a/src/subsystem/subsystem_handle.rs b/src/subsystem/subsystem_handle.rs index 702dacc..0cc4c99 100644 --- a/src/subsystem/subsystem_handle.rs +++ b/src/subsystem/subsystem_handle.rs @@ -165,7 +165,7 @@ impl SubsystemHandle { } /// Waits until all the children of this subsystem are finished. - pub async fn wait_for_children(&mut self) { + pub async fn wait_for_children(&self) { self.inner.joiner_token.join_children().await } diff --git a/src/utils/joiner_token.rs b/src/utils/joiner_token.rs index a60ecc5..4973791 100644 --- a/src/utils/joiner_token.rs +++ b/src/utils/joiner_token.rs @@ -70,9 +70,7 @@ impl JoinerToken { (Self { inner }, weak_ref) } - // Requires `mut` access to prevent children from being spawned - // while waiting - pub(crate) async fn join_children(&mut self) { + pub(crate) async fn join_children(&self) { let mut subscriber = self.inner.counter.subscribe(); // Ignore errors; if the channel got closed, that definitely means diff --git a/src/utils/joiner_token/tests.rs b/src/utils/joiner_token/tests.rs index 03ba7c9..c63c3b8 100644 --- a/src/utils/joiner_token/tests.rs +++ b/src/utils/joiner_token/tests.rs @@ -116,7 +116,7 @@ fn counters_weak() { async fn join() { let (superroot, _) = JoinerToken::::new(|_| None); - let (mut root, _) = superroot.child_token(|_| None); + let (root, _) = superroot.child_token(|_| None); let (child1, _) = root.child_token(|_| None); let (child2, _) = child1.child_token(|_| None); diff --git a/tests/integration_test_2.rs b/tests/integration_test_2.rs index 9748f58..6510fe3 100644 --- a/tests/integration_test_2.rs +++ b/tests/integration_test_2.rs @@ -70,7 +70,7 @@ async fn wait_for_children() { BoxedResult::Ok(()) }; - let subsys1 = move |mut subsys: SubsystemHandle| async move { + let subsys1 = move |subsys: SubsystemHandle| async move { subsys.start(SubsystemBuilder::new("nested1", nested_subsys1)); sleep(Duration::from_millis(100)).await; @@ -97,3 +97,64 @@ async fn wait_for_children() { .await .unwrap(); } + +#[tokio::test] +#[traced_test] +async fn request_local_shutdown() { + let (nested1_started, set_nested1_started) = Event::create(); + let (nested1_finished, set_nested1_finished) = Event::create(); + let (nested2_started, set_nested2_started) = Event::create(); + let (nested2_finished, set_nested2_finished) = Event::create(); + let (global_finished, set_global_finished) = Event::create(); + + let nested_subsys2 = move |subsys: SubsystemHandle| async move { + set_nested2_started(); + subsys.on_shutdown_requested().await; + set_nested2_finished(); + BoxedResult::Ok(()) + }; + + let nested_subsys1 = move |subsys: SubsystemHandle| async move { + subsys.start(SubsystemBuilder::new("nested2", nested_subsys2)); + set_nested1_started(); + subsys.on_shutdown_requested().await; + set_nested1_finished(); + BoxedResult::Ok(()) + }; + + let subsys1 = move |subsys: SubsystemHandle| async move { + subsys.start(SubsystemBuilder::new("nested1", nested_subsys1)); + + sleep(Duration::from_millis(100)).await; + + assert!(nested1_started.get()); + assert!(!nested1_finished.get()); + assert!(nested2_started.get()); + assert!(!nested2_finished.get()); + assert!(!global_finished.get()); + + subsys.request_local_shutdown(); + sleep(Duration::from_millis(200)).await; + + assert!(nested1_finished.get()); + assert!(nested2_finished.get()); + assert!(!global_finished.get()); + + subsys.request_shutdown(); + sleep(Duration::from_millis(50)).await; + + assert!(global_finished.get()); + + BoxedResult::Ok(()) + }; + + Toplevel::new(move |s| async move { + s.start(SubsystemBuilder::new("subsys", subsys1)); + + s.on_shutdown_requested().await; + set_global_finished(); + }) + .handle_shutdown_requests(Duration::from_millis(100)) + .await + .unwrap(); +} From 9733dcd9696f4db5d55119de7d45ee416dc1859d Mon Sep 17 00:00:00 2001 From: Finomnis Date: Sun, 22 Oct 2023 16:37:59 +0200 Subject: [PATCH 13/14] Remove coverage ignore feature again --- src/lib.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index f520e50..e1d4a89 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -91,8 +91,6 @@ test(no_crate_inject, attr(deny(warnings))), test(attr(allow(dead_code))) )] -// Allows functions to be ignored by the coverage algorithm -#![cfg_attr(coverage_nightly, feature(coverage_attribute))] type BoxedError = Box; From b11617e074b4a6c61c7bab0f9ccb7c740cfa110e Mon Sep 17 00:00:00 2001 From: Finomnis Date: Sun, 22 Oct 2023 16:48:02 +0200 Subject: [PATCH 14/14] Add missing assert in request_local_shutdown test --- tests/integration_test_2.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/integration_test_2.rs b/tests/integration_test_2.rs index 6510fe3..28eff68 100644 --- a/tests/integration_test_2.rs +++ b/tests/integration_test_2.rs @@ -132,6 +132,7 @@ async fn request_local_shutdown() { assert!(nested2_started.get()); assert!(!nested2_finished.get()); assert!(!global_finished.get()); + assert!(!subsys.is_shutdown_requested()); subsys.request_local_shutdown(); sleep(Duration::from_millis(200)).await; @@ -139,11 +140,13 @@ async fn request_local_shutdown() { assert!(nested1_finished.get()); assert!(nested2_finished.get()); assert!(!global_finished.get()); + assert!(subsys.is_shutdown_requested()); subsys.request_shutdown(); sleep(Duration::from_millis(50)).await; assert!(global_finished.get()); + assert!(subsys.is_shutdown_requested()); BoxedResult::Ok(()) };