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

Fixing issue where a server running in the background cannot be stopped #33

Closed
wants to merge 2 commits into from

Conversation

krmichelos
Copy link

When the chef-zero server is being run in the background calling stop doesn't stop the server. While testing my changes I also noticed that the running? method was written in such a way that it would always return true. This is because the running method on the puma server is the number of spawned processes being processed so even if there aren't any being processed !!0 is still true. I didn't see anything publicly available in the puma server that indicated what its status is so I thought connecting to the server to ensure it responds was probably the best way to handle it.

@sethvargo
Copy link
Contributor

@krmichelos could we just check if!server.running.zero?? That seems much simpler

@krmichelos
Copy link
Author

@sethvargo I did that at first but, if I read the puma code correctly, if the server isn't actively processing anything then that number will also be 0 which would make !server.running.zero? return false if the server is just idle.

@sethvargo
Copy link
Contributor

@krmichelos hrm. I really don't like this solution, but I also have nothing better to propose right now

@krmichelos
Copy link
Author

I agree. I really didn't care for it either. Without making a change in puma I couldn't think of anything better either.

@sethvargo
Copy link
Contributor

@jkeiser

@krmichelos
Copy link
Author

@sethvargo @jkeiser There is a problem with the running? method when the server is running on 0.0.0.0. I will look at it further and fix. I am also open to suggestions about a better way to determine if the server is running.

@jkeiser
Copy link
Contributor

jkeiser commented Sep 5, 2013

If the server is on 0.0.0.0, we can always hit 127.0.0.1. I'm sad there's apparently no way to ask Puma if it's actually doing anything :(

@jkeiser
Copy link
Contributor

jkeiser commented Sep 5, 2013

I wonder if there is a "listening" method or something ...

@sethvargo
Copy link
Contributor

I think we should open an issue on Puma and ask

This message was sent from my mobile device.

I apologize in advance for any typographical errors or autocorrections.

On Sep 5, 2013, at 12:54 AM, John Keiser notifications@github.com wrote:

I wonder if there is a "listening" method or something ...


Reply to this email directly or view it on GitHub.

@krmichelos
Copy link
Author

I opened an issue with puma. puma/puma#364

@krmichelos
Copy link
Author

Puma said they would add an API for the status of a server.

@krmichelos
Copy link
Author

@jkeiser I think the changes made in a45e8fb continue to leave the API broken as nothing is enforcing the stop wait limit. It would be easy to fix by wrapping the existing code in a Timeout block. I started looking at making running? use the changes made in puma. However, the changes are in the 2.* line and chef-zero is locked on the 1.* line. Unfortunately, I don't have the time right now to ensure that chef-zero can work with puma 2.6. Also it looks like there are additional steps to get puma 2.* to install on windows: puma/puma#202 because of openssl.

@jkeiser
Copy link
Contributor

jkeiser commented Sep 16, 2013

Oh heck. Yeah, we're using puma 1 specifically due to the Windows issues.

@sethvargo
Copy link
Contributor

@jkeiser is this still an issue with the switch to webrick?

@jkeiser
Copy link
Contributor

jkeiser commented Nov 3, 2013

We still support puma, is just optional now. But since we no longer have to run on every platform, I bet we could use puma 2 now.

@sethvargo
Copy link
Contributor

This is fixed in #43

@krmichelos krmichelos closed this Dec 17, 2013
@chef chef locked and limited conversation to collaborators Nov 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants