-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Periodically send status to systemd #2833
Conversation
lib/puma/systemd.rb
Outdated
sleep_time = 1.0 | ||
log "Sending status to systemd every #{sleep_time.round(1)} sec" | ||
|
||
Thread.new do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than create threads ourselves, puma
should provide an API that a puma plugin can use to manage this process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds like a good idea because it should also be used to stop. Just like the thread for the watchdog. It may be more important in the test suite, but right now it may be leaking the thread.
And there is actually for plugins some code for plugins with threads:
Lines 60 to 71 in eb59ed0
def add_background(blk) | |
@background << blk | |
end | |
def fire_background | |
@background.each_with_index do |b, i| | |
Thread.new do | |
Puma.set_thread_name "plgn bg #{i}" | |
b.call | |
end | |
end | |
end |
Perhaps that should be utilized instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL 😆 Yup that's fine too.
Happy to make this happen, but we should take the approach of providing APIs necessary for a plugin to provide this functionality. |
This is still very much a draft since I didn't actually test this yet, but I took a stab at rewriting the systemd integration as a plugin that is automatically loaded. I wonder if this is a good direction to take. If so, I can clean up the patch and split it in two (first one that transforms it into a plugin, then one that adds status watching). |
To me this looks great. I think if you rebase and add tests it's good to go. |
With systemd it's possible to send a status line. This will show up when users run systemctl status puma.service. Most of this code is based on puma-plugin-systemd.
The primary motiviation for this is that plugins have native integration for background threads. This is much cleaner since it allows tracking of those. For example, it's possible to clean up those threads in the test suite.
I've rebased it to resolve conflicts, but I don't have time to write tests right now. If anyone does have the time, I'd highly welcome it. |
Adding the |
Closing in favour of #3006 |
Description
With systemd it's possible to send a status line. This will show up when users run systemctl status puma.service.
Most of this code is based on puma-plugin-systemd.
At the moment this is completely untested, but I had written some code that I wanted to share. If anyone wants to finish this before I do, they should feel welcome to.
Closes #2604
Your checklist for this pull request
[ci skip]
to the title of the PR.#issue
" to the PR description or my commit messages.