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 out_of_band hook never executed when multiple worker threads (clo… #2178

Closed
wants to merge 1 commit into from
Closed

Fix out_of_band hook never executed when multiple worker threads (clo… #2178

wants to merge 1 commit into from

Conversation

GuiTeK
Copy link

@GuiTeK GuiTeK commented Mar 12, 2020

Description

Alternative solution to this PR: #2180
I believe this one should be merged and #2180 should be discarded but the maintainers are probably wiser than me on Puma!

Issue description

C.F. #2177

Fix description

Before this patch, the out_of_band hook was executed only when all worker threads were free:

pool << client
busy_threads = pool.wait_until_not_full
if busy_threads == 0
  @options[:out_of_band].each(&:call) if @options[:out_of_band]
end

In addition, due to how the function wait_until_not_full (in lib/puma/thread_pool.rb) works, namely:

  • If not all threads are busy, return the number of busy threads
  • Otherwise, wait (loop) for the condition above

The condition if busy_threads == 0 could never be satisfied if the number of worker threads was > 1.

EDIT: it could be satisfied if the worker finished its job before the call to wait_until_not_full, which is very unlikely because this function is called right after adding the client to the pool.

I believe that the out_of_band hook should be allowed to run if there is at least 1 available worker thread. It seems reasonable to me and provides an easy fix for this bug.

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.

@@ -291,7 +291,7 @@ def handle_servers

pool << client
busy_threads = pool.wait_until_not_full
if busy_threads == 0
Copy link
Member

Choose a reason for hiding this comment

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

Well, the problem here is that if busy_threads != 0, it's not really out of band anymore, is it?

@nateberkopec
Copy link
Member

I think this is a non-starter because it makes the OOB hook no longer OOB and just a background thread.

@wjordan wjordan mentioned this pull request Apr 7, 2020
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug waiting-for-review Waiting on review from anyone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants