From 8065a9b3d8480e4c2a7fa154f7e5e48274d7784b Mon Sep 17 00:00:00 2001 From: Nikos Baxevanis Date: Tue, 8 Jul 2025 16:30:16 +0200 Subject: [PATCH] Handle thread join errors uniformly with helper --- Cargo.lock | 1 + stackslib/Cargo.toml | 1 + stackslib/src/burnchains/burnchain.rs | 44 ++++++--- stackslib/src/burnchains/tests/mod.rs | 1 + stackslib/src/burnchains/tests/thread_join.rs | 98 +++++++++++++++++++ 5 files changed, 132 insertions(+), 13 deletions(-) create mode 100644 stackslib/src/burnchains/tests/thread_join.rs diff --git a/Cargo.lock b/Cargo.lock index 662e4b9900..7a5b7005eb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3372,6 +3372,7 @@ dependencies = [ "percent-encoding", "pox-locking", "prometheus", + "proptest", "rand 0.8.5", "rand_chacha 0.3.1", "rand_core 0.6.4", diff --git a/stackslib/Cargo.toml b/stackslib/Cargo.toml index c8f4a1fa7f..e337fdd0e3 100644 --- a/stackslib/Cargo.toml +++ b/stackslib/Cargo.toml @@ -93,6 +93,7 @@ mutants = "0.0.3" rlimit = "0.10.2" chrono = "0.4.19" tempfile = "3.3" +proptest = { version = "1.6.0", default-features = false, features = ["std"] } [features] default = [] diff --git a/stackslib/src/burnchains/burnchain.rs b/stackslib/src/burnchains/burnchain.rs index a90c970ce5..976e5d0a8d 100644 --- a/stackslib/src/burnchains/burnchain.rs +++ b/stackslib/src/burnchains/burnchain.rs @@ -450,6 +450,23 @@ impl BurnchainBlock { } impl Burnchain { + pub fn handle_thread_join( + handle: std::thread::JoinHandle>, + name: &str, + ) -> Result { + match handle.join() { + Ok(Ok(val)) => Ok(val), + Ok(Err(e)) => { + warn!("{} thread error: {:?}", name, e); + Err(e) + } + Err(_) => { + error!("{} thread panicked", name); + Err(burnchain_error::ThreadChannelError) + } + } + } + pub fn new( working_dir: &str, chain_name: &str, @@ -1783,20 +1800,21 @@ impl Burnchain { } // join up - let _ = download_thread.join().unwrap(); - let _ = parse_thread.join().unwrap(); - let block_header = match db_thread.join().unwrap() { - Ok(x) => x, - Err(e) => { - warn!("Failed to join burnchain download thread: {:?}", &e); - if let burnchain_error::CoordinatorClosed = e { - return Err(burnchain_error::CoordinatorClosed); - } else { - return Err(burnchain_error::TrySyncAgain); - } - } - }; + let download_result = Self::handle_thread_join(download_thread, "download"); + let parse_result = Self::handle_thread_join(parse_thread, "parse"); + let db_result = Self::handle_thread_join(db_thread, "db"); + + if let Err(e) = download_result { + warn!("Download thread failed: {:?}", e); + return Err(e); + } + + if let Err(e) = parse_result { + warn!("Parse thread failed: {:?}", e); + return Err(e); + } + let block_header = db_result?; if block_header.block_height < end_block { warn!( "Try synchronizing the burn chain again: final snapshot {} < {}", diff --git a/stackslib/src/burnchains/tests/mod.rs b/stackslib/src/burnchains/tests/mod.rs index b0099def6f..d71c796cb2 100644 --- a/stackslib/src/burnchains/tests/mod.rs +++ b/stackslib/src/burnchains/tests/mod.rs @@ -17,6 +17,7 @@ pub mod affirmation; pub mod burnchain; pub mod db; +pub mod thread_join; use std::collections::HashMap; diff --git a/stackslib/src/burnchains/tests/thread_join.rs b/stackslib/src/burnchains/tests/thread_join.rs new file mode 100644 index 0000000000..56b81ded68 --- /dev/null +++ b/stackslib/src/burnchains/tests/thread_join.rs @@ -0,0 +1,98 @@ +// Copyright (C) 2025 Stacks Open Internet Foundation +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +use std::time::Duration; +use std::{panic, thread}; + +use proptest::prelude::*; + +use crate::burnchains::bitcoin::Error as bitcoin_error; +use crate::burnchains::{Burnchain, Error as burnchain_error}; + +#[test] +fn join_success() { + proptest!(|(v in any::())| { + let h = thread::spawn(move || Ok(v)); + let r = Burnchain::handle_thread_join::(h, "test"); + prop_assert!(r.is_ok()); + prop_assert_eq!(r.unwrap(), v); + }); +} + +#[test] +fn join_with_name() { + proptest!(|(s in "[a-zA-Z0-9_-]{1,20}")| { + let h = thread::spawn(|| Ok(42)); + let r = Burnchain::handle_thread_join::(h, &s); + prop_assert!(r.is_ok()); + prop_assert_eq!(r.unwrap(), 42); + }); +} + +#[test] +fn join_download_error() { + proptest!(|(x in any::())| { + let h = thread::spawn(move || { + let _ = x; + Err(burnchain_error::DownloadError( + bitcoin_error::ConnectionError)) + }); + let r = Burnchain::handle_thread_join::(h, "test"); + prop_assert!(r.is_err()); + match r { + Err(burnchain_error::DownloadError(_)) => {} + _ => return Err(TestCaseError::fail("Expected DownloadError")), + } + }); +} + +#[test] +fn join_parse_error() { + let h = thread::spawn(move || Err(burnchain_error::ParseError)); + let r = Burnchain::handle_thread_join::(h, "test"); + assert!(r.is_err()); + match r { + Err(burnchain_error::ParseError) => {} + _ => panic!("Expected ParseError"), + } +} + +#[test] +fn join_delay() { + proptest!(|(d in 10u64..100, v in any::())| { + let h = thread::spawn(move || { + thread::sleep(Duration::from_millis(d)); + Ok(v) + }); + let r = Burnchain::handle_thread_join::(h, "test"); + prop_assert!(r.is_ok()); + prop_assert_eq!(r.unwrap(), v); + }); +} + +#[test] +fn join_panics() { + let h = thread::spawn(|| { + panic!("boom"); + #[allow(unreachable_code)] + Ok(42) + }); + let r = Burnchain::handle_thread_join::(h, "test"); + assert!(r.is_err()); + match r { + Err(burnchain_error::ThreadChannelError) => {} + _ => panic!("Expected ThreadChannelError"), + } +}