From d9fa8f1c3872a32d107ac2e5630d6c3922bf9cc9 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Sun, 18 Jul 2021 12:59:27 -0500 Subject: [PATCH 1/3] Correctly assert BackgroundProcessor error The specific error from the ChannelManager persister is not asserted for in test_persist_error. Rather, any error will do. Update the test to use BackgroundProcessor::stop and assert for the expected value. --- lightning-background-processor/src/lib.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lightning-background-processor/src/lib.rs b/lightning-background-processor/src/lib.rs index 0b886f7bf4e..f5c914ed5df 100644 --- a/lightning-background-processor/src/lib.rs +++ b/lightning-background-processor/src/lib.rs @@ -416,7 +416,13 @@ mod tests { let persister = |_: &_| Err(std::io::Error::new(std::io::ErrorKind::Other, "test")); let event_handler = |_| {}; let bg_processor = BackgroundProcessor::start(persister, event_handler, nodes[0].chain_monitor.clone(), nodes[0].node.clone(), nodes[0].peer_manager.clone(), nodes[0].logger.clone()); - let _ = bg_processor.thread_handle.join().unwrap().expect_err("Errored persisting manager: test"); + match bg_processor.stop() { + Ok(_) => panic!("Expected error persisting manager"), + Err(e) => { + assert_eq!(e.kind(), std::io::ErrorKind::Other); + assert_eq!(e.get_ref().unwrap().to_string(), "test"); + }, + } } #[test] From 4f05db6af84d76597d0cd0a03c6a00ec825fc7bf Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Sun, 18 Jul 2021 13:11:01 -0500 Subject: [PATCH 2/3] Stop BackgroundProcessor's thread on drop Without stopping the thread when BackgroundProcessor is dropped, it will run free. In the context of language bindings, it is difficult to know how long references held by the thread should live. Implement Drop to stop the thread just as is done when explicitly calling stop(). --- lightning-background-processor/src/lib.rs | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/lightning-background-processor/src/lib.rs b/lightning-background-processor/src/lib.rs index f5c914ed5df..cc4c9e63502 100644 --- a/lightning-background-processor/src/lib.rs +++ b/lightning-background-processor/src/lib.rs @@ -43,7 +43,7 @@ pub struct BackgroundProcessor { stop_thread: Arc, /// May be used to retrieve and handle the error if `BackgroundProcessor`'s thread /// exits due to an error while persisting. - pub thread_handle: JoinHandle>, + pub thread_handle: Option>>, } #[cfg(not(test))] @@ -158,13 +158,27 @@ impl BackgroundProcessor { } } }); - Self { stop_thread: stop_thread_clone, thread_handle: handle } + Self { stop_thread: stop_thread_clone, thread_handle: Some(handle) } } /// Stop `BackgroundProcessor`'s thread. - pub fn stop(self) -> Result<(), std::io::Error> { + pub fn stop(mut self) -> Result<(), std::io::Error> { + assert!(self.thread_handle.is_some()); + self.stop_and_join_thread() + } + + fn stop_and_join_thread(&mut self) -> Result<(), std::io::Error> { self.stop_thread.store(true, Ordering::Release); - self.thread_handle.join().unwrap() + match self.thread_handle.take() { + Some(handle) => handle.join().unwrap(), + None => Ok(()), + } + } +} + +impl Drop for BackgroundProcessor { + fn drop(&mut self) { + self.stop_and_join_thread().unwrap(); } } From e260cfcd9bb647b4f7838b3435b49132b36b54cf Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Mon, 19 Jul 2021 12:50:56 -0500 Subject: [PATCH 3/3] Add join method to BackgroundProcessor The previous commit wraps the background thread's JoinHandle in an Option. Providing a dedicated method to join hides this implementation detail from users. --- lightning-background-processor/src/lib.rs | 54 +++++++++++++++++------ 1 file changed, 41 insertions(+), 13 deletions(-) diff --git a/lightning-background-processor/src/lib.rs b/lightning-background-processor/src/lib.rs index cc4c9e63502..55ac14c048c 100644 --- a/lightning-background-processor/src/lib.rs +++ b/lightning-background-processor/src/lib.rs @@ -41,9 +41,7 @@ use std::ops::Deref; /// for unilateral chain closure fees are at risk. pub struct BackgroundProcessor { stop_thread: Arc, - /// May be used to retrieve and handle the error if `BackgroundProcessor`'s thread - /// exits due to an error while persisting. - pub thread_handle: Option>>, + thread_handle: Option>>, } #[cfg(not(test))] @@ -84,21 +82,25 @@ ChannelManagerPersister for Fun where } impl BackgroundProcessor { - /// Start a background thread that takes care of responsibilities enumerated in the top-level - /// documentation. + /// Start a background thread that takes care of responsibilities enumerated in the [top-level + /// documentation]. /// - /// If `persist_manager` returns an error, then this thread will return said error (and - /// `start()` will need to be called again to restart the `BackgroundProcessor`). Users should - /// wait on [`thread_handle`]'s `join()` method to be able to tell if and when an error is - /// returned, or implement `persist_manager` such that an error is never returned to the - /// `BackgroundProcessor` + /// The thread runs indefinitely unless the object is dropped, [`stop`] is called, or + /// `persist_manager` returns an error. In case of an error, the error is retrieved by calling + /// either [`join`] or [`stop`]. + /// + /// Typically, users should either implement [`ChannelManagerPersister`] to never return an + /// error or call [`join`] and handle any error that may arise. For the latter case, the + /// `BackgroundProcessor` must be restarted by calling `start` again after handling the error. /// /// `persist_manager` is responsible for writing out the [`ChannelManager`] to disk, and/or /// uploading to one or more backup services. See [`ChannelManager::write`] for writing out a /// [`ChannelManager`]. See [`FilesystemPersister::persist_manager`] for Rust-Lightning's /// provided implementation. /// - /// [`thread_handle`]: BackgroundProcessor::thread_handle + /// [top-level documentation]: Self + /// [`join`]: Self::join + /// [`stop`]: Self::stop /// [`ChannelManager`]: lightning::ln::channelmanager::ChannelManager /// [`ChannelManager::write`]: lightning::ln::channelmanager::ChannelManager#impl-Writeable /// [`FilesystemPersister::persist_manager`]: lightning_persister::FilesystemPersister::persist_manager @@ -161,7 +163,29 @@ impl BackgroundProcessor { Self { stop_thread: stop_thread_clone, thread_handle: Some(handle) } } - /// Stop `BackgroundProcessor`'s thread. + /// Join `BackgroundProcessor`'s thread, returning any error that occurred while persisting + /// [`ChannelManager`]. + /// + /// # Panics + /// + /// This function panics if the background thread has panicked such as while persisting or + /// handling events. + /// + /// [`ChannelManager`]: lightning::ln::channelmanager::ChannelManager + pub fn join(mut self) -> Result<(), std::io::Error> { + assert!(self.thread_handle.is_some()); + self.join_thread() + } + + /// Stop `BackgroundProcessor`'s thread, returning any error that occurred while persisting + /// [`ChannelManager`]. + /// + /// # Panics + /// + /// This function panics if the background thread has panicked such as while persisting or + /// handling events. + /// + /// [`ChannelManager`]: lightning::ln::channelmanager::ChannelManager pub fn stop(mut self) -> Result<(), std::io::Error> { assert!(self.thread_handle.is_some()); self.stop_and_join_thread() @@ -169,6 +193,10 @@ impl BackgroundProcessor { fn stop_and_join_thread(&mut self) -> Result<(), std::io::Error> { self.stop_thread.store(true, Ordering::Release); + self.join_thread() + } + + fn join_thread(&mut self) -> Result<(), std::io::Error> { match self.thread_handle.take() { Some(handle) => handle.join().unwrap(), None => Ok(()), @@ -430,7 +458,7 @@ mod tests { let persister = |_: &_| Err(std::io::Error::new(std::io::ErrorKind::Other, "test")); let event_handler = |_| {}; let bg_processor = BackgroundProcessor::start(persister, event_handler, nodes[0].chain_monitor.clone(), nodes[0].node.clone(), nodes[0].peer_manager.clone(), nodes[0].logger.clone()); - match bg_processor.stop() { + match bg_processor.join() { Ok(_) => panic!("Expected error persisting manager"), Err(e) => { assert_eq!(e.kind(), std::io::ErrorKind::Other);