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

Remove puma and fork a subprocess #43

Merged
merged 1 commit into from
Dec 17, 2013
Merged

Remove puma and fork a subprocess #43

merged 1 commit into from
Dec 17, 2013

Conversation

sethvargo
Copy link
Contributor

This commit removes all instances of the Puma webserver, since it has known issues on a number of supported platforms and adds significant branching logic to the code.

This commit also re-defines what it means when the server is "running". In the past, "running" has meant the web server is up. Testing has revealed that web servers actually lie and say they are running, even if they are not accepting requests. The new running? method uses OpenURI to access a known URL on the server.

WEBrick does not support running on a socket, so the --socket option has also been removed.

@sethvargo
Copy link
Contributor Author

@jkeiser I don't know why Pedant is failing - it passes locally.

fork doesn't work on JRuby, any proposed ideas?

@stevendanna
Copy link
Contributor

Loss of the --socket option makes me sad, but understand the tradeoff. Won't the use of fork() cause problems on windows as well?

@sethvargo
Copy link
Contributor Author

@stevendanna yea, but I think the socket option encouraged people to use Chef Zero as a "real Chef Server", which is concerning to me. I also think puma caused more problems than it was worth (and I added it in the first place). It was a dumb decision on my part - I think webrick is the best solution, especially since this is now embedded in Chef.

And sigh. Fork may have been a bad decision as well (although the current use of threads also has some problems WRT to starting and stopping the server process). I wonder if we could use Process.spawn and drop 1.8 support? Or try Process.spawn and then fallback to fork (since JRuby has had processing spawning for awhile). What do you think?

uri = URI.join(url, 'cookbooks')
headers = { 'Accept' => 'application/json' }

timeout(0.1) { !open(uri, headers).nil? }
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you preface this with if server.status != :Running false, so that we only spend the 0.1 seconds when it's lying to us? This method should be cheap most of the time.

This commit removes all instances of the Puma webserver, since it has known
issues on a number of supported platforms and adds significant branching
logic to the code.

This commit also re-defines what it means when the server is "running". In
the past, "running" has meant the web server is up. Testing has revealed that
web servers actually lie and say they are running, even if they are not
accepting requests. The new `running?` method uses OpenURI to access a known
URL on the server.

WEBrick does not support running on a socket, so the `--socket` option has
also been removed.
@jkeiser
Copy link
Contributor

jkeiser commented Dec 17, 2013

👍 assuming this passes puma locally!

@jkeiser
Copy link
Contributor

jkeiser commented Dec 17, 2013

er. passes tests locally. Calling Freud.

@sethvargo sethvargo merged commit 0aaabe6 into master Dec 17, 2013
@sethvargo sethvargo deleted the sethvargo/fork_exec branch December 17, 2013 22:20
@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