Fix timer_service crash when use_service races on multi-threaded pool#230
Conversation
When multiple pool threads call use_service<timer_service>() concurrently, the double-checked lock in use_service_impl can create a duplicate that is immediately deleted. Without a destructor, the std::thread member is destroyed non-joined, calling std::terminate(). Add ~timer_service() that calls shutdown() to join the background thread, and a concurrent delay test that reproduces the bug.
📝 WalkthroughWalkthroughAdds explicit resource cleanup to the timer_service class by introducing a public destructor and private Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes 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 for PR comments
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/detail/timer_service.hpp (1)
50-53: Minor comment inconsistency: destructor callsstop_and_join(), notshutdown().The comment states "Calls shutdown()" but the implementation directly calls
stop_and_join(). While functionally equivalent (sinceshutdown()also delegates tostop_and_join()), the comment could confuse maintainers inspecting the destructor implementation.📝 Suggested comment clarification
- // Calls shutdown() to join the background thread. + // Calls stop_and_join() to join the background thread. // Handles the discard path in use_service_impl where // a duplicate service is deleted without shutdown(). ~timer_service();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/capy/ex/detail/timer_service.hpp` around lines 50 - 53, Update the destructor comment for timer_service to accurately reflect the implementation: replace "Calls shutdown()" with "Calls stop_and_join()" (or mention both with clarification that shutdown() delegates to stop_and_join()) so it matches the destructor behavior and clarifies the discard path in use_service_impl where duplicates may be deleted without shutdown().
🤖 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/detail/timer_service.hpp`:
- Around line 50-53: Update the destructor comment for timer_service to
accurately reflect the implementation: replace "Calls shutdown()" with "Calls
stop_and_join()" (or mention both with clarification that shutdown() delegates
to stop_and_join()) so it matches the destructor behavior and clarifies the
discard path in use_service_impl where duplicates may be deleted without
shutdown().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bc1ffa6b-4149-49a1-bd0e-fbde037ae665
⛔ Files ignored due to path filters (1)
test/unit/delay.cppis excluded by!**/test/**
📒 Files selected for processing (2)
include/boost/capy/ex/detail/timer_service.hppsrc/ex/detail/timer_service.cpp
|
An automated preview of the documentation is available at https://230.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-12 16:41:56 UTC |
|
GCOVR code coverage report https://230.capy.prtest3.cppalliance.org/gcovr/index.html Build time: 2026-03-12 16:55:10 UTC |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #230 +/- ##
===========================================
+ Coverage 92.38% 92.39% +0.01%
===========================================
Files 162 162
Lines 8854 8866 +12
===========================================
+ Hits 8180 8192 +12
Misses 674 674
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
2 participants
When multiple pool threads call use_service<timer_service>() concurrently, the double-checked lock in use_service_impl can create a duplicate that is immediately deleted. Without a destructor, the std::thread member is destroyed non-joined, calling std::terminate().
Add ~timer_service() that calls shutdown() to join the background thread, and a concurrent delay test that reproduces the bug.
Summary by CodeRabbit
Release Notes