Merge bitcoin/bitcoin#34562: ThreadPool follow-ups, proactive shutdown and HasReason dependency cleanup

408d5b12e8 test: include response body in non-JSON HTTP error msg (Matthew Zipkin)
9dc653b3b4 test: threadpool, add coverage for all Submit() errors (furszy)
ce2a984ee3 test: cleanup, use HasReason in threadpool_tests.cpp (l0rinc)
d9c6769d03 test: refactor, decouple HasReason from test framework machinery (furszy)
dbbb780af0 test: move and simplify BOOST_CHECK ostream helpers (Hodlinator)
3b7cbcafcb test: ensure Stop() thread helps drain the queue (seduless)
ca101a2315 test: coverage for queued tasks completion after interrupt (furszy)
bf2c607aaa threadpool: active-wait during shutdown (furszy)
e88d274430 test: add threadpool Start-Stop race coverage (furszy)
8cd4a4363f threadpool: guard against Start-Stop race (furszy)
9ff1e82e7d test: cleanup, block threads via semaphore instead of shared_future (l0rinc)

Pull request description:

  A few follow-ups to #33689, includes:

  1) `ThreadPool` active-wait during shutdown:
  Instead of just waiting for workers to finish processing tasks, `Stop()` now helps them actively.
  This speeds up the JSON-RPC and REST server shutdown, resulting in a faster node shutdown when many requests remain unhandled. This wasn't included in the original PR due to the behavior change this introduces.

  2) Decouple `HasReason` from the unit test framework machinery
  This avoids providing the entire unit test framework dependency to low-level tests that only require access to the `HasReason` utility class. Examples are: `reverselock_tests.cpp`, `sync_tests.cpp`, `util_check_tests.cpp`, `util_string_tests.cpp`, `script_parse_tests.cpp` and `threadpool_tests.cpp`. These tests no longer gain access to unnecessary components like the chainstate, node context, caches, etc. It includes l0rinc's `threadpool_tests.cpp` `HasReason` changes.

  3) Include response body in non-JSON HTTP error messages
  Straight from pinheadmz [comment](https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2783817192), it makes debugging CI issues easier.

ACKs for top commit:
  maflcko:
    review ACK 408d5b12e8 🕗
  achow101:
    ACK 408d5b12e8
  hodlinator:
    re-ACK 408d5b12e8

Tree-SHA512: 57aa0ef96886f32bf95a0bd7f87c878d31c9df9e34cb96de615eee703ce0824b5cfdf8f5c9cd19a3594559994295b5810c38c94f5efd6291cbbd83a95473357a
This commit is contained in:
Ava Chow
2026-02-26 12:44:11 -08:00
65 changed files with 295 additions and 126 deletions

View File

@@ -105,8 +105,8 @@ public:
{
assert(num_workers > 0);
LOCK(m_mutex);
if (m_interrupt) throw std::runtime_error("Thread pool has been interrupted or is stopping");
if (!m_workers.empty()) throw std::runtime_error("Thread pool already started");
m_interrupt = false; // Reset
// Create workers
m_workers.reserve(num_workers);
@@ -122,6 +122,7 @@ public:
* Any remaining tasks in the queue will be processed before returning.
*
* Must be called from a controller (non-worker) thread.
* Concurrent calls to Start() will be rejected while Stop() is in progress.
*/
void Stop() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
{
@@ -138,10 +139,16 @@ public:
threads_to_join.swap(m_workers);
}
m_cv.notify_all();
// Help draining queue
while (ProcessTask()) {}
// Free resources
for (auto& worker : threads_to_join) worker.join();
// Since we currently wait for tasks completion, sanity-check empty queue
WITH_LOCK(m_mutex, Assume(m_work_queue.empty()));
// Note: m_interrupt is left true until next Start()
LOCK(m_mutex);
Assume(m_work_queue.empty());
// Re-allow Start() now that all workers have exited
m_interrupt = false;
}
enum class SubmitError {
@@ -183,18 +190,19 @@ public:
* @brief Execute a single queued task synchronously.
* Removes one task from the queue and executes it on the calling thread.
*/
void ProcessTask() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
bool ProcessTask() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
{
std::packaged_task<void()> task;
{
LOCK(m_mutex);
if (m_work_queue.empty()) return;
if (m_work_queue.empty()) return false;
// Pop the task
task = std::move(m_work_queue.front());
m_work_queue.pop();
}
task();
return true;
}
/**
@@ -202,6 +210,10 @@ public:
*
* Wakes all worker threads so they can drain the queue and exit.
* Unlike Stop(), this function does not wait for threads to finish.
*
* Note: The next step in the pool lifecycle is calling Stop(), which
* releases any dangling resources and resets the pool state
* for shutdown or restart.
*/
void Interrupt() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
{