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

Add Default Puma Config #23057

Merged
merged 1 commit into from Feb 1, 2016

Conversation

Projects
None yet
@schneems
Member

schneems commented Jan 14, 2016

When the puma command is run without any configuration options it will detect presence of a config/puma.rb file and use that. Currently there is discrepancy between puma command and rails server but Evan said it would be reasonable to add in reading in config from the default location. I am working on that right now as a feature in puma/puma.

Why do we need this? By default Puma uses 16 threads, and by default ActiveRecord only has 5 threads. Due to the architecture of AR it is guaranteed that if you're running with fewer DB connections than your server has threads you will hit ActiveRecord::ConnectionTimeoutError eventually if your app gets modest amounts of traffic. Since we are providing a default webserver, we should provide reasonable configuration for that webserver.

This PR does a few things, first it sets the default Puma thread count to 5 to mach ActiveRecord's default. It sets the default environment to "development" and the default port to 300 so that booting the server with $ puma will give you the same default port as rails server. It is worth mentioning that by reading in from PORT environment variable this config can work with containerized deployments, such as on Heroku.

We are not using worker processes by default, that way JRuby and windows devs can use this configuration without modification. I went ahead and included a default on_worker_boot. It won't be used unless a worker count is specified, that means this config will not use it. Even though it's not being used now It will make someone who wants to try modifying their config to run extra workers easier.

cc/ @pixeltrix

@nateberkopec

This comment has been minimized.

Show comment
Hide comment
@nateberkopec

nateberkopec Jan 14, 2016

Contributor

👍 newbs getting ActiveRecord::ConnectionTimeoutError would be awful.

Contributor

nateberkopec commented Jan 14, 2016

👍 newbs getting ActiveRecord::ConnectionTimeoutError would be awful.

Show outdated Hide outdated railties/lib/rails/generators/rails/app/templates/config/puma.rb
port ENV.fetch("PORT") { 3000 }
environment ENV.fetch("RAILS_ENV") { "development" }
on_worker_boot do

This comment has been minimized.

@vipulnsward

vipulnsward Jan 14, 2016

Member

@schneems we should annotate what is going around here, like other config files.

@vipulnsward

vipulnsward Jan 14, 2016

Member

@schneems we should annotate what is going around here, like other config files.

This comment has been minimized.

@schneems

schneems Jan 14, 2016

Member

👍 I can add comments for all the config options

@schneems

schneems Jan 14, 2016

Member

👍 I can add comments for all the config options

This comment has been minimized.

@nateberkopec

nateberkopec Jan 14, 2016

Contributor

Also, since ActionCable is a runtime dependency (at least for now), should we be adding anything here for Redis?

@nateberkopec

nateberkopec Jan 14, 2016

Contributor

Also, since ActionCable is a runtime dependency (at least for now), should we be adding anything here for Redis?

This comment has been minimized.

@schneems

schneems Jan 14, 2016

Member

Seems good to be on the safe side. Is there an equivalent for an ActionCable disconnect?

@schneems

schneems Jan 14, 2016

Member

Seems good to be on the safe side. Is there an equivalent for an ActionCable disconnect?

This comment has been minimized.

@nateberkopec

nateberkopec Jan 14, 2016

Contributor

shrug depends on @maclover7's adapterization I think.

@nateberkopec

nateberkopec Jan 14, 2016

Contributor

shrug depends on @maclover7's adapterization I think.

This comment has been minimized.

@maclover7

maclover7 Jan 14, 2016

Member

Connection/disconnection to Postgres/Redis/whatever adapter will automatically be handled on server boot. Do we want to add a specific disconnect method? Wouldn't be that hard to add. cc @matthewd

@maclover7

maclover7 Jan 14, 2016

Member

Connection/disconnection to Postgres/Redis/whatever adapter will automatically be handled on server boot. Do we want to add a specific disconnect method? Wouldn't be that hard to add. cc @matthewd

This comment has been minimized.

@matthewd

matthewd Jan 14, 2016

Member

I'm not quite sure where it should live / look like, but I think we should probably provide a method somewhere that does this... allowing components to separately register such behaviour.

@matthewd

matthewd Jan 14, 2016

Member

I'm not quite sure where it should live / look like, but I think we should probably provide a method somewhere that does this... allowing components to separately register such behaviour.

This comment has been minimized.

@maclover7

maclover7 Jan 14, 2016

Member

👍 adding to my checklist

@maclover7

maclover7 Jan 14, 2016

Member

👍 adding to my checklist

Show outdated Hide outdated railties/lib/rails/generators/rails/app/templates/config/puma.rb
@@ -0,0 +1,9 @@
threads_count = ENV.fetch("MAX_THREADS") { 5 }.to_i

This comment has been minimized.

@matthewd

matthewd Jan 14, 2016

Member

I'm not sure about this ENV-all-the-things.. that's really not how we configure Rails.

@matthewd

matthewd Jan 14, 2016

Member

I'm not sure about this ENV-all-the-things.. that's really not how we configure Rails.

This comment has been minimized.

@schneems

schneems Jan 14, 2016

Member

that's really not how we configure Rails.

We do all of our rails configuration behind environment variables:

$ RAILS_ENV=production rails server

For the puma config, we want something that can be over-rideable. We need to have rails server work out of the box with some sane defaults but be extensible. I don't know if there's another way to accomplish this. I don't think we should tie the interface of rails server flags to the implementation of Puma's server, $ rails server --minimum-puma-threads=5 --maximum-puma-threads=5. We could put this in a YML somewhere and have values for different environments but honestly I would set all 3 to the same values. Then that adds yet another place to look for configuration. We can't use Rail's config methods already in place because this code will get booted before your Rails code will.

In my perfect world we would figure out a way to better integrate AR's pool setting with a server's thread count setting. I.e. so if you increase your threads on your server then AR knows and uses a higher pool value. Right now you have to know about EVERYWHERE that uses a DB connection. So instead of 1 change you have to modify sidekiq config + puma config + AR pool size.

@schneems

schneems Jan 14, 2016

Member

that's really not how we configure Rails.

We do all of our rails configuration behind environment variables:

$ RAILS_ENV=production rails server

For the puma config, we want something that can be over-rideable. We need to have rails server work out of the box with some sane defaults but be extensible. I don't know if there's another way to accomplish this. I don't think we should tie the interface of rails server flags to the implementation of Puma's server, $ rails server --minimum-puma-threads=5 --maximum-puma-threads=5. We could put this in a YML somewhere and have values for different environments but honestly I would set all 3 to the same values. Then that adds yet another place to look for configuration. We can't use Rail's config methods already in place because this code will get booted before your Rails code will.

In my perfect world we would figure out a way to better integrate AR's pool setting with a server's thread count setting. I.e. so if you increase your threads on your server then AR knows and uses a higher pool value. Right now you have to know about EVERYWHERE that uses a DB connection. So instead of 1 change you have to modify sidekiq config + puma config + AR pool size.

This comment has been minimized.

@nateberkopec

nateberkopec Jan 14, 2016

Contributor

It's either that or adding config flags:

  config.web_server.minimum_threads 4
  config.web_server.maximum_threads 4

Seems yucky to me.

@nateberkopec

nateberkopec Jan 14, 2016

Contributor

It's either that or adding config flags:

  config.web_server.minimum_threads 4
  config.web_server.maximum_threads 4

Seems yucky to me.

This comment has been minimized.

@dhh

dhh Jan 15, 2016

Member

@schneems If you're not likely to change these values per-environment, why do you need to be able to overwrite them ad-hoc via ENV?

@dhh

dhh Jan 15, 2016

Member

@schneems If you're not likely to change these values per-environment, why do you need to be able to overwrite them ad-hoc via ENV?

This comment has been minimized.

@schneems

schneems Jan 21, 2016

Member

I like to be able to change my server behavior without having to wait for an entire code deploy. I see this line as a benefit for those who can configure env separately from code and nothing lost for those who only change configurations via code/deploys.

Not now, but eventually if we could standardize on a environment variable we could use it to set all locations that have a thread pool, so changing in one place would work in all locations. For example imagine if we had this code and also had a database.yml

production:
  pool: <%= ENV.fetch("MAX_THREADS") { 5 } %>

Now when you increase your puma threads, your AR pool is increased automatically and it does the right thing out of the box. Right now you have to know EVERY place that uses a thread pool when you change your puma config. Sidekiq, AR, sucker_punch, AMQP or other stores, threaded API clients. If you change one but not all of them, then you get errors like ActiveRecord::ConnectionTimeoutError. Even if you're not configuring using env vars, it gives a standard thing to grep for to change all those values. Having all of those components work together by a convention is the direction I would eventually like to see default Rails config, but again that's an "eventually" goal, not within the scope of this PR.

@schneems

schneems Jan 21, 2016

Member

I like to be able to change my server behavior without having to wait for an entire code deploy. I see this line as a benefit for those who can configure env separately from code and nothing lost for those who only change configurations via code/deploys.

Not now, but eventually if we could standardize on a environment variable we could use it to set all locations that have a thread pool, so changing in one place would work in all locations. For example imagine if we had this code and also had a database.yml

production:
  pool: <%= ENV.fetch("MAX_THREADS") { 5 } %>

Now when you increase your puma threads, your AR pool is increased automatically and it does the right thing out of the box. Right now you have to know EVERY place that uses a thread pool when you change your puma config. Sidekiq, AR, sucker_punch, AMQP or other stores, threaded API clients. If you change one but not all of them, then you get errors like ActiveRecord::ConnectionTimeoutError. Even if you're not configuring using env vars, it gives a standard thing to grep for to change all those values. Having all of those components work together by a convention is the direction I would eventually like to see default Rails config, but again that's an "eventually" goal, not within the scope of this PR.

This comment has been minimized.

@nateberkopec

nateberkopec Jan 22, 2016

Contributor

If you're not likely to change these values per-environment, why do you need to be able to overwrite them ad-hoc via ENV?

A common workflow for me, especially regarding MAX_THREADS, is to tune this variable up or down 1 or 2 at a time to find the most comfortable number for the server's memory. I like that it's as easy as "change the environment variable, restart the server".

I do the same with Puma's worker count when running in cluster mode. The memory usage of a Rails process can change after a deploy because of some code change (perhaps you added a heavy gem), and it's nice to be able to react to that with a quick "env change/restart" rather than pushing code changes through each time this variable needs to be tuned (which is fairly often on memory-constrained environments like Heroku).

@nateberkopec

nateberkopec Jan 22, 2016

Contributor

If you're not likely to change these values per-environment, why do you need to be able to overwrite them ad-hoc via ENV?

A common workflow for me, especially regarding MAX_THREADS, is to tune this variable up or down 1 or 2 at a time to find the most comfortable number for the server's memory. I like that it's as easy as "change the environment variable, restart the server".

I do the same with Puma's worker count when running in cluster mode. The memory usage of a Rails process can change after a deploy because of some code change (perhaps you added a heavy gem), and it's nice to be able to react to that with a quick "env change/restart" rather than pushing code changes through each time this variable needs to be tuned (which is fairly often on memory-constrained environments like Heroku).

This comment has been minimized.

@dhh

dhh Jan 24, 2016

Member

At the very least, this ENV needs to be prefixed. So either it's PUMA_MAX_THREADS or it's RAILS_MAX_THREADS.

@dhh

dhh Jan 24, 2016

Member

At the very least, this ENV needs to be prefixed. So either it's PUMA_MAX_THREADS or it's RAILS_MAX_THREADS.

This comment has been minimized.

@schneems

schneems Jan 25, 2016

Member

Updated to use a prefix RAILS_MAX_THREADS. I think that's the last piece of actionable feedback since --skip-puma is fine as a flag.

@schneems

schneems Jan 25, 2016

Member

Updated to use a prefix RAILS_MAX_THREADS. I think that's the last piece of actionable feedback since --skip-puma is fine as a flag.

Show outdated Hide outdated railties/lib/rails/generators/rails/app/templates/config/puma.rb
@@ -0,0 +1,9 @@
threads_count = ENV.fetch("MAX_THREADS") { 5 }.to_i
threads threads_count, threads_count

This comment has been minimized.

@matthewd

matthewd Jan 14, 2016

Member

I assume this is setting the minimum and maximum respectively? Why do we want a minimum?

@matthewd

matthewd Jan 14, 2016

Member

I assume this is setting the minimum and maximum respectively? Why do we want a minimum?

This comment has been minimized.

@schneems

schneems Jan 14, 2016

Member

You have to set a minimum. Setting minimum == maximum means that 5 threads get spawned at start time instead of waiting to spawn threads at request time. The alternative would be to set to 1.

@schneems

schneems Jan 14, 2016

Member

You have to set a minimum. Setting minimum == maximum means that 5 threads get spawned at start time instead of waiting to spawn threads at request time. The alternative would be to set to 1.

This comment has been minimized.

@evanphx

evanphx Jan 14, 2016

Contributor

You can set it to 0 so that they spin down to nothing and only spin up when used.

@evanphx

evanphx Jan 14, 2016

Contributor

You can set it to 0 so that they spin down to nothing and only spin up when used.

This comment has been minimized.

@evanphx

evanphx Jan 14, 2016

Contributor

It being the minimum.

@evanphx

evanphx Jan 14, 2016

Contributor

It being the minimum.

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Jan 14, 2016

Member

@evanphx opinions plz

Member

matthewd commented Jan 14, 2016

@evanphx opinions plz

@evanphx

This comment has been minimized.

Show comment
Hide comment
@evanphx

evanphx Jan 14, 2016

Contributor

👍 Sounds good to me. I want to make puma and rails work together as well as humanly possible.

Contributor

evanphx commented Jan 14, 2016

👍 Sounds good to me. I want to make puma and rails work together as well as humanly possible.

@maclover7 maclover7 added the railties label Jan 14, 2016

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jan 15, 2016

Member

It is fine to me but I have my doubts about advertising a server by default.

We are including puma by default in the Gemfile because the current implementation of Action Cable requires it. It may not require in the future. Also, many applications will not use Action Cable. In my opinion puma should only be added to Gemfile if --skip-action-cable is not given. Same thing with this config file.

cc @dhh

Member

rafaelfranca commented Jan 15, 2016

It is fine to me but I have my doubts about advertising a server by default.

We are including puma by default in the Gemfile because the current implementation of Action Cable requires it. It may not require in the future. Also, many applications will not use Action Cable. In my opinion puma should only be added to Gemfile if --skip-action-cable is not given. Same thing with this config file.

cc @dhh

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jan 15, 2016

Member

I think it's fine that now we have puma as the default, we also have a default config. BUT, we should provide consideration for extending/skipping that. So we can either do something like --skip-puma or have --web-server=thin|puma|whatever.

But I think we're better off to standardize around 1 default webserver for Rails, regardless of whether you're using Action Cable or not.

Member

dhh commented Jan 15, 2016

I think it's fine that now we have puma as the default, we also have a default config. BUT, we should provide consideration for extending/skipping that. So we can either do something like --skip-puma or have --web-server=thin|puma|whatever.

But I think we're better off to standardize around 1 default webserver for Rails, regardless of whether you're using Action Cable or not.

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Jan 21, 2016

Member

I added a puma skip option and comments describing the different puma options. Please review.

Member

schneems commented Jan 21, 2016

I added a puma skip option and comments describing the different puma options. Please review.

def webserver_gemfile_entry
return [] if options[:skip_puma]
comment = 'Use Puma as the app server'
GemfileEntry.new('puma', nil, comment)

This comment has been minimized.

@maclover7

maclover7 Jan 21, 2016

Member

Do we want to set a default version for Puma?

@maclover7

maclover7 Jan 21, 2016

Member

Do we want to set a default version for Puma?

This comment has been minimized.

@schneems

schneems Jan 21, 2016

Member

We will when my changes to puma puma/puma#856 get merged. For now i don't think we're relying on any specific version feature. The previous Gemfile declaration did not list a version.

@schneems

schneems Jan 21, 2016

Member

We will when my changes to puma puma/puma#856 get merged. For now i don't think we're relying on any specific version feature. The previous Gemfile declaration did not list a version.

This comment has been minimized.

@maclover7

maclover7 Jan 21, 2016

Member

👍 just checking :)

@maclover7

maclover7 Jan 21, 2016

Member

👍 just checking :)

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Jan 22, 2016

Member

Everyone fine with --skip-puma as a flag? Do those comments look good? Anywhere else needs a test? Ready for review.

Member

schneems commented Jan 22, 2016

Everyone fine with --skip-puma as a flag? Do those comments look good? Anywhere else needs a test? Ready for review.

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Jan 22, 2016

Member

@schneems why not an --appserver option that defaults to puma ?

Member

pixeltrix commented Jan 22, 2016

@schneems why not an --appserver option that defaults to puma ?

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Jan 22, 2016

Member

@pixeltrix seems good

Member

schneems commented Jan 22, 2016

@pixeltrix seems good

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Jan 22, 2016

Member

@pixeltrix on second though that could open us up to a can of worms. I.e. now we have to maintain a config and gemfile options for basically ALL webservers. Either that or we make the though call of telling some webserver authors why or why not they can't add support for their project directly into rails. Right now i'm leaning towards a --no-webserver option that would skip whatever default server and config we want to add now or in the future. If we want to later come back and allow --appserver= we could always do it. Similar to how you can generate a rails app with either no active record, or with a specific server configured. How does that sound?

Member

schneems commented Jan 22, 2016

@pixeltrix on second though that could open us up to a can of worms. I.e. now we have to maintain a config and gemfile options for basically ALL webservers. Either that or we make the though call of telling some webserver authors why or why not they can't add support for their project directly into rails. Right now i'm leaning towards a --no-webserver option that would skip whatever default server and config we want to add now or in the future. If we want to later come back and allow --appserver= we could always do it. Similar to how you can generate a rails app with either no active record, or with a specific server configured. How does that sound?

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jan 24, 2016

Member

I think it's fine to go with --skip-puma. We're making a statement that Puma is the default server for Rails. This doesn't mean it's the only server that'll work, but it is the default. So in that sense, it's similar to stuff like --skip-turbolinks. It's not --skip-page-accelerator=pjax or whatever.

Member

dhh commented Jan 24, 2016

I think it's fine to go with --skip-puma. We're making a statement that Puma is the default server for Rails. This doesn't mean it's the only server that'll work, but it is the default. So in that sense, it's similar to stuff like --skip-turbolinks. It's not --skip-page-accelerator=pjax or whatever.

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Jan 28, 2016

Member

No new feedback. Looks like this is good to merge

Member

schneems commented Jan 28, 2016

No new feedback. Looks like this is good to merge

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
Member

rafaelfranca commented Jan 29, 2016

:shipit:

Show outdated Hide outdated railties/lib/rails/generators/rails/app/templates/config/puma.rb
# Workers do not work on JRuby or Windows (both of which do not support
# processes).
#
# workers ENV.fetch('WEB_CONCURRENCY') { 2 }

This comment has been minimized.

@JuanitoFatas

JuanitoFatas Jan 29, 2016

Contributor

Do you want to use double quote consistent with above?

@JuanitoFatas

JuanitoFatas Jan 29, 2016

Contributor

Do you want to use double quote consistent with above?

This comment has been minimized.

@schneems

schneems Jan 29, 2016

Member

👍 thanks, updated

@schneems

schneems Jan 29, 2016

Member

👍 thanks, updated

Add Default Puma Config
When the `puma` command is run without any configuration options it will detect presence of a `config/puma.rb` file and use that. Currently there is discrepancy between `puma` command and `rails server` but Evan said it would be reasonable to add in reading in config from the default location. I am working on that right now as a feature in puma/puma.

Why do we need this? By default Puma uses 16 threads, and by default ActiveRecord only has 5 threads. Due to the architecture of AR it is guaranteed that if you're running with fewer DB connections than your server has threads you will hit `ActiveRecord::ConnectionTimeoutError ` eventually if your app gets modest amounts of traffic. Since we are providing a default webserver, we should provide reasonable configuration for that webserver.

This PR does a few things, first it sets the default Puma thread count to 5 to mach ActiveRecord's default. It sets the default environment to `"development"` and the default port to 300 so that booting the server with `$ puma` will give you the same default port as `rails server`. It is worth mentioning that by reading in from `PORT` environment variable this config can work with containerized deployments, such as on Heroku. 

We are not using worker processes by default, that way JRuby and windows devs can use this configuration without modification. I went ahead and included a default `on_worker_boot`. It won't be used unless a worker count is specified, that means this config will not use it. Even though it's not being used now It will make someone who wants to try modifying their config to run extra workers easier.

cc/ @pixeltrix

schneems added a commit that referenced this pull request Feb 1, 2016

@schneems schneems merged commit 25a4275 into rails:master Feb 1, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@naltun

This comment has been minimized.

Show comment
Hide comment

naltun commented Feb 6, 2016

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment