ThreadPool concurrency refactoring #2220
Merged
Conversation
This file contains 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
- 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
wjordan
force-pushed
the
thread_pool_refactor
branch
from
8d61027
to
e1daf1b
Compare
| @@ -36,6 +36,7 @@ | |||
| * Simplify `Runner#start_control` URL parsing (#2111) | |||
| * Removed the IOBuffer extension and replaced with Ruby (#1980) | |||
| * Update `Rack::Handler::Puma.run` to use `**options` (#2189) | |||
| * ThreadPool concurrency refactoring (#2220) | |||
There was a problem hiding this comment.
I think the trim bugfix could also be mentioned
|
I'll have to find some time to review this proper, but the test changes are gorgeous 😍 |
| @@ -99,20 +102,13 @@ def spawn_thread | |||
| while true | |||
| work = nil | |||
|
|
|||
| continue = true | |||
|
|
|||
| mutex.synchronize do | |||
There was a problem hiding this comment.
Clever change and much more readable
| end | ||
| Thread.pass until finish | ||
| pool.signal | ||
| sleep |
There was a problem hiding this comment.
typo or are you triggering a context switch?
dmarcotte
added a commit
to looker/puma
that referenced
this pull request
Apr 23, 2020
Well, "fix" might overstate it, but we were able to get them passing by backporting test skips that are currently in Puma master upstream. With these skips, not only does tag `4.3.3.` all pass, but so does our custom branch, so we can be pretty confident we haven't broken anything vs. stock Puma. Some notes on test runs: - Invoke with `bundle exec rake test --trace` - Ctrl-C'ing the tests usually left a zombified version running in the background, which would *cause failures on subsequent runs*. Ensure after killing the a test run that no test processes remain in `jps -lm` - `TestThreadPool` tests are flaky in this release, though this was a known issue, and has been recently fixed here: puma#2220
JohnPhillips31416
pushed a commit
to looker/puma
that referenced
this pull request
May 29, 2020
Well, "fix" might overstate it, but we were able to get them passing by backporting test skips that are currently in Puma master upstream. With these skips, not only does tag `4.3.3.` all pass, but so does our custom branch, so we can be pretty confident we haven't broken anything vs. stock Puma. Some notes on test runs: - Invoke with `bundle exec rake test --trace` - Ctrl-C'ing the tests usually left a zombified version running in the background, which would *cause failures on subsequent runs*. Ensure after killing the a test run that no test processes remain in `jps -lm` - `TestThreadPool` tests are flaky in this release, though this was a known issue, and has been recently fixed here: puma#2220
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
This PR refactors some tricky concurrency parts of
TestThreadPool, in order to make the unit tests more stable (all tests pass on jruby), faster (no more 1-second sleeps), and a bit more readable. Many of the tests now use a subclass ofThreadPoolthat overrides#<<to wait on the existing@not_fullcondition variable, to ensure work begins executing on a worker thread before continuing the test.There are also some small changes to the
ThreadPoolclass itself:#with_mutexto make it easier to chain multiple@mutex.synchronize-wrapped methods together, and to extend existing mutex-wrapped methods in unit tests.#initialize. This fixes a subtle edge-case concurrency bug mostly affecting unit-test reliability, demonstrated by the following simple test that fails onmaster:puma/test/test_thread_pool.rb
Lines 239 to 242 in 8d61027
#trimwhere a trim could still be requested even when enough work to utilize all threads has been queued but not yet picked up by workers (e.g.,waiting > 0butwaiting - todo.size == 0). (I believe this was the cause of some jruby test flakiness in sometrimunit tests.)spawn_threadto make it a bit simpler and easier to read and reason about.Your checklist for this pull request
[changelog skip]the pull request title.[ci skip]to the title of the PR.#issue" to the PR description or my commit messages.