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 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
Copy link
Member

nateberkopec commented Apr 12, 2020

LGTM, anyone else have any thoughts?

lib/puma/thread_pool.rb Outdated Show resolved Hide resolved
@wjordan wjordan force-pushed the wjordan:thread_pool_shutdown_timeout branch from d5bca80 to e8e4283 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
@wjordan wjordan force-pushed the wjordan:thread_pool_shutdown_timeout branch from 09702d1 to ccc1d35 Apr 16, 2020
@nateberkopec nateberkopec merged commit ec2cda3 into puma:master Apr 17, 2020
27 checks passed
27 checks passed
ubuntu 2.2
Details
build
Details
ubuntu 2.3
Details
ubuntu 2.4
Details
ubuntu 2.5
Details
ubuntu 2.6
Details
ubuntu 2.7
Details
ubuntu head
Details
ubuntu jruby
Details
ubuntu truffleruby-head
Details
macos 2.2
Details
macos 2.3
Details
macos 2.4
Details
macos 2.5
Details
macos 2.6
Details
macos 2.7
Details
macos head
Details
macos jruby
Details
macos truffleruby-head
Details
windows 2.2
Details
windows 2.3
Details
windows 2.4
Details
windows 2.5
Details
windows 2.6
Details
windows 2.7
Details
windows mingw
Details
optional: ubuntu jruby-head optional: ubuntu jruby-head
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.