ThreadPool follow-ups, proactive shutdown and HasReason dependency cleanup by furszy · Pull Request #34562 · bitcoin/bitcoin

furszy · 2026-02-11T18:00:13Z

A few follow-ups to #33689, includes:

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.
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.
Include response body in non-JSON HTTP error messages
Straight from pinheadmz comment, it makes debugging CI issues easier.

DrahtBot · 2026-02-11T18:00:48Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hodlinator, maflcko, achow101

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34576 (threadpool: add ranged Submit overload by andrewtoth)
#34411 ([POC] Full Libevent removal by fanquake)
#33421 (node: add BlockTemplateCache by ismaelsadeeq)
#32061 (Replace libevent with our own HTTP and socket-handling implementation by pinheadmz)
#31682 ([IBD] specialize CheckBlock's input & coinbase checks by l0rinc)
#31132 (validation: fetch block inputs on parallel threads 3x faster IBD by andrewtoth)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

drain -> drains [In the comment "Delay release so Stop() drain all tasks from the calling thread", the verb should agree with the singular subject "Stop()", so use "drains".]

No other typos were found.

^{2026-02-26 14:30:33}

src/util/threadpool.h

test/functional/test_framework/authproxy.py

src/test/util/common.h

src/test/threadpool_tests.cpp

furszy · 2026-02-17T22:04:32Z

Thanks for the feedback!

Have tackled everything except one point which will address it post-#34577. Have also rebased the PR on top of #34577 since both touch the pool code. Better to focus on that one first.

seduless

Reviewed adaf571

While testing, I noticed that commenting out while (ProcessTask()) {} in Stop() doesn't cause any test failures, so the active-wait drain behavior isn't currently covered by the test suite. Here's a test that exercises it by verifying the calling thread processes queued tasks during Stop(), and provably fails if the active-wait is removed:

BOOST_AUTO_TEST_CASE(stop_active_wait_drains_queue)
{
    ThreadPool threadPool(POOL_NAME);
    threadPool.Start(NUM_WORKERS_DEFAULT);

    std::counting_semaphore<> blocker(0);
    const auto blocking_tasks = BlockWorkers(threadPool, blocker, NUM_WORKERS_DEFAULT);

    auto main_thread_id = std::this_thread::get_id();
    std::atomic<int> main_thread_tasks{0};
    const int num_tasks = 20;
    for (int i = 0; i < num_tasks; i++) {
        (void)Submit(threadPool, [&main_thread_tasks, main_thread_id]() {
            if (std::this_thread::get_id() == main_thread_id)
                main_thread_tasks.fetch_add(1, std::memory_order_relaxed);
        });
    }
    BOOST_CHECK_EQUAL(threadPool.WorkQueueSize(), static_cast<size_t>(num_tasks));

    // Delay release so Stop() has a window to drain tasks from the calling thread
    std::thread unblocker([&blocker]() {
        UninterruptibleSleep(300ms);
        blocker.release(NUM_WORKERS_DEFAULT);
    });

    threadPool.Stop();
    unblocker.join();

    BOOST_CHECK_GT(main_thread_tasks.load(), 0);
    WAIT_FOR(blocking_tasks);
}

@gregwebs

@gregwebs bitcoin#32950 @ldenman bitcoin#34418 @casey bitcoin#34562 @eklitzke bitcoin#32150 @rion bitcoin#34549

@gregwebs

@gregwebs bitcoin#34610 @ldenman bitcoin#32950 @casey bitcoin#34418 @eklitzke bitcoin#34562 @rion bitcoin#32150

furszy · 2026-02-19T23:20:57Z

@seduless, just pushed the test with a few small modifications. Since you haven't contributed to any repository yet, I couldn't find your email to add you as the commit author. You can send it to me via IRC or leaving a comment here, and I'll give you authorship.

seduless · 2026-02-20T00:53:22Z

@furszy Thanks! Appreciate it: seduless@proton.me

furszy · 2026-02-20T01:51:57Z

@seduless done. Feel very welcome to keep reviewing!

hodlinator

re-ACK 566f7f8

achow101 · 2026-02-25T20:45:18Z

ACK 566f7f8

maflcko

lgtm, but I think the DEBUG_LOG_OUT regression should not be hidden, and probably reverted.

review ACK 566f7f8 🔭

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 566f7f81de967065afebc153ec41dc7b8d8300d0 🔭
djAJqobfVijAJhOWxsBW8FcWoW6+27+15S5YpVNGfATR3dMyeVI0VKItWZUCTQjN3RbjDvRiymA04OyDpnk4Dg==

src/test/threadpool_tests.cpp

src/test/txreconciliation_tests.cpp

src/test/threadpool_tests.cpp

Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>

Instead of waiting for the workers to finish processing tasks, help them actively inside Stop(). This speeds up the JSON-RPC and REST server shutdown procedure, and results in a faster node shutdown when many requests remain unhandled

Exercise the case where tasks remain pending and verify that the thread calling Stop() participates in draining the queue

furszy · 2026-02-26T14:23:22Z

Updated per feedback, thanks @maflcko.

maflcko · 2026-02-26T14:23:57Z

#34562 (comment)

775d6d6 🕹

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: 775d6d6d8f8796eacf690875fe2a78f397aab320 🕹
QwvFHlDaFCAgy1I+7OiI8E8aDUrgH/gePWpp2F1sMNRNah9d/I4//SDl6l8pL1i8ZU9mFExzfDFxmA/obzihBA==

Move the operator<< overloads used by BOOST_CHECK_* out of the unit test machinery test/setup_common, into test/util/common.h. And replace the individual per-type ToString() overloads with a single concept-constrained template that covers any type exposing a ToString() method. This is important to not add uint256.h and transaction_identifier.h dependencies to the shared test/util/common.h file. Co-authored-by: furszy <matiasfurszyfer@protonmail.com>

Avoid providing the entire unit test framework dependency to tests that only require access to the HasReason utility class. E.g. reverselock_tests.cpp, sync_tests.cpp, util_check_tests.cpp, util_string_tests.cpp, and script_parse_tests.cpp only require access to HasReason and nothing else.

Submit tasks to a non-started, interrupted, or stopped pool and verify the proper error is always returned.

Useful for debugging issues. Co-authored-by: Matias Furszyfer <matiasfurszyfer@protonmail.com>

hodlinator

re-ACK 408d5b1

src/test/txreconciliation_tests.cpp

maflcko · 2026-02-26T17:12:47Z

review ACK 408d5b1 🕗

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 408d5b12e80151feded31c2a5509e2dc5f15edf3 🕗
JJta6Ss0/OBWFyZepOHB0Iyzl9klXuQr0EmD24LbgoVXIFsDBSWAgN/CYlsllIFYf4meXWND7PgQfKLKTR67AQ==

achow101 · 2026-02-26T20:23:11Z

ACK 408d5b1

fanquake added this to the 31.0 milestone Feb 11, 2026

DrahtBot mentioned this pull request Feb 12, 2026

util: Remove brittle and confusing sp::Popen(std::string) #34349

Merged

hodlinator reviewed Feb 12, 2026

View reviewed changes

src/util/threadpool.h Outdated Show resolved Hide resolved

maflcko reviewed Feb 12, 2026

View reviewed changes

test/functional/test_framework/authproxy.py Outdated Show resolved Hide resolved

maflcko reviewed Feb 12, 2026

View reviewed changes

src/test/util/common.h Show resolved Hide resolved

This was referenced Feb 13, 2026

test: Remove system_tests/run_command runtime dependencies #33929

Merged

node: add BlockTemplateCache #33421

Open

maflcko reviewed Feb 13, 2026

View reviewed changes

src/test/threadpool_tests.cpp Show resolved Hide resolved

This was referenced Feb 14, 2026

http: fix submission during shutdown race #34577

Merged

Replace libevent with our own HTTP and socket-handling implementation #32061

Open

furszy force-pushed the 2026_threadpool_follow_ups branch from ddf2273 to adaf571 Compare February 17, 2026 22:00

seduless reviewed Feb 18, 2026

View reviewed changes

gniumg-source added a commit to gniumg-source/bitcoin that referenced this pull request Feb 18, 2026

README.md

a93cf6c

@gregwebs bitcoin#32950 @ldenman bitcoin#34418 @casey bitcoin#34562 @eklitzke bitcoin#32150 @rion bitcoin#34549

gniumg-source mentioned this pull request Feb 18, 2026

README.md #34610

Closed

gniumg-source added a commit to gniumg-source/bitcoin that referenced this pull request Feb 18, 2026

Contributing,md

e73f089

@gregwebs bitcoin#34610 @ldenman bitcoin#32950 @casey bitcoin#34418 @eklitzke bitcoin#34562 @rion bitcoin#32150

gniumg-source mentioned this pull request Feb 18, 2026

. #34611

Closed

DrahtBot added the Needs rebase label Feb 18, 2026

furszy force-pushed the 2026_threadpool_follow_ups branch from adaf571 to 4c6cdb0 Compare February 19, 2026 22:58

DrahtBot added CI failed and removed Needs rebase labels Feb 19, 2026

furszy force-pushed the 2026_threadpool_follow_ups branch from b2d2db5 to c919959 Compare February 19, 2026 23:22

furszy force-pushed the 2026_threadpool_follow_ups branch from c919959 to 5561d0c Compare February 20, 2026 01:50

DrahtBot removed the CI failed label Feb 20, 2026

This was referenced Feb 20, 2026

threadpool: add ranged Submit overload #34576

Merged

validation: fetch block inputs on parallel threads 3x faster IBD #31132

Open

hodlinator approved these changes Feb 25, 2026

View reviewed changes

DrahtBot mentioned this pull request Feb 25, 2026

[IBD] specialize CheckBlock's input & coinbase checks #31682

Open

DrahtBot removed the CI failed label Feb 25, 2026

DrahtBot mentioned this pull request Feb 26, 2026

[POC] Full Libevent removal #34411

Draft

maflcko reviewed Feb 26, 2026

View reviewed changes

src/test/threadpool_tests.cpp Outdated Show resolved Hide resolved

src/test/threadpool_tests.cpp Outdated Show resolved Hide resolved

src/test/txreconciliation_tests.cpp Outdated Show resolved Hide resolved

src/test/threadpool_tests.cpp Outdated Show resolved Hide resolved

furszy and others added 4 commits February 26, 2026 11:15

test: add threadpool Start-Stop race coverage

e88d274

Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>

threadpool: active-wait during shutdown

bf2c607

Instead of waiting for the workers to finish processing tasks, help them actively inside Stop(). This speeds up the JSON-RPC and REST server shutdown procedure, and results in a faster node shutdown when many requests remain unhandled

test: coverage for queued tasks completion after interrupt

ca101a2

test: ensure Stop() thread helps drain the queue

3b7cbca

Exercise the case where tasks remain pending and verify that the thread calling Stop() participates in draining the queue

furszy force-pushed the 2026_threadpool_follow_ups branch from 566f7f8 to 775d6d6 Compare February 26, 2026 14:18

hodlinator and others added 5 commits February 26, 2026 11:28

test: cleanup, use HasReason in threadpool_tests.cpp

ce2a984

test: threadpool, add coverage for all Submit() errors

9dc653b

Submit tasks to a non-started, interrupted, or stopped pool and verify the proper error is always returned.

test: include response body in non-JSON HTTP error msg

408d5b1

Useful for debugging issues. Co-authored-by: Matias Furszyfer <matiasfurszyfer@protonmail.com>

furszy force-pushed the 2026_threadpool_follow_ups branch from 775d6d6 to 408d5b1 Compare February 26, 2026 14:30

DrahtBot added the CI failed label Feb 26, 2026

hodlinator approved these changes Feb 26, 2026

View reviewed changes

src/test/txreconciliation_tests.cpp Outdated Show resolved Hide resolved

DrahtBot requested review from achow101 and maflcko February 26, 2026 14:32

DrahtBot removed the CI failed label Feb 26, 2026

achow101 merged commit b6b8f8a into bitcoin:master Feb 26, 2026
51 of 52 checks passed

furszy deleted the 2026_threadpool_follow_ups branch February 26, 2026 20:56

ryanofsky mentioned this pull request Mar 2, 2026

kernel, logging: Pass Logger instances to kernel objects #30342

Draft

kevkevinpal mentioned this pull request Mar 4, 2026

http: fix submission during shutdown race kevkevinpal/bitcoin#283

Open

Conversation

furszy commented Feb 11, 2026 • edited by achow101 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Feb 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

LLM Linter (✨ experimental)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

furszy commented Feb 17, 2026

Uh oh!

seduless left a comment

Choose a reason for hiding this comment

Uh oh!

furszy commented Feb 19, 2026

Uh oh!

seduless commented Feb 20, 2026

Uh oh!

furszy commented Feb 20, 2026

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Feb 25, 2026

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

furszy commented Feb 26, 2026

Uh oh!

maflcko commented Feb 26, 2026

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Feb 26, 2026

Uh oh!

achow101 commented Feb 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

furszy commented Feb 11, 2026 •

edited by achow101

Loading

DrahtBot commented Feb 11, 2026 •

edited

Loading