ThreadPool concurrency refactoring (#2220)#3
ThreadPool concurrency refactoring (#2220)#3MitchLewis930 wants to merge 1 commit intopr_053_beforefrom
Conversation
- Wait for threads to enter waiting loop on ThreadPool startup - Simplify #spawn_thread inner threadpool loop - Refactor TestThreadPool to make tests faster and more stable Co-authored-by: Nate Berkopec <nate.berkopec@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors ThreadPool concurrency management to fix race conditions and improve synchronization, particularly for non-GVL Ruby implementations.
Changes:
- Introduces
MutexPooltest helper class that wraps ThreadPool with mutex-based synchronization for deterministic test execution - Refactors thread lifecycle management in ThreadPool to use
Thread.exitinstead of break statements and moves cleanup logic - Adds
with_mutexhelper method to prevent potential deadlocks when mutex is already owned
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/test_thread_pool.rb | Adds MutexPool helper class and refactors tests to use it, removing sleep-based synchronization and platform-specific skips |
| lib/puma/thread_pool.rb | Refactors thread spawn/exit logic, adds with_mutex helper, and improves trim request handling during shutdown |
| History.md | Documents the concurrency bug fixes and refactoring in the changelog |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @spawned -= 1 | ||
| @workers.delete th | ||
| Thread.exit |
There was a problem hiding this comment.
Thread cleanup (decrementing @Spawned and removing from @Workers) happens inside the mutex before Thread.exit, but the original code performed cleanup after the thread loop ended. This change means if Thread.exit fails or is interrupted, the cleanup has already occurred, potentially leaving @Spawned and @Workers in an inconsistent state. Consider whether cleanup should remain after the thread exits to ensure accurate accounting.
| free = @waiting - @todo.size | ||
| if (force or free > 0) and @spawned - @trim_requested > @min |
There was a problem hiding this comment.
The variable name 'free' is ambiguous. Consider renaming it to 'free_threads' or 'idle_threads' to clarify that it represents the number of threads available to handle new work.
| free = @waiting - @todo.size | |
| if (force or free > 0) and @spawned - @trim_requested > @min | |
| free_threads = @waiting - @todo.size | |
| if (force or free_threads > 0) and @spawned - @trim_requested > @min |
2 participants
PR_053