Skip to content

Fix thread_pool shutdown race#199

Merged
mvandeberg merged 1 commit into
cppalliance:developfrom
mvandeberg:bug/thread-pool-deadlock
Mar 4, 2026
Merged

Fix thread_pool shutdown race#199
mvandeberg merged 1 commit into
cppalliance:developfrom
mvandeberg:bug/thread-pool-deadlock

Conversation

@mvandeberg

@mvandeberg mvandeberg commented Mar 4, 2026

Copy link
Copy Markdown
Contributor

thread_pool::impl::stop() set the stop_ flag without holding the mutex, creating a window where notify_all() could fire before a worker thread entered cv_.wait(), causing a permanent hang. This was the root cause of async_event test timeouts on slower FreeBSD CI runners.

Fix by acquiring the mutex before setting stop_, per the C++ standard requirement that shared variables used in condition_variable predicates must be modified under the mutex.

Also demote stop_ from std::atomic<bool> to plain bool since all accesses are now mutex-protected.

Summary by CodeRabbit

  • Refactor
    • Internal changes to thread-pool synchronization to improve reliability and consistency of shutdown and waiting behavior, reducing subtle concurrency issues without changing public interfaces.

@coderabbitai

coderabbitai Bot commented Mar 4, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 6abaec83-3f6f-4faf-9089-d7444b3fc9e4

📥 Commits

Reviewing files that changed from the base of the PR and between 0b568f0 and 04146ae.

📒 Files selected for processing (1)
  • src/ex/thread_pool.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/ex/thread_pool.cpp

📝 Walkthrough

Walkthrough

Replaces stop_ atomic with a mutex-guarded bool in src/ex/thread_pool.cpp. stop() now sets the flag under the mutex; the worker wait predicate and shutdown checks read the mutex-protected stop_. The <atomic> include was removed.

Changes

Cohort / File(s) Summary
Thread Pool Synchronization
src/ex/thread_pool.cpp
Replaced std::atomic<bool> stop_ with bool stop_ protected by a mutex; updated stop() to lock the mutex when setting the flag; changed condition-variable wait predicate and shutdown checks to read stop_ under mutex; removed <atomic> include.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Poem

🐇 I guard the flag with cozy locks,
No atomic leaps or memory shocks,
Threads now wait with mutex song,
Quiet stops where they belong,
A carrot-coded sync — hop along! 🥕

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Fix thread_pool shutdown race' directly and clearly describes the main change: fixing a race condition in the thread pool shutdown mechanism.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

thread_pool::impl::stop() set the stop_ flag without holding the
mutex, creating a window where notify_all() could fire before a
worker thread entered cv_.wait(), causing a permanent hang. This
was the root cause of async_event test timeouts on slower FreeBSD
CI runners.

Fix by acquiring the mutex before setting stop_, per the C++
standard requirement that shared variables used in condition_variable
predicates must be modified under the mutex.

Also demote stop_ from std::atomic<bool> to plain bool since all
accesses are now mutex-protected.
@mvandeberg
mvandeberg force-pushed the bug/thread-pool-deadlock branch from 0b568f0 to 04146ae Compare March 4, 2026 16:20
@cppalliance-bot

Copy link
Copy Markdown

An automated preview of the documentation is available at https://199.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-04 16:26:47 UTC

@cppalliance-bot

Copy link
Copy Markdown

GCOVR code coverage report https://199.capy.prtest3.cppalliance.org/gcovr/index.html
LCOV code coverage report https://199.capy.prtest3.cppalliance.org/genhtml/index.html
Coverage Diff Report https://199.capy.prtest3.cppalliance.org/diff-report/index.html

Build time: 2026-03-04 16:34:40 UTC

@cppalliance-bot

Copy link
Copy Markdown

GCOVR code coverage report https://199.capy.prtest3.cppalliance.org/gcovr/index.html
LCOV code coverage report https://199.capy.prtest3.cppalliance.org/genhtml/index.html
Coverage Diff Report https://199.capy.prtest3.cppalliance.org/diff-report/index.html

Build time: 2026-03-04 16:40:29 UTC

@mvandeberg
mvandeberg merged commit 7715a31 into cppalliance:develop Mar 4, 2026
32 of 33 checks passed
@github-project-automation github-project-automation Bot moved this from Backlog to Done in Beast2 Mar 4, 2026
@mvandeberg
mvandeberg deleted the bug/thread-pool-deadlock branch March 4, 2026 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants