Repository: bitcoin/bitcoin
File: src/util/threadpool.h (C/C++)

Code before:
// Copyright (c) The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or https://www.opensource.org/licenses/mit-license.php.

#ifndef BITCOIN_UTIL_THREADPOOL_H
#define BITCOIN_UTIL_THREADPOOL_H

#include <sync.h>
#include <tinyformat.h>
#include <util/expected.h>
#include <util/check.h>
#include <util/thread.h>

#include <algorithm>
#include <condition_variable>
#include <functional>
#include <future>
#include <queue>
#include <stdexcept>
#include <thread>
#include <type_traits>
#include <utility>
#include <vector>

/**
 * @brief Fixed-size thread pool for running arbitrary tasks concurrently.
 *
 * The thread pool maintains a set of worker threads that consume and execute
 * tasks submitted through Submit(). Once started, tasks can be queued and
 * processed asynchronously until Stop() is called.
 *
 * ### Thread-safety and lifecycle
 * - `Start()` and `Stop()` must be called from a controller (non-worker) thread.
 *   Calling `Stop()` from a worker thread will deadlock, as it waits for all
 *   workers to join, including the current one.
 *
 * - `Submit()` can be called from any thread, including workers. It safely
 *   enqueues new work for execution as long as the pool has active workers.
 *
 * - `Interrupt()` stops new task submission and lets queued ones drain
 *   in the background. Callers can continue other shutdown steps and call
 *   Stop() at the end to ensure no remaining tasks are left to execute.
 *
 * - `Stop()` prevents further task submission and blocks until all the
 *   queued ones are completed.
 */
class ThreadPool
{

Review comment (bug):
In "threadpool: make Submit return Expected instead of throwing" 818e2edadc11dca226539ddee945f0acf6e53814

This introduces inconsistency. In `Start`, we throw a runtime error as a result of multiple calls to `Start`, or possibly an exception could occur when thread creation fails for some reason. 

 I'm also a bit conflicted here. Why are we using `util::Expected`? Shouldn't the client be patched to handle throws when interacting with the thread pool? It's likely that we throw for some cases, and still back to an issue like the unclean shutdown we are fixing here?


Code after:
// Copyright (c) The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or https://www.opensource.org/licenses/mit-license.php.

#ifndef BITCOIN_UTIL_THREADPOOL_H
#define BITCOIN_UTIL_THREADPOOL_H

#include <sync.h>
#include <tinyformat.h>
#include <util/expected.h>
#include <util/check.h>
#include <util/thread.h>

#include <algorithm>
#include <condition_variable>
#include <functional>
#include <future>
#include <queue>
#include <thread>
#include <type_traits>
#include <utility>
#include <vector>

/**
 * @brief Fixed-size thread pool for running arbitrary tasks concurrently.
 *
 * The thread pool maintains a set of worker threads that consume and execute
 * tasks submitted through Submit(). Once started, tasks can be queued and
 * processed asynchronously until Stop() is called.
 *
 * ### Thread-safety and lifecycle
 * - `Start()` and `Stop()` must be called from a controller (non-worker) thread.
 *   Calling `Stop()` from a worker thread will deadlock, as it waits for all
 *   workers to join, including the current one.
 *
 * - `Submit()` can be called from any thread, including workers. It safely
 *   enqueues new work for execution as long as the pool has active workers.
 *
 * - `Interrupt()` stops new task submission and lets queued ones drain
 *   in the background. Callers can continue other shutdown steps and call
 *   Stop() at the end to ensure no remaining tasks are left to execute.
 *
 * - `Stop()` prevents further task submission and blocks until all the
 *   queued ones are completed.
 */
class ThreadPool
{
private: