Replace per-operation thread spawning with shared thread pool#191
Merged
sgerbino merged 1 commit intocppalliance:developfrom Mar 5, 2026
Merged
Replace per-operation thread spawning with shared thread pool#191sgerbino merged 1 commit intocppalliance:developfrom
sgerbino merged 1 commit intocppalliance:developfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a fixed-size per-execution_context thread pool service and rewrites POSIX and Windows resolver implementations to offload blocking getaddrinfo/GetNameInfoW work to that pool, removing per-operation detached threads and per-service thread-tracking/shutdown primitives. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ResolverSvc as Resolver Service
participant ThreadPool as Thread Pool
participant Worker as Worker Thread
participant Scheduler as Scheduler
Client->>ResolverSvc: resolve()/reverse_resolve(op)
ResolverSvc->>ThreadPool: post(pool_work_item for op)
ThreadPool->>Worker: notify / worker dequeues task
Worker->>Worker: perform blocking getaddrinfo/getnameinfo
Worker->>ResolverSvc: post completion (via scheduler)
ResolverSvc->>Scheduler: schedule completion handler
Scheduler-->>Client: invoke completion handler
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
An automated preview of the documentation is available at https://191.corosio.prtest3.cppalliance.org/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-03-05 11:36:46 UTC |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/boost/corosio/detail/thread_pool.hpp`:
- Around line 104-124: The worker threads currently return immediately when
shutdown_ is set, which combined with work_queue_.clear() in shutdown() discards
queued tasks; change the logic so queued tasks are drained and executed before
workers exit: update thread_pool::worker_loop() to, upon seeing shutdown_, not
return immediately but first process any remaining entries in work_queue_
(pop_front and execute until empty) and then return, and remove or stop clearing
the queue in thread_pool::shutdown() (stop clearing work_queue_ there or only
stop accepting new tasks) so tasks posted before shutdown are allowed to run;
reference functions/fields: thread_pool::worker_loop, thread_pool::shutdown,
work_queue_, shutdown_, work_finished.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ea4e5772-cfa4-4ecf-9947-848d0439df23
📒 Files selected for processing (5)
include/boost/corosio/detail/thread_pool.hppinclude/boost/corosio/native/detail/iocp/win_resolver.hppinclude/boost/corosio/native/detail/iocp/win_resolver_service.hppinclude/boost/corosio/native/detail/posix/posix_resolver.hppinclude/boost/corosio/native/detail/posix/posix_resolver_service.hpp
|
GCOVR code coverage report https://191.corosio.prtest3.cppalliance.org/gcovr/index.html Build time: 2026-03-05 11:40:26 UTC |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/boost/corosio/native/detail/iocp/win_resolver_service.hpp`:
- Around line 407-414: The code calls svc_.work_started() before calling
svc_.pool().post(...), but pool().post can throw and the lambda's
svc_.work_finished() will then never run; either move the svc_.work_started()
call to after svc_.pool().post(...) succeeds, or wrap the svc_.pool().post(...)
call in a try/catch and call svc_.work_finished() from the catch block (making
sure to still capture self via shared_from_this() as before); update the call
site around svc_.pool().post([this, self = std::move(self)](){ ...
svc_.work_finished(); }) accordingly so the io_context work counter is balanced
on exceptions.
In `@include/boost/corosio/native/detail/posix/posix_resolver_service.hpp`:
- Around line 381-384: The code calls svc_.pool().post(...) after calling
on_work_started() in resolve and reverse_resolve, but if post() throws (e.g.,
std::thread construction failure) the work counter is never decremented; wrap
the svc_.pool().post(...) invocation in a try-catch that on any exception calls
on_work_finished() (or otherwise signals the corresponding operation completion
with an error) and rethrows or handles the error; update both resolve and
reverse_resolve paths (the block capturing self = shared_from_this()) to ensure
the self lifetime handling/ownership is preserved and that the failure path
reports an error back to the operation instead of leaking executor work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4e52115a-44c8-4363-ad89-3268ed33ba16
⛔ Files ignored due to path filters (1)
test/unit/thread_pool.cppis excluded by!**/test/**
📒 Files selected for processing (5)
include/boost/corosio/detail/thread_pool.hppinclude/boost/corosio/native/detail/iocp/win_resolver.hppinclude/boost/corosio/native/detail/iocp/win_resolver_service.hppinclude/boost/corosio/native/detail/posix/posix_resolver.hppinclude/boost/corosio/native/detail/posix/posix_resolver_service.hpp
🚧 Files skipped from review as they are similar to previous changes (2)
- include/boost/corosio/native/detail/iocp/win_resolver.hpp
- include/boost/corosio/detail/thread_pool.hpp
include/boost/corosio/native/detail/iocp/win_resolver_service.hpp
Outdated
Show resolved
Hide resolved
include/boost/corosio/native/detail/posix/posix_resolver_service.hpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
include/boost/corosio/detail/thread_pool.hpp (1)
13-22: Add a file-level implementation overview comment block.This file contains non-trivial worker/shutdown/concurrency behavior, but it lacks the requested
/* ... */high-level overview immediately after includes.As per coding guidelines "Files containing non-trivial implementation logic should include a
/* */block comment after the includes that provides a high-level overview of how the implementation works."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/detail/thread_pool.hpp` around lines 13 - 22, Add a file-level /* ... */ block comment immediately after the includes in thread_pool.hpp that gives a high-level overview of the thread pool's worker, shutdown and concurrency behavior; describe how workers are created and scheduled, how shutdown is signaled/handled, and any important invariants or synchronization used by symbols in this file (namespace boost::corosio::detail and the thread pool implementation such as the worker threads, shutdown path, and concurrency primitives like condition_variable/mutex) so readers can understand the design without diving into implementation details.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/boost/corosio/detail/thread_pool.hpp`:
- Around line 153-160: thread_pool::post currently pushes work under mutex_ but
shutdown() pops/clears work_queue_ without holding mutex_, causing a
race/corruption; fix by synchronizing queue access so shutdown() locks mutex_
while removing/clearing items (or swaps work_queue_ with an empty local queue
while holding mutex_) and then processes or deletes the popped items outside the
lock; update shutdown() logic that iterates/pops work_queue_ to use
std::lock_guard<std::mutex> or std::unique_lock<std::mutex> on mutex_, ensure
cv_.notify_one()/notify_all semantics remain correct (notify after releasing
lock if needed), and take care to run pending resolver completions safely after
the queue is emptied to avoid orphaning.
---
Nitpick comments:
In `@include/boost/corosio/detail/thread_pool.hpp`:
- Around line 13-22: Add a file-level /* ... */ block comment immediately after
the includes in thread_pool.hpp that gives a high-level overview of the thread
pool's worker, shutdown and concurrency behavior; describe how workers are
created and scheduled, how shutdown is signaled/handled, and any important
invariants or synchronization used by symbols in this file (namespace
boost::corosio::detail and the thread pool implementation such as the worker
threads, shutdown path, and concurrency primitives like
condition_variable/mutex) so readers can understand the design without diving
into implementation details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 52a111be-98a4-461f-b8fa-5300387a3dc8
⛔ Files ignored due to path filters (1)
test/unit/thread_pool.cppis excluded by!**/test/**
📒 Files selected for processing (5)
include/boost/corosio/detail/thread_pool.hppinclude/boost/corosio/native/detail/iocp/win_resolver.hppinclude/boost/corosio/native/detail/iocp/win_resolver_service.hppinclude/boost/corosio/native/detail/posix/posix_resolver.hppinclude/boost/corosio/native/detail/posix/posix_resolver_service.hpp
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/boost/corosio/detail/thread_pool.hpp`:
- Around line 118-119: The post(pool_work_item* w) API currently drops work
silently during shutdown; change its signature from void post(pool_work_item* w)
noexcept to return a bool (e.g., bool post(pool_work_item* w) noexcept) that
indicates enqueue success/failure, ensure thread_pool::post checks shutdown
state and returns false when it cannot enqueue, and update all callers of
thread_pool::post/pool_work_item handling so they check the returned status and
immediately complete or cancel the work when false to avoid orphaned
completions; apply the same change to any sibling overloads in the same file
that currently return void (lines referenced around post) and update related
tests and call sites accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1c746429-0a7e-48a6-82c9-2c9dbbb88813
⛔ Files ignored due to path filters (1)
test/unit/thread_pool.cppis excluded by!**/test/**
📒 Files selected for processing (5)
include/boost/corosio/detail/thread_pool.hppinclude/boost/corosio/native/detail/iocp/win_resolver.hppinclude/boost/corosio/native/detail/iocp/win_resolver_service.hppinclude/boost/corosio/native/detail/posix/posix_resolver.hppinclude/boost/corosio/native/detail/posix/posix_resolver_service.hpp
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
include/boost/corosio/detail/thread_pool.hpp (1)
13-21: Add a file-level implementation overview block after the includes.This file contains non-trivial worker lifecycle/shutdown logic and is missing the requested
/* ... */high-level maintainer overview near the include section.As per coding guidelines: “Files containing non-trivial implementation logic should include a
/* */block comment after the includes that provides a high-level overview of how the implementation works.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/detail/thread_pool.hpp` around lines 13 - 21, Add a file-level /* ... */ block comment immediately after the includes in include/boost/corosio/detail/thread_pool.hpp that gives a high-level overview of the thread pool implementation: describe the role of the thread_pool class and worker threads, the lifecycle (startup, work-stealing/dispatch, idle behavior), the shutdown sequence and coordination (how workers are signaled, how condition_variable/mutex are used, and how ongoing tasks are drained or abandoned), and any important invariants or concurrency guarantees for maintainers reading functions like thread_pool::start, thread_pool::stop/shutdown, and the worker loop. Ensure the comment is concise, non-redundant, and placed right after the include block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/boost/corosio/detail/thread_pool.hpp`:
- Around line 96-104: The thread_pool constructor must validate num_threads and
be exception-safe: ensure num_threads >= 1 (guard and adjust or throw) before
reserving threads_, then create threads inside a try-catch around the
emplace_back loop in the constructor; if emplace_back throws, set the shutdown_
flag, call cv_.notify_all(), join any threads already created in threads_, and
rethrow the exception to avoid std::terminate and leaking joinable threads;
reference the thread_pool constructor, threads_, worker_loop(), shutdown_, and
cv_ when applying these changes.
---
Nitpick comments:
In `@include/boost/corosio/detail/thread_pool.hpp`:
- Around line 13-21: Add a file-level /* ... */ block comment immediately after
the includes in include/boost/corosio/detail/thread_pool.hpp that gives a
high-level overview of the thread pool implementation: describe the role of the
thread_pool class and worker threads, the lifecycle (startup,
work-stealing/dispatch, idle behavior), the shutdown sequence and coordination
(how workers are signaled, how condition_variable/mutex are used, and how
ongoing tasks are drained or abandoned), and any important invariants or
concurrency guarantees for maintainers reading functions like
thread_pool::start, thread_pool::stop/shutdown, and the worker loop. Ensure the
comment is concise, non-redundant, and placed right after the include block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7c579e70-33e3-4254-9045-a96b886e316b
⛔ Files ignored due to path filters (1)
test/unit/thread_pool.cppis excluded by!**/test/**
📒 Files selected for processing (5)
include/boost/corosio/detail/thread_pool.hppinclude/boost/corosio/native/detail/iocp/win_resolver.hppinclude/boost/corosio/native/detail/iocp/win_resolver_service.hppinclude/boost/corosio/native/detail/posix/posix_resolver.hppinclude/boost/corosio/native/detail/posix/posix_resolver_service.hpp
sgerbino
force-pushed
the
pr/thread-pool
branch
2 times, most recently
from
ff29990 to
88fdcd2
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #191 +/- ##
===========================================
+ Coverage 76.02% 76.17% +0.15%
===========================================
Files 98 99 +1
Lines 10534 10565 +31
Branches 2390 2408 +18
===========================================
+ Hits 8008 8048 +40
+ Misses 1801 1780 -21
- Partials 725 737 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolver operations previously created and detached a new std::thread for every blocking getaddrinfo()/getnameinfo() call. Add a generic detail::thread_pool execution_context service that reuses threads across operations, and wire both POSIX and Windows resolver services to dispatch blocking work through it.
Resolves #118.
Summary by CodeRabbit
New Features
Improvements
Public API