Navigation Menu

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

out_of_band hook not working when using multiple threads #2177

Closed
GuiTeK opened this issue Mar 12, 2020 · 7 comments · Fixed by #2218
Closed

out_of_band hook not working when using multiple threads #2177

GuiTeK opened this issue Mar 12, 2020 · 7 comments · Fixed by #2218
Labels

Comments

@GuiTeK
Copy link

GuiTeK commented Mar 12, 2020

Describe the bug
The out_of_band hook (C.F. #1648) seems not to work when the number of threads specified with threads N, N is > 1.

Puma config:

threads 2, 2
port        ENV.fetch("PUMA_PORT") { 3000 }
environment ENV.fetch("RAILS_ENV") { "development" }
workers ENV.fetch("PUMA_WORKERS") { 1 }

out_of_band do
  puts 'OOB Hook Working'
end

preload_app!

plugin :tmp_restart

Command: bundle exec puma -C config/puma.rb oob.ru

Contents of oob.ru:

run lambda { |env| [200, {"Content-Type" => "text/plain"}, ["Hello World"]] }

To Reproduce

  1. Make sure config/puma.rb and oob.ru have the contents showed above
  2. Run bundle exec puma -C config/puma.rb oob.ru
  3. Make some requests: curl http://127.0.0.1:3000/
  4. Note that OOB Hook Working does NOT get printed in Puma output

Now, to show it works with threads 1, 1:

  1. Stop the Puma server if already running
  2. Replace threads 2, 2 in the Puma config with threads 1, 1
  3. Run bundle exec puma -C config/puma.rb oob.ru
  4. Make some requests: curl http://127.0.0.1:3000/
  5. Note that OOB Hook Working IS present in Puma output

Expected behavior
I expect out_of_band hook to be called even when Puma is configured to run with multiple threads.

Desktop:

  • OS: Mac OS X 10.14
  • Puma Version: 4.3.3
@nateberkopec
Copy link
Member

@GuiTeK Just curious, what do you use out_of_band for?

@GuiTeK
Copy link
Author

GuiTeK commented Mar 12, 2020

@nateberkopec I'd like to use it for Garbage Collection (to run GC.start).

@nateberkopec
Copy link
Member

I see. So, if we provide a feature in Puma, it should obviously work, and this definitely doesn't, so I want to fix that.

However, OOBGC is an outdated technique since the changes introduced in the Ruby 2.3 GC. I can't recommend it. GitHub disabled it after they realized it was causing an additional 10% CPU usage.

@wjordan
Copy link
Contributor

wjordan commented Apr 6, 2020

I contributed the PR #1648 that implemented the out_of_band hook. I use threads 1, N in my application which may be why I didn't consider the N, N case very thoroughly in the implementation. I didn't add tests at the time but perhaps it's now worth adding some to pin down the behavior a bit more carefully.

However, OOBGC is an outdated technique since the changes introduced in the Ruby 2.3 GC. I can't recommend it. GitHub disabled it after they realized it was causing an additional 10% CPU usage.

My use-case for this feature has also been OOBGC, here are my previous comments on the topic #450 (comment):

OOBGC still provides my own production workload ~10% faster response times on Ruby 2.5 (see tmm1/gctools#16 (comment) for some details/discussion). Whether OOBGC ends up being useful or not will depend a lot on workload, exact tuning of GC variables, and also the exact OOBGC algorithm used.

@nateberkopec
Copy link
Member

That has got to be a weird workload indeed. I've rarely seen GC time >1% for apps I've worked on. I wonder if your workload is also part of the reason you see a lot of improvement on your dirty-fork PR.

@wjordan wjordan mentioned this issue Apr 7, 2020
8 tasks
@wjordan
Copy link
Contributor

wjordan commented Apr 7, 2020

After looking at this more closely, it does look like the OOB hook never worked in any configuration with threads > 1. (It's been running in my production workload, but on a fork from which I've extracted various feature PRs, and I should have tested/verified the extracted out-of-band PR more thoroughly. Sorry!)

I added my own attempt at a fix to #2218, which also includes a couple of tests.

@nateberkopec
Copy link
Member

Closing in favor of #2218

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 a pull request may close this issue.

3 participants