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.

Sign up for GitHub

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

Jump to bottom

Feature | Shutdown on idle #3209

Merged
merged 7 commits into from
Sep 4, 2023
Merged

Feature | Shutdown on idle #3209

merged 7 commits into from
Sep 4, 2023

Conversation

joshuay03
Copy link
Contributor

@joshuay03 joshuay03 commented Aug 12, 2023 •
edited

Description

Closes #2580.

I attended RubyConfAU earlier this year and was inspired by this talk to contribute to puma. During my search for a small enough issue (preferably a bug) to tackle I stumbled upon the feature request linked above and noticed that it'd been open for a while. I liked the idea and thought it'd be a useful feature, and decided to give it a go.

I'm very new to this 'area' of Ruby so I might need some guidance on how sensible my implementation is...

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.

@joshuay03 joshuay03 marked this pull request as ready for review August 12, 2023 11:41
@dentarg
Copy link
Member

dentarg commented Aug 12, 2023

From #2580

There should be an (opt-in) setting that allows to specify a timeout. If no new request is received inside that timeout, the server should shut down and exit cleanly. Care must be taken not to close the sockets received from systemd.

Is that last thing (in bold) happening here?

@joshuay03
Copy link
Contributor Author

Is that last thing (in bold) happening here?

I haven't looked into this (so probably not). I'll add a test and go from there.

@joshuay03 joshuay03 force-pushed the shutdown-on-idle branch 3 times, most recently from df57bd0 to d4ee5a3 Compare August 12, 2023 13:09
@joshuay03 joshuay03 marked this pull request as draft August 13, 2023 04:18
@joshuay03
Copy link
Contributor Author

I'll add a test and go from there.

I'm hoping that this covers it: 272f2c6.

@joshuay03 joshuay03 marked this pull request as ready for review August 13, 2023 22:12
nateberkopec
nateberkopec reviewed Aug 15, 2023
View reviewed changes
lib/puma/server.rb Outdated Show resolved Hide resolved
@nateberkopec nateberkopec added the waiting-for-review Waiting on review from anyone label Aug 15, 2023
nateberkopec
nateberkopec requested changes Aug 24, 2023
View reviewed changes
lib/puma/server.rb Outdated Show resolved Hide resolved
@nateberkopec
Copy link
Member

I don't really have a better solution. I'm trying to think about how a timer thread or something like that could be used instead.

@joshuay03
Copy link
Contributor Author

joshuay03 commented Aug 25, 2023 •
edited

I'm trying to think about how a timer thread or something like that could be used instead.

Hmm that sounds interesting, I'll experiement and see how it goes. Thanks for the review!

@nateberkopec nateberkopec added waiting-for-changes Waiting on changes from the requestor and removed waiting-for-review Waiting on review from anyone labels Aug 31, 2023
@nateberkopec
Copy link
Member

I asked the hivemind

@nateberkopec
Copy link
Member

The ChatGPT answer I got wasn't bad either: update the time you last received data each time you get data, and have a background timer thread check every second if that time is X seconds ago or greater.

@joshuay03 @byroot
Rely on just the IO.select timeout
9c4a15a 
Co-authored-by: Jean Boussier <byroot@ruby-lang.org>
byroot
byroot reviewed Aug 31, 2023
View reviewed changes
lib/puma/server.rb Outdated Show resolved Hide resolved
nateberkopec
nateberkopec requested changes Sep 1, 2023
View reviewed changes
Copy link
Member

@nateberkopec nateberkopec left a comment

Choose a reason for hiding this comment

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

Thanks for catching the easiest possible solution @byroot!

lib/puma/server.rb Outdated Show resolved Hide resolved
@joshuay03 joshuay03 force-pushed the shutdown-on-idle branch 2 times, most recently from 191e7fa to cbe68bd Compare September 1, 2023 05:03
@nateberkopec
Copy link
Member

Thanks @joshuay03, this ended up being a great PR 👍

@nateberkopec nateberkopec merged commit 0b696a2 into puma:master Sep 4, 2023
59 checks passed
@davidalejandroaguilar davidalejandroaguilar mentioned this pull request Sep 12, 2023
Open
@davidalejandroaguilar davidalejandroaguilar mentioned this pull request Sep 21, 2023
Closed
@dentarg dentarg mentioned this pull request Oct 17, 2023
Closed
@netikular netikular mentioned this pull request Nov 6, 2023
Open
@bensheldon bensheldon mentioned this pull request Nov 26, 2023
Merged
@joshuay03 joshuay03 deleted the shutdown-on-idle branch November 27, 2023 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dentarg dentarg dentarg left review comments

@byroot byroot byroot left review comments

@jgaskins jgaskins jgaskins left review comments

@ghiculescu ghiculescu ghiculescu left review comments

@nateberkopec nateberkopec nateberkopec approved these changes

Assignees
No one assigned
Labels
feature waiting-for-changes Waiting on changes from the requestor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for shutdown on idle
6 participants
@joshuay03 @dentarg @nateberkopec @byroot @jgaskins @ghiculescu