thread_pool improvements: jthread, stop, lazy #86
Conversation
📝 WalkthroughWalkthroughAdded a non-blocking stop() to thread_pool and refactored implementation to use std::jthread/std::stop_token, std::condition_variable_any, lazy startup, cooperative shutdown, and adjusted test Jamfiles removing the buffers test project. (49 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Pool as ThreadPool
participant Queue as TaskQueue
participant Worker as Worker
Client->>Pool: post(task)
Pool->>Pool: ensure_started() (call_once, launch jthreads)
Pool->>Worker: start jthread (with stop_token)
Client->>Queue: enqueue(task)
Worker->>Queue: wait (condvar_any) or stop requested
Queue-->>Worker: deliver task
Worker->>Worker: execute task
Client->>Pool: stop()
Pool->>Pool: request_stop (stop_source)
Pool->>Queue: notify_all()
Worker->>Worker: finish current task or exit if stopped and queue drained
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars-githubusercontent-com.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/ex/thread_pool.cpp`:
- Around line 87-92: The stop() currently calls stop_source_.request_stop()
which does not affect the std::jthread instances because their functions receive
the jthread-owned stop_token; change stop() to iterate the thread container
(e.g., the vector of std::jthread instances used by this pool) and call
request_stop() on each jthread (or call .request_stop() on each thread's
associated stop_source if you've stored one), then notify_all() on cv_; after
this, remove stop_source_ or alternatively make worker thread functions accept
and use the shared stop_source_.get_token() so the original stop_source_ becomes
effective (update worker thread launch code and stop() accordingly).
- Around line 124-130: The destructor currently deletes impl_ before running
shutdown() and destroy(), creating a use-after-free if those methods or
callbacks (e.g., post(), impl_->stop()) access impl_; fix it by reordering
thread_pool::~thread_pool() so it first calls shutdown() and destroy() and only
after those complete deletes impl_, ensuring impl_ remains valid during
base-class cleanup and any callbacks.
|
An automated preview of the documentation is available at https://86.capy.prtest3.cppalliance.org/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-01-20 10:32:05 UTC |
|
GCOVR code coverage report https://86.capy.prtest3.cppalliance.org/gcovr/index.html Build time: 2026-01-20 10:41:28 UTC |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ex/thread_pool.cpp (1)
47-103: Synchronizestop()with lazy-start and make stop sticky to prevent races and orphaned work.The implementation has two issues:
Data race on
threads_vector:stop()iteratesthreads_whileensure_started()modifies it undercall_once. Ifstop()is called concurrently withpost()(a reasonable scenario where the main thread stops the pool while executor operations trigger post internally), this is an unsynchronized read/write on a vector.Non-sticky stop semantic: Calling
stop()before the firstpost()has no persistent effect. A subsequentpost()will triggerensure_started(), creating fresh threads that bypass the earlierstop()signal. This is semantically wrong even in single-threaded usage—once stopped, the pool should not create new work threads.The proposed fix correctly addresses both by introducing:
- An atomic
stopped_flag to make stop sticky- A
start_mutex_to synchronize thread creation/iteration- Early returns in
post()andensure_started()if already stoppedSuggested fix
+#include <atomic> + class thread_pool::impl { std::mutex mutex_; std::condition_variable_any cv_; intrusive_queue<work> q_; std::vector<std::jthread> threads_; std::size_t num_threads_; std::once_flag start_flag_; + std::mutex start_mutex_; + std::atomic<bool> stopped_{false}; ... void post(any_coro h) { + if(stopped_.load(std::memory_order_acquire)) + return; ensure_started(); auto* w = new work(h); ... } ... void stop() noexcept { + stopped_.store(true, std::memory_order_release); + std::scoped_lock lk(start_mutex_); for (auto& t : threads_) t.request_stop(); cv_.notify_all(); } ... void ensure_started() { + if(stopped_.load(std::memory_order_acquire)) + return; std::call_once(start_flag_, [this]{ + std::scoped_lock lk(start_mutex_); + if(stopped_.load(std::memory_order_acquire)) + return; threads_.reserve(num_threads_); for(std::size_t i = 0; i < num_threads_; ++i) threads_.emplace_back([this](std::stop_token st){ run(st); }); }); }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/unit/Jamfile`:
- Around line 29-32: The buffers tests lost the ASIO dependency and required
Windows define when the Jamfiles were consolidated; update the Jamfile by adding
a dedicated glob-tree-ex loop for buffers sources (e.g., match asio.cpp) that
invokes run $(f) with the ASIO library stanza
(<library>/boost/asio//boost_asio/<warnings-as-errors>off) and the define
_WIN32_WINNT=0x0601, or alternatively add that ASIO library and define to the
top-level project requirements so the asio.cpp test will find ASIO headers and
build successfully.
| for local f in [ glob-tree-ex . : *.cpp : file*.cpp ] | ||
| { | ||
| run $(f) ; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for any remaining buffers-specific configuration in Jamfiles
rg -n --glob 'Jamfile*' -C2 'buffers'Repository: cppalliance/capy
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check the current structure of test/unit directory
fd --type f 'Jamfile*' test/unit | head -20Repository: cppalliance/capy
Length of output: 77
🏁 Script executed:
#!/bin/bash
# Get the content of test/unit/Jamfile to see the full context
cat test/unit/JamfileRepository: cppalliance/capy
Length of output: 1274
🏁 Script executed:
#!/bin/bash
# Check for any buffers subdirectory and its contents
fd --type f '\.(cpp|h)$' test/unit/buffers 2>/dev/null | head -20Repository: cppalliance/capy
Length of output: 694
🏁 Script executed:
#!/bin/bash
# List all subdirectories in test/unit to see the current structure
fd --type d . test/unit | head -20Repository: cppalliance/capy
Length of output: 195
🏁 Script executed:
#!/bin/bash
# Check git history for buffers/Jamfile
git log --oneline --all -- 'test/unit/buffers/Jamfile' 2>/dev/null | head -10Repository: cppalliance/capy
Length of output: 188
🏁 Script executed:
#!/bin/bash
# Search for any comments or notes about buffers consolidation
rg -n 'buffers' test/unit/JamfileRepository: cppalliance/capy
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check if there are any special dependencies or requirements mentioned in buffers test files
rg -l 'boost/filesystem|requirements' test/unit/buffers/*.cpp | head -5Repository: cppalliance/capy
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check the commits that touched buffers/Jamfile to see what was there
git show a915f4d:test/unit/buffers/Jamfile 2>/dev/null || echo "File not in that commit"
git show 3e0ebab:test/unit/buffers/Jamfile 2>/dev/null || echo "File not in that commit"Repository: cppalliance/capy
Length of output: 1994
🏁 Script executed:
#!/bin/bash
# Check if buffers/Jamfile still exists
ls -la test/unit/buffers/Jamfile 2>/dev/null || echo "Jamfile does not exist in buffers"Repository: cppalliance/capy
Length of output: 93
🏁 Script executed:
#!/bin/bash
# Check the diff of the commit that may have removed it
git show --stat a915f4d | grep -i jamfileRepository: cppalliance/capy
Length of output: 242
🏁 Script executed:
#!/bin/bash
# Check if buffers tests include ASIO headers
rg 'boost/asio|boost.asio' test/unit/buffers/ 2>/dev/null | head -10Repository: cppalliance/capy
Length of output: 119
🏁 Script executed:
#!/bin/bash
# Check the asio.cpp test specifically
cat test/unit/buffers/asio.cppRepository: cppalliance/capy
Length of output: 5304
Add ASIO library dependency for buffers tests.
The consolidation removed the <library>/boost/asio//boost_asio/<warnings-as-errors>off and Windows define (<define>_WIN32_WINNT=0x0601) requirements that were in test/unit/buffers/Jamfile. The asio.cpp test depends on ASIO headers and won't build without this library dependency. Consider adding a separate glob-tree-ex loop for buffers tests (similar to the file*.cpp loop on lines 34-39) with the required ASIO library, or add ASIO to the top-level project requirements.
🤖 Prompt for AI Agents
In `@test/unit/Jamfile` around lines 29 - 32, The buffers tests lost the ASIO
dependency and required Windows define when the Jamfiles were consolidated;
update the Jamfile by adding a dedicated glob-tree-ex loop for buffers sources
(e.g., match asio.cpp) that invokes run $(f) with the ASIO library stanza
(<library>/boost/asio//boost_asio/<warnings-as-errors>off) and the define
_WIN32_WINNT=0x0601, or alternatively add that ASIO library and define to the
top-level project requirements so the asio.cpp test will find ASIO headers and
build successfully.
2 participants
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.