Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ThreadPool#shutdown timeout accuracy #2221

Merged
merged 1 commit into from Apr 17, 2020

Conversation

wjordan
Copy link
Contributor

@wjordan wjordan commented Apr 10, 2020

Description

Fixes up ThreadPool#shutdown so that the provided timeout value (which corresponds to the force_shutdown_after configuration option) is more accurate, and can be set as a float instead of just an integer value.

Previously, the shutdown timeout ran a loop timeout.times, which called t.join 1 on each thread (one after another, not parallel), followed by a sleep 1 each loop iteration, then t.join SHUTDOWN_GRACE_TIME on each thread after raising ForceShutdown. Instead of forcing shutdown in timeout (+ SHUTDOWN_GRACE_TIME) seconds, this forced a shutdown in timeout * (n + 1) (+ n * SHUTDOWN_GRACE_TIME) seconds (where n is the number of running threads). The updated code fixes this by joining a maximum of timeout seconds (followed by a maximum of grace seconds) across all threads.

I also added an extra step at the end that calls kill on any remaining threads still alive after the grace period, plus another short (1-second) join to wait for force-killed threads to (finally) exit, as an extra precaution that threads are fully cleaned up when shutdown returns. I thought this might be helpful, but could be removed as it's not strictly needed for the rest of this PR.

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] the pull request title.
  • I have added appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

History.md Outdated Show resolved Hide resolved
@nateberkopec nateberkopec added the waiting-for-changes Waiting on changes from the requestor label Apr 12, 2020
@nateberkopec
Copy link
Member

LGTM, anyone else have any thoughts?

lib/puma/thread_pool.rb Outdated Show resolved Hide resolved
@wjordan wjordan force-pushed the thread_pool_shutdown_timeout branch from d5bca80 to e8e4283 Compare April 13, 2020 05:30
@nateberkopec nateberkopec added waiting-for-review Waiting on review from anyone and removed waiting-for-changes Waiting on changes from the requestor labels Apr 13, 2020
lib/puma/thread_pool.rb Show resolved Hide resolved
lib/puma/thread_pool.rb Outdated Show resolved Hide resolved
test/helper.rb Outdated Show resolved Hide resolved
@nateberkopec nateberkopec added waiting-for-changes Waiting on changes from the requestor and removed waiting-for-review Waiting on review from anyone labels Apr 14, 2020
@wjordan wjordan force-pushed the thread_pool_shutdown_timeout branch from 09702d1 to ccc1d35 Compare April 16, 2020 19:03
@nateberkopec nateberkopec merged commit ec2cda3 into puma:master Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature maintenance waiting-for-changes Waiting on changes from the requestor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants