f jeff tweaks

Author: valentinewallace

background-processor/src/lib.rs

@@ -44,8 +44,8 @@ const CHAN_FRESHNESS_TIMER: u64 = 60;
 const CHAN_FRESHNESS_TIMER: u64 = 1;
 
 impl BackgroundProcessor {
-	/// Start the background thread that takes care of responsibilities (enumerated in the top-level
-	/// documentation). Marked as `must_use` because otherwise the result is dropped immediately,
+	/// 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`).
@@ -84,7 +84,7 @@ impl BackgroundProcessor {
 						panic!("Errored persisting manager: {}", e);
 					};
 				}
-				// If we see that the thread has been stopped, exit now.
+				// 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;
@@ -214,70 +214,51 @@ mod tests {
 		let nodes = create_nodes(2, "test_background_processor".to_string());
 
 		// Initiate the background processors to watch each node.
-		let data_dir_0 = nodes[0].persister.get_data_dir();
-		let data_dir_1 = nodes[1].persister.get_data_dir();
-		let callback_0 = move |node| FilesystemPersister::persist_manager(data_dir_0.clone(), node);
-		let callback_1 = move |node| FilesystemPersister::persist_manager(data_dir_1.clone(), node);
-		let _processor_0 = BackgroundProcessor::start(callback_0, nodes[0].node.clone(), nodes[0].logger.clone());
-		let _processor_1 = BackgroundProcessor::start(callback_1, nodes[1].node.clone(), nodes[1].logger.clone());
+		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());
 
 		// Go through the channel creation process until each node should have something persisted.
 		let tx = open_channel!(nodes[0], nodes[1], 100000);
 
-		let mut done_persisting = false;
 		macro_rules! check_persisted_data {
-			($node: expr, $filepath: expr) => {
-				let bytes = loop {
-					match std::fs::read($filepath) {
-						Ok(bytes) => break bytes,
-						Err(_) => continue
-					}
-				};
-				let mut expected_bytes = Vec::new();
-				assert!($node.write(&mut expected_bytes).is_ok());
-				if bytes == expected_bytes {
-					done_persisting = true;
+			($node: expr, $filepath: expr, $expected_bytes: expr) => {
+				match $node.write(&mut $expected_bytes) {
+					Ok(()) => {
+						loop {
+							match std::fs::read($filepath) {
+								Ok(bytes) => {
+									if bytes == $expected_bytes {
+										break
+									} else {
+										continue
+									}
+								},
+								Err(_) => continue
+							}
+						}
+					},
+					Err(e) => panic!("Unexpected error: {}", e)
 				}
 			}
 		}
 
 		// Check that the initial channel manager data is persisted as expected.
-		let filepath_0 = get_full_filepath("test_background_processor_persister_0".to_string(), "manager".to_string());
-		let filepath_1 = get_full_filepath("test_background_processor_persister_1".to_string(), "manager".to_string());
+		let filepath = get_full_filepath("test_background_processor_persister_0".to_string(), "manager".to_string());
+		let mut expected_bytes = Vec::new();
+		check_persisted_data!(nodes[0].node, filepath.clone(), expected_bytes);
 		loop {
-			check_persisted_data!(nodes[0].node, filepath_0.clone());
-			if done_persisting {
-				// Check that eventually BackgroundProcessor resets the condvar when everything's done persisting.
-				loop {
-					if !nodes[0].node.get_persistence_condvar_value() { break }
-				}
-				break
-			}
-		}
-		done_persisting = false;
-		loop {
-			check_persisted_data!(nodes[1].node, filepath_1.clone());
-			if done_persisting {
-				loop {
-					if !nodes[1].node.get_persistence_condvar_value() { break }
-				}
-				break
-			}
+			if !nodes[0].node.get_persistence_condvar_value() { break }
 		}
 
 		// Force-close the channel.
-		nodes[0].node.force_close_channel(&OutPoint { txid: tx.txid(), index: 0 }.to_channel_id());
+		nodes[0].node.force_close_channel(&OutPoint { txid: tx.txid(), index: 0 }.to_channel_id()).unwrap();
 
 		// Check that the force-close updates are persisted.
-		done_persisting = false;
+		let mut expected_bytes = Vec::new();
+		check_persisted_data!(nodes[0].node, filepath.clone(), expected_bytes);
 		loop {
-			check_persisted_data!(nodes[0].node, filepath_0.clone());
-			if done_persisting {
-				loop {
-					if !nodes[0].node.get_persistence_condvar_value() { break }
-				}
-				break
-			}
+			if !nodes[0].node.get_persistence_condvar_value() { break }
 		}
 	}
 
@@ -314,6 +295,8 @@ 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");

lightning-persister/src/lib.rs

@@ -1,4 +1,4 @@
-pub(crate) mod util;
+mod util;
 
 extern crate lightning;
 extern crate bitcoin;
@@ -78,7 +78,11 @@ impl FilesystemPersister {
 
 	/// Writes the provided `ChannelManager` to the path provided at `FilesystemPersister`
 	/// initialization, within a file called "manager".
-	pub fn persist_manager<ChanSigner, M, T, K, F, L>(data_dir: String, manager: Arc<ChannelManager<ChanSigner, Arc<M>, Arc<T>, Arc<K>, Arc<F>, Arc<L>>>) -> Result<(), std::io::Error>
+	pub fn persist_manager<ChanSigner, M, T, K, F, L>(
+		data_dir: String,
+		manager: Arc<ChannelManager<ChanSigner, Arc<M>, Arc<T>, Arc<K>, Arc<F>,
+		Arc<L>>>
+	) -> Result<(), std::io::Error>
 	where ChanSigner: ChannelKeys + Writeable,
         M: chain::Watch<Keys=ChanSigner>,
         T: BroadcasterInterface,