Replace Unicorn with Puma #501

calebhearth · 2015-01-26T20:05:09Z

Heroku now recommends using the Puma webserver. The previously
recommended webserver, Unicorn, is susceptible to slow client attacks.

https://devcenter.heroku.com/changelog-items/594

houndci-bot · 2015-01-26T20:05:12Z

lib/suspenders/app_builder.rb

-    def configure_unicorn
-      copy_file 'unicorn.rb', 'config/unicorn.rb'
+    def configure_puma
+      copy_file 'puma.rb', 'config/puma.rb'


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

JoelQ · 2015-01-26T21:00:20Z

👍

calebhearth · 2015-01-26T21:07:07Z

Okay before merging I'm going to try actually running an app with this server and these config.

calebhearth · 2015-01-26T21:07:27Z

And leave this at least a day.

croaky · 2015-01-26T23:40:42Z

templates/Procfile

@@ -1,2 +1,2 @@
-web: bundle exec unicorn -p $PORT -c ./config/unicorn.rb
+web: bundle exec rails serve Puma -p $PORT -c ./config/puma.rb


Why rails serve Puma rather than bundle exec puma per https://devcenter.heroku.com/articles/deploying-rails-applications-with-the-puma-web-server?

I pulled it from https://github.com/puma/puma#rails, and the move closer to the traditional Rails flow appeals to me. I'll check into actual differences when I try this in a fresh app.

Maybe some details here: http://stackoverflow.com/a/21218048 That answer
makes sense to me but I wonder why Heroku's docs are different, if there's
anything platform-specific that makes the Heroku docs version better on
Heroku. Maybe @schneems or @hone knows?

On Mon, Jan 26, 2015 at 3:42 PM, Caleb Thompson notifications@github.com
wrote:

In templates/Procfile
#501 (comment):

@@ -1,2 +1,2 @@
-web: bundle exec unicorn -p $PORT -c ./config/unicorn.rb
+web: bundle exec rails serve Puma -p $PORT -c ./config/puma.rb

I pulled it from https://github.com/puma/puma#rails.

—
Reply to this email directly or view it on GitHub
https://github.com/thoughtbot/suspenders/pull/501/files#r23575253.

I don't think there are differences really. There were some conflicts with some flags being parsed and passed to rails, but i guess those got sorted out 🤷 . The main reason we're doing it that way is so anyone running straight Rack can copy most of the article verbatim and still have it work. Same command on every project.

Btw, i think serve should be server. Also have you considered providing default config values? So you can run without a port specified i.e. -p ${PORT:-3000}

croaky · 2015-01-26T23:51:06Z

Definitely want to hear reports of people using this config in production before we merge this in.

Otherwise, makes sense and thanks for the PR. Good to squash and merge at your discretion.

calebhearth · 2015-01-27T01:14:02Z

templates/unicorn.rb

@@ -1,30 +0,0 @@
-# https://devcenter.heroku.com/articles/rails-unicorn
-
-worker_processes (ENV["UNICORN_WORKERS"] || 3).to_i


@croaky we lost something here too. Any idea what?

calebhearth · 2015-01-27T01:19:12Z

@croaky @schneems updated with most feedback addressed.

croaky · 2015-01-27T01:39:43Z

templates/postgresql_database.yml.erb

  timeout: 5000

 test:
  <<: *default
  database: <%= app_name %>_test
+
+production:


Do we want this for staging and production? Not sure if there's a better way, but maybe something like:

staging: &deploy <<: *default url: <%%= ENV.fetch("DATABASE_URL") %> production: <<: *deploy

I don't think so. We'd have to have that URL instead of the rest of this config.

This works because heroku provides this URL, Rails knows how to parse it, and we just override a single setting (or set it in the first place, in this case).

Actually, I wonder why we need to set url to DATABASE_URL at all. I thought Heroku already did that? Can we drop this line and only set pool?

It's a best practice. Rails does merge in DATABASE_URL into your config but explicit is better than implicit imho. As of 4.1 all this behavior is in Rails, Heroku isn't doing anything other than setting the env var

Looks like you might be right:

database.yml present
DATABASE_URL present
=> Merged into url sub key. If both specify url sub key, the database.yml url
sub key "wins". If other paramaters adapter or database are specified in YAML,
they are discarded as the url sub key "wins".

rails/rails#13463 (comment) linked from rails/rails#13578

Which would mean that we can't use defaults in production if it specifies database names, etc. but can set things like pool to be merged in.

These are the keys we already have which I think we can set safely, without overriding DATABASE_URL:

encoding: utf8 min_messages: warning pool: 2 timeout: 5000

Done in ba12e09

jferris · 2015-01-27T02:01:37Z

This is great. I've been thinking we should be switching to Puma for a while, and I've always been uncomfortable with the slow client problem.

I think Upcase is a good mid-traffic app to try this on. I can hopefully try it out this week, but if you're looking for a test project, feel free to open a pull request there.

calebhearth · 2015-01-27T02:05:34Z

Thanks, Joe.

pedrosmmoreira · 2015-01-27T10:24:02Z

templates/Procfile

@@ -1,2 +1,2 @@
-web: bundle exec unicorn -p $PORT -c ./config/unicorn.rb
+web: bundle exec rails server Puma -p ${PORT:-3000} -c ./config/puma.rb


This seems to break starting the app with Foreman. While running bundle exec rails s puma works, passing the config file will break with undefined methodworkers'`. This is a known issue (see puma/puma#512) and the example Procfile in the Heroku docs uses puma directly. Of course, the downside to that solution is losing logging...

Changing the Procfile to web: bundle exec puma -C ./config/puma.rb does boot the app loading the config.
but will also throw a thread warning related to the connection pool (discussion around similar issues can be found in puma/puma#614).

Why default to 3000? That seems like an additional (unnecessary) change. Is it something to do with Puma? Doesn't foreman handle $PORT for us?

Good catch. We should either re-use def port from lib/suspenders/app_builder.rb or rely on Foreman.

the example Procfile in the Heroku docs uses puma directly. Of course, the downside to that solution is losing logging...

@pedrosmmoreira Curious what you mean by "losing logging"? The Rails app itself will still log to whatever mechanism you've set the log to (STDOUT when running on Heroku). Are there other logs I'm not aware of or missing?

@stevenharman I might not have explained myself correctly. Using Puma directly runs the server unaware of the rails environment: this modifies logging behaviour, if I'm not mistaken.

The problem with -c ./config/puma.rb is that the rails server command is expecting a rackup file there, e.g. config.ru by default.

To do what you want, what you basically need to do is:

require ::File.expand_path('../config/environment', __FILE__) require "puma" require "puma/configuration" puma_config_file = ::File.expand_path('../config/puma.rb', __FILE__) Puma::Configuration.new(config_file: puma_config_file).load run Rails.application

which doesn't work, so we'd need to ask the puma peeps how to configure puma from a rackup file

Perhaps by interacting with Rails::Server#server (which subclasses Rack::Server)

I don't think the rackup file is supposed to know about the server:

# This file is used by Rack-based servers to start the application. require ::File.expand_path('../config/environment', __FILE__) run CodeTriage::Application

What's the problem with manually using the puma command in the web: line of the Procfile?

Works for me. Done.

`rake dev` spun up a server on 9292 using webrick. Webrick is slow and old. Let's upgrade to puma, per recommendation from the people at Heroku: https://devcenter.heroku.com/changelog-items/594 `puma.rb` pulled from thoughtbot's suspenders project here: thoughtbot/suspenders#501

jayroh · 2015-01-31T02:36:01Z

templates/postgresql_database.yml.erb

  timeout: 5000

 test:
  <<: *default
  database: <%= app_name %>_test
+
+production:
+  url:  <%%= ENV.fetch("DATABASE_URL") %>


When running this in my development env it chokes because I don't have DATABASE_URL set. Suffice it to say - on heroku this will be set, and locally it's not needed. A change to <%%= ENV.fetch("DATABASE_URL", "") %> fixed this for me.

Hi @jayroh. <3 <3

Hollaaaaaaaaaa @croaky!

Fixed in a78dc1e

#501 (comment)

gabebw · 2015-02-02T19:53:08Z

Since we now never specify the port in either puma.rb or the Procfile, will Puma still work with Foreman?

schneems · 2015-02-02T19:54:10Z

Foreman uses port 5000 by default. Also even if you start using rails s rails sees puma is in the gemfile and uses it by default, but won't use your config/puma.rb setup.

gabebw · 2015-02-02T19:56:36Z

So, this will work with Foreman even if I use a custom port like foreman start -p 7001?

`rake dev` spun up a server on 9292 using webrick. Webrick is slow and old. Let's upgrade to puma, per recommendation from the people at Heroku: https://devcenter.heroku.com/changelog-items/594 `puma.rb` pulled from thoughtbot's suspenders project here: thoughtbot/suspenders#501

seanpdoyle · 2015-05-21T15:09:47Z

We've run into issues using ember-cli-rails and a forking server (i.e. unicorn).

tricknotes/ember-cli-rails#94

My understanding is that puma can be configured to have a single, non-forking worker in development, which would smooth over the difficulties with ember-cli-rails.

I'm willing to try out puma on my next ember-cli-rails project (heroku or otherwise).

croaky · 2015-05-21T15:59:27Z

I've used Puma a few times in the last month or so, for example on a production app serving an API for an active iPhone app. This change in general seems well-vetted now in prod.

@seandoyle I think you can set WEB_CONCURRENCY and MAX_THREADS to 1 each in development? Maybe we add that to .sample.env?

https://devcenter.heroku.com/articles/deploying-rails-applications-with-the-puma-web-server#config

seanpdoyle · 2015-05-21T16:10:02Z

@drapergeek and I tried setting UNICORN_WORKERS=1 to no avail.

I'll try out puma with WEB_CONCURRENCY=1 and MAX_THREADS=1.

`rake dev` spun up a server on 9292 using webrick. Webrick is slow and old. Let's upgrade to puma, per recommendation from the people at Heroku: https://devcenter.heroku.com/changelog-items/594 `puma.rb` pulled from thoughtbot's suspenders project here: thoughtbot/suspenders#501

thoughtbot/suspenders#501

`rake dev` spun up a server on 9292 using webrick. Webrick is slow and old. Let's upgrade to puma, per recommendation from the people at Heroku: https://devcenter.heroku.com/changelog-items/594 `puma.rb` pulled from thoughtbot's suspenders project here: thoughtbot/suspenders#501

croaky · 2015-07-08T04:52:52Z

@seanpdoyle How has Puma been running for FormKeep? Any settings in this PR that you recommend as different defaults?

croaky · 2015-07-10T21:29:32Z

@calebthompson Possible to push this over the finish line? Seems like we're ready. @halogenandtoast had one piece of feedback above.

l4u · 2015-07-21T21:30:30Z

👍

gylaz · 2015-07-24T22:05:56Z

templates/postgresql_database.yml.erb

+  encoding: utf8
+  min_messages: warning
+  pool: <%%= [ENV.fetch("MAX_THREADS", 2), ENV.fetch("DB_POOL", 2)].max %>
+  timeout: 5000


Is this a reasonable timeout for production?

I can't think of a reason it wouldn't be. Can you?

croaky · 2015-07-28T07:04:45Z

@calebthompson I think you're available tomorrow? Possible to do a final review, merge this, and release a new version?

Seen a couple of brand-new apps recently getting slow client errors with Unicorn, Heroku, two dynos, almost no traffic.

Might be something worth grabbing from https://github.com/thoughtbot/hound/pull/846/files

calebhearth · 2015-07-28T14:23:19Z

Ready for final review.

calebhearth · 2015-07-28T21:31:00Z

I'm content with this. If it's a 👍 please go ahead and merge this.

croaky · 2015-07-28T22:00:21Z

Looks good to me.

@schneems

> Heroku now recommends using the Puma webserver. The previously > recommended webserver, Unicorn, is susceptible to slow client attacks. https://devcenter.heroku.com/changelog-items/594 * Specify deploy settings - Set values that DATABASE_URL won't override / set - Add staging - Use max of thread count or db pool, since @schneems says: > You need your pool to at least be as large as your puma thread > count.

mcmire · 2015-07-29T20:47:54Z

🎉

houndci-bot reviewed Jan 26, 2015
View reviewed changes

croaky reviewed Jan 26, 2015
View reviewed changes

calebhearth reviewed Jan 27, 2015
View reviewed changes

calebhearth force-pushed the ct-unicorn branch from dd7dcdc to 211517e Compare January 27, 2015 01:22

croaky reviewed Jan 27, 2015
View reviewed changes

pedrosmmoreira reviewed Jan 27, 2015
View reviewed changes

jayroh reviewed Jan 31, 2015
View reviewed changes

calebhearth added a commit that referenced this pull request Feb 2, 2015

$DATABASE_URL isn't in dev, probably.

a78dc1e

#501 (comment)

tute added the waiting-on-experiment label May 6, 2015

digitalcora added a commit to Vermonster/schoolbot that referenced this pull request Jun 22, 2015

Use Puma instead of Unicorn

09bfed7

thoughtbot/suspenders#501

gylaz mentioned this pull request Jul 7, 2015

Puma? houndci/hound#845

Closed

gylaz reviewed Jul 24, 2015
View reviewed changes

calebhearth force-pushed the ct-unicorn branch from 2ac07d2 to e941cec Compare July 28, 2015 14:15

calebhearth force-pushed the ct-unicorn branch from 993c88d to 166474c Compare July 28, 2015 14:23

calebhearth force-pushed the ct-unicorn branch from 166474c to c681ed3 Compare July 28, 2015 23:39

calebhearth merged commit c681ed3 into master Jul 28, 2015

calebhearth deleted the ct-unicorn branch July 28, 2015 23:40

tute mentioned this pull request Aug 24, 2015

web-console && workers #570

Closed

		@@ -1,2 +1,2 @@
		web: bundle exec unicorn -p $PORT -c ./config/unicorn.rb
		web: bundle exec rails serve Puma -p $PORT -c ./config/puma.rb

		@@ -1,30 +0,0 @@
		# https://devcenter.heroku.com/articles/rails-unicorn

		worker_processes (ENV["UNICORN_WORKERS"] \|\| 3).to_i

		@@ -1,2 +1,2 @@
		web: bundle exec unicorn -p $PORT -c ./config/unicorn.rb
		web: bundle exec rails server Puma -p ${PORT:-3000} -c ./config/puma.rb

Replace Unicorn with Puma #501

Replace Unicorn with Puma #501

Conversation

calebhearth commented Jan 26, 2015

Choose a reason for hiding this comment

JoelQ commented Jan 26, 2015

calebhearth commented Jan 26, 2015

calebhearth commented Jan 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

croaky commented Jan 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebhearth commented Jan 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jferris commented Jan 27, 2015

calebhearth commented Jan 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabebw commented Feb 2, 2015

schneems commented Feb 2, 2015

gabebw commented Feb 2, 2015

seanpdoyle commented May 21, 2015

croaky commented May 21, 2015

seanpdoyle commented May 21, 2015

croaky commented Jul 8, 2015

croaky commented Jul 10, 2015

l4u commented Jul 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

croaky commented Jul 28, 2015

calebhearth commented Jul 28, 2015

calebhearth commented Jul 28, 2015

croaky commented Jul 28, 2015

mcmire commented Jul 29, 2015