f matt tweaks

Author: valentinewallace

background-processor/src/lib.rs

@@ -1,8 +1,9 @@
+#[macro_use] extern crate lightning;
+
 use lightning::chain;
 use lightning::chain::chaininterface::{BroadcasterInterface, FeeEstimator};
 use lightning::chain::keysinterface::{ChannelKeys, KeysInterface};
 use lightning::ln::channelmanager::ChannelManager;
-use lightning::{log_internal, log_trace};
 use lightning::util::logger::Logger;
 use lightning::util::ser::Writeable;
 use std::sync::Arc;
@@ -28,14 +29,7 @@ pub struct BackgroundProcessor {
 	stop_thread: Arc<AtomicBool>,
 	/// May be used to retrieve and handle the error if `BackgroundProcessor`'s thread
 	/// panics.
-	pub thread_handle: JoinHandle<()>
-}
-
-// Re-export these modules for the logging macros.
-mod util {
-	pub(crate) mod logger {
-		pub(crate) use lightning::util::logger::Record;
-	}
+	pub thread_handle: JoinHandle<Result<(), std::io::Error>>
 }
 
 #[cfg(not(test))]
@@ -45,25 +39,20 @@ const CHAN_FRESHNESS_TIMER: u64 = 1;
 
 impl BackgroundProcessor {
 	/// Start a background thread that takes care of responsibilities enumerated in the top-level
-	/// documentation. Marked as `must_use` because otherwise the result is dropped immediately,
-	/// resulting in the thread being terminated.
-	/// Important note: this thread will panic if invoking `persist_manager` results in an error (and
-	/// `start()` will need to be called again to restart the `BackgroundProcessor`).
-	/// There are 3 main options for handling this panic:
-	/// * wait on [`thread_handle`]'s `join()`, handle the error
-	/// * [configure] to abort on panic
-	/// * write a custom `persist_manager` to handle the error so it never gets returned to
-	///   `BackgroundProcessor`.
+	/// 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`
 	///
 	/// `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`]: struct.BackgroundProcessor.html#structfield.thread_handle
-	/// [configure]: https://doc.rust-lang.org/edition-guide/rust-2018/error-handling-and-panics/aborting-on-panic.html
 	/// [`ChannelManager::write`]: ../lightning/ln/channelmanager/struct.ChannelManager.html#method.write
 	/// [`FilesystemPersister::persist_manager`]: ../lightning_persister/struct.FilesystemPersister.html#impl
-	#[must_use]
 	pub fn start<PM, ChanSigner, M, T, K, F, L>(persist_manager: PM, manager: Arc<ChannelManager<ChanSigner, Arc<M>, Arc<T>, Arc<K>, Arc<F>, Arc<L>>>, logger: Arc<L>) -> Self
 	where ChanSigner: 'static + ChannelKeys + Writeable,
 	      M: 'static + chain::Watch<Keys=ChanSigner>,
@@ -75,19 +64,17 @@ impl BackgroundProcessor {
 	{
 		let stop_thread = Arc::new(AtomicBool::new(false));
 		let stop_thread_clone = stop_thread.clone();
-		let handle = thread::spawn(move || {
+		let handle = thread::spawn(move || -> Result<(), std::io::Error>{
 			let mut current_time = Instant::now();
 			loop {
 				let updates_available = manager.wait_timeout(Duration::from_millis(100));
 				if updates_available {
-					if let Err(e) = persist_manager(manager.clone()) {
-						panic!("Errored persisting manager: {}", e);
-					};
+					persist_manager(manager.clone())?;
 				}
 				// Exit the loop if the background processor was requested to stop.
 				if stop_thread.load(Ordering::Acquire) == true {
 					log_trace!(logger, "Terminating background processor.");
-					break;
+					return Ok(())
 				}
 				if current_time.elapsed().as_secs() > CHAN_FRESHNESS_TIMER {
 					log_trace!(logger, "Calling manager's timer_chan_freshness_every_min");
@@ -217,7 +204,7 @@ mod tests {
 		// Initiate the background processors to watch each node.
 		let data_dir = nodes[0].persister.get_data_dir();
 		let callback = move |node| FilesystemPersister::persist_manager(data_dir.clone(), node);
-		let _processor = BackgroundProcessor::start(callback, nodes[0].node.clone(), nodes[0].logger.clone());
+		BackgroundProcessor::start(callback, nodes[0].node.clone(), nodes[0].logger.clone());
 
 		// Go through the channel creation process until each node should have something persisted.
 		let tx = open_channel!(nodes[0], nodes[1], 100000);
@@ -270,7 +257,7 @@ mod tests {
 		let nodes = create_nodes(1, "test_chan_freshness_called".to_string());
 		let data_dir = nodes[0].persister.get_data_dir();
 		let callback = move |node| FilesystemPersister::persist_manager(data_dir.clone(), node);
-		let processor = BackgroundProcessor::start(callback, nodes[0].node.clone(), nodes[0].logger.clone());
+		BackgroundProcessor::start(callback, nodes[0].node.clone(), nodes[0].logger.clone());
 		loop {
 			let log_entries = nodes[0].logger.lines.lock().unwrap();
 			let desired_log = "Calling manager's timer_chan_freshness_every_min".to_string();
@@ -294,16 +281,6 @@ mod tests {
 			Err(std::io::Error::new(std::io::ErrorKind::Other, "test"))
 		}
 
-		// We don't want the expected panic to print to the console during testing, so
-		// swallow it here.
-		std::panic::set_hook(Box::new(|panic_info| {
-			if let Some(s) = panic_info.payload().downcast_ref::<String>() {
-				assert_eq!(s, "Errored persisting manager: test");
-			} else {
-				panic!("Expected string panic");
-			}
-		}));
-
 		let nodes = create_nodes(2, "test_persist_error".to_string());
 		let bg_processor = BackgroundProcessor::start(persist_manager, nodes[0].node.clone(), nodes[0].logger.clone());
 		open_channel!(nodes[0], nodes[1], 100000);

lightning/src/util/macro_logger.rs

@@ -160,7 +160,7 @@ macro_rules! log_spendable {
 #[macro_export]
 macro_rules! log_internal {
 	($logger: expr, $lvl:expr, $($arg:tt)+) => (
-		$logger.log(&::util::logger::Record::new($lvl, format_args!($($arg)+), module_path!(), file!(), line!()));
+		$logger.log(&$crate::util::logger::Record::new($lvl, format_args!($($arg)+), module_path!(), file!(), line!()));
 	);
 }