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 deadlock issue in thread pool #2656

Merged
merged 1 commit into from Jul 7, 2021
Merged

Fix deadlock issue in thread pool #2656

merged 1 commit into from Jul 7, 2021

Conversation

olivierbellone
Copy link
Contributor

Description

Fix deadlock issue in thread pool.

This deadlock can happen when a thread is being trimmed, if the main server thread has already accepted a new incoming request and is waiting for the thread pool to not be full.

I was unable to write a test for this (race conditions are hard to reproduce!), but I was able to confirm this is a regression introduced in 5.0 by #2220. Previously, Puma did call not_full.signal when a thread was being trimmed: https://github.com/puma/puma/blob/v4.3.8/lib/puma/thread_pool.rb#L109.

I'm happy to add a test if anyone has suggestions for how to write one :)

Fixes #2655.

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added (or updated) 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.

@cjlarose cjlarose self-assigned this Jul 2, 2021
@cjlarose cjlarose added the waiting-for-review Waiting on review from anyone label Jul 2, 2021
@cjlarose
Copy link
Member

cjlarose commented Jul 3, 2021

@wjordan Any thoughts on if it'd be possible to write a test for this?

History.md Outdated Show resolved Hide resolved
@olivierbellone
Copy link
Contributor Author

@cjlarose Do you think there's any chance this could be merged and released this week? This bug is affecting our production servers and I'd much rather do a clean Puma upgrade rather than monkey patch my fix in. Totally understand if that doesn't work for you though.

dentarg added a commit that referenced this pull request Jul 7, 2021
Copy link
Member

@cjlarose cjlarose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reviewed the changes and this looks great. I'm comfortable merging without a regression test given the nature of the problem.

@cjlarose
Copy link
Member

cjlarose commented Jul 7, 2021

@olivierbellone I'm comfortable merging, but I can't guarantee a release this week. I'll bring it up with the team, but recall that you can always pull dependencies directly from git in your Gemfile.

@cjlarose cjlarose merged commit 1893628 into puma:master Jul 7, 2021
@olivierbellone olivierbellone deleted the ob-fix-2655 branch July 7, 2021 20:01
@olivierbellone
Copy link
Contributor Author

@cjlarose Thanks! Appreciate the quick merge. Using an untagged version is unfortunately not an option for us, but I'll figure something out.

nateberkopec pushed a commit that referenced this pull request Jul 13, 2021
@cjlarose
Copy link
Member

@olivierbellone This change is available in Puma 5.4.0 now.

@olivierbellone
Copy link
Contributor Author

Awesome! Thank you very much.

@MSP-Greg MSP-Greg added bug and removed waiting-for-review Waiting on review from anyone labels Jul 30, 2021
JuanitoFatas pushed a commit to JuanitoFatas/puma that referenced this pull request Sep 9, 2022
JuanitoFatas pushed a commit to JuanitoFatas/puma that referenced this pull request Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Thread pool deadlock
4 participants