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

[GuiTeK]

Description

This is an alternative solution for #2177.
The other PR is #2178. The difference is that this PR keeps strictly the same behaviour as before (i.e. runs OOB hook only if all threads are free).

Issue description

C.F. #2177

Fix description

Here is the code responsible for executing the out_of_band hook:

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

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. 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.

This PR fixes this problem by giving a callback function to ThreadPool.initialize to signal the Server when all worker threads are free (using the boolean variable @all_worker_threads_free).

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.

[nateberkopec]

This approach makes a bit more sense as it's still truly OOB. Need to think about implementation...

[nateberkopec]

Rather than use the pipe to communicate intra-process, couldn't we use a condition variable to signal to a thread in the server to run the oob calls?

[GuiTeK]

Hey! Thanks for the feedback @nateberkopec.

I'm not sure about the ConditionVariable, especially because the "main" thread (by that, I mean the thread in which handle_servers is running) will not always be waiting for it for the signal emitted from the worker threads (and in that case, it will be lost).

If you really want to implement it that way, can you please explain me how you would do it?

For now, I implemented it using a boolean & a mutex.

EDIT: I edited the PR description.

[GuiTeK]

I removed a now useless include Puma::Const I had added in a previous commit.

[nateberkopec]

Oh awesome! Thanks for correcting me - as weird as this may sound coming from a current maintainer of Puma, I'm not an expert on the Mutex/CV classes (that code is almost entirely still Evan's). But it seemed to me like the checkpipe was unnecessary when the threadpool and the runner were always going to be in the same process - you figured it out.

[nateberkopec]

I'm wondering about how we could test this.

[GuiTeK]

I'm wondering about how we could test this.

I don't know either, it seems there were no tests at all for the OOB hook.

[nateberkopec]

Thanks for your contribution. #2218 has tests and is a little cleaner IMO, so I'm going to close this PR.