Fix MSVC thread_pool join test failure#228
Fix MSVC thread_pool join test failure#228mvandeberg wants to merge 1 commit intocppalliance:developfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis change replaces Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant CoroutineHandle as "coroutine_handle<h>"
participant Promise
participant DestroyerAwaiter as "destroyer awaiter"
Caller->>CoroutineHandle: resume / run coroutine
CoroutineHandle->>Promise: reaches final_suspend()
Promise->>DestroyerAwaiter: return destroyer awaiter
CoroutineHandle->>DestroyerAwaiter: await_suspend(handle)
DestroyerAwaiter->>CoroutineHandle: h.destroy()
DestroyerAwaiter-->>Caller: completes (no resume value)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
include/boost/capy/ex/run_async.hpp (1)
262-275: Consider extracting the shareddestroyerawaiter.The
destroyerstruct is duplicated between the primary template and this specialization. While acceptable for detail code, you could extract it to reduce duplication.♻️ Optional: Extract shared destroyer awaiter
Add before the
run_async_trampolineprimary template:/// Awaiter that explicitly destroys the coroutine frame at final suspension. /// Returning void from await_suspend provides a clean exit for MSVC's /// symmetric transfer trampoline loop. struct final_destroyer { bool await_ready() noexcept { return false; } void await_suspend(std::coroutine_handle<> h) noexcept { h.destroy(); } void await_resume() noexcept {} };Then in both
promise_type::final_suspend()implementations:auto final_suspend() noexcept { - struct destroyer - { - bool await_ready() noexcept { return false; } - void await_suspend( - std::coroutine_handle<> h) noexcept - { - h.destroy(); - } - void await_resume() noexcept {} - }; - return destroyer{}; + return final_destroyer{}; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/capy/ex/run_async.hpp` around lines 262 - 275, The duplicate local destroyer awaiter in promise_type::final_suspend should be extracted into a shared type: add a struct named (for example) final_destroyer before the run_async_trampoline primary template with the same members and noexcept signatures (bool await_ready() noexcept, void await_suspend(std::coroutine_handle<> h) noexcept, void await_resume() noexcept), then replace the existing local destroyer return statements in both promise_type::final_suspend() implementations with return final_destroyer{}; to remove duplication and keep behavior identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@include/boost/capy/ex/run_async.hpp`:
- Around line 262-275: The duplicate local destroyer awaiter in
promise_type::final_suspend should be extracted into a shared type: add a struct
named (for example) final_destroyer before the run_async_trampoline primary
template with the same members and noexcept signatures (bool await_ready()
noexcept, void await_suspend(std::coroutine_handle<> h) noexcept, void
await_resume() noexcept), then replace the existing local destroyer return
statements in both promise_type::final_suspend() implementations with return
final_destroyer{}; to remove duplication and keep behavior identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 05f5a17e-cde2-4153-8f40-734ad835ca6e
📒 Files selected for processing (1)
include/boost/capy/ex/run_async.hpp
|
An automated preview of the documentation is available at https://228.capy.prtest3.cppalliance.org/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-03-13 21:08:25 UTC |
|
GCOVR code coverage report https://228.capy.prtest3.cppalliance.org/gcovr/index.html Build time: 2026-03-13 21:22:21 UTC |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #228 +/- ##
===========================================
- Coverage 92.41% 92.37% -0.04%
===========================================
Files 162 162
Lines 8854 8976 +122
===========================================
+ Hits 8182 8292 +110
- Misses 672 684 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 21 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
The run_async trampoline coroutine used suspend_never for final_suspend, relying on automatic frame destruction when the coroutine falls through. MSVC's symmetric transfer implementation (which uses an internal trampoline loop rather than true tail calls) can mishandle this pattern, potentially double-destroying the frame. When the work_guard destructor fires twice, outstanding_work_ reaches zero one task early, stop_ is set, and the remaining queued task is abandoned without its handler running. Replace suspend_never with an explicit destroyer awaiter that calls h.destroy() in await_suspend and returns void. This gives MSVC's symmetric transfer loop a clean exit point and avoids the problematic auto-destruction codepath. Both trampoline specializations (allocator-based and memory_resource*) are updated.
mvandeberg
force-pushed
the
pr/msvc-thread-pool
branch
from
600f672 to
f4f9db6
Compare
2 participants
The run_async trampoline coroutine used suspend_never for final_suspend, relying on automatic frame destruction when the coroutine falls through. MSVC's symmetric transfer implementation (which uses an internal trampoline loop rather than true tail calls) can mishandle this pattern, potentially double-destroying the frame. When the work_guard destructor fires twice, outstanding_work_ reaches zero one task early, stop_ is set, and the remaining queued task is abandoned without its handler running.
Replace suspend_never with an explicit destroyer awaiter that calls h.destroy() in await_suspend and returns void. This gives MSVC's symmetric transfer loop a clean exit point and avoids the problematic auto-destruction codepath. Both trampoline specializations (allocator-based and
memory_resource*) are updated.
Summary by CodeRabbit
Bug Fixes
Documentation