Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Use Rack::Cache middleware only if is in Gemfile #7794

Merged
merged 2 commits into from Oct 2, 2012

Conversation

Projects
None yet
6 participants
Owner

guilleiguaran commented Sep 30, 2012

This being discussed by Rails core right now, IMO this PR is a fair solution, if the user want to use rack-cache then it should be specified in Gemfile explicitly

Owner

guilleiguaran commented Sep 30, 2012

should we add rack-cache to default generated Gemfile? (commented or uncommented?)

@guilleiguaran guilleiguaran and 1 other commented on an outdated diff Sep 30, 2012

...ties/lib/rails/generators/rails/app/templates/Gemfile
@@ -9,6 +9,10 @@ source 'https://rubygems.org'
<%= assets_gemfile_entry %>
<%= javascript_gemfile_entry %>
+# Rack::Cache enables HTTP caching on your app
+# Varnish/Squid are recommended for big apps
@guilleiguaran

guilleiguaran Sep 30, 2012

Owner

Let me know if you have ideas about a better message

@jeremy

jeremy Sep 30, 2012

Owner

To put a simple HTTP cache in front of your app, add rack-cache to your bundle and set config.action_dispatch.rack_cache = true.
For large-scale production use, consider using nginx, varnish, or squid as caching reverse proxies.

@guilleiguaran

guilleiguaran Oct 1, 2012

Owner

@jeremy should we add config.action_dispatch.rack_cache = true to default production.rb (commented/uncommented?) or leave it as hidden option?

@jeremy

jeremy Oct 1, 2012

Owner

Probably leave it out. If we extract a rack-cache-rails plugin, can talk about config in its docs.

@jeremy jeremy and 2 others commented on an outdated diff Sep 30, 2012

railties/lib/rails/application.rb
@@ -281,9 +281,13 @@ def reload_dependencies? #:nodoc:
def default_middleware_stack #:nodoc:
ActionDispatch::MiddlewareStack.new.tap do |middleware|
app = self
- if rack_cache = config.action_controller.perform_caching && config.action_dispatch.rack_cache
- require "action_dispatch/http/rack_cache"
- middleware.use ::Rack::Cache, rack_cache
+ begin
+ require "rack-cache"
+ if rack_cache = config.action_controller.perform_caching && config.action_dispatch.rack_cache
+ require "action_dispatch/http/rack_cache"
+ middleware.use ::Rack::Cache, rack_cache
+ end
+ rescue LoadError
@jeremy

jeremy Sep 30, 2012

Owner

This silently fails when you set config.action_dispatch.rack_cache = true but didn't include rack-cache in your Gemfile. It should raise a LoadError. Maybe something like:

if rack_cache = config.action_controller.perform_caching && config.action_dispatch.rack_cache
  begin
    require 'rack/cache'
  rescue LoadError => error
    error.message << ' Be sure to add rack-cache to your Gemfile'
    raise
  end
  require "action_dispatch/http/rack_cache"
  middleware.use ::Rack::Cache, rack_cache
end
@spastorino

spastorino Sep 30, 2012

Owner

What we should be doing is ...

if config.action_controller.perform_caching
  begin
    require 'rack/cache'
  rescue LoadError => error
    error.message << ' Be sure to add rack-cache to your Gemfile'
    raise
  end
  require "action_dispatch/http/rack_cache"
  middleware.use ::Rack::Cache, { :metastore => "rails:/",
                                  :entitystore => "rails:/",
                                  :verbose => false }
end

And removing this https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/railtie.rb#L16-21

@jeremy

jeremy Sep 30, 2012

Owner

Then you can't set up rack-cache yourself. Adding the middleware just because it's in your Gemfile is surprising.

@guilleiguaran

guilleiguaran Sep 30, 2012

Owner

Agree with @jeremy, we should keep config.action_dispatch.rack_cache to allow customisation of Rack Cache setup

@spastorino

spastorino Sep 30, 2012

Owner

ok, agree :P

@jeremy jeremy commented on an outdated diff Sep 30, 2012

...ties/lib/rails/generators/rails/app/templates/Gemfile
@@ -9,6 +9,10 @@ source 'https://rubygems.org'
<%= assets_gemfile_entry %>
<%= javascript_gemfile_entry %>
+# Rack::Cache enables HTTP caching on your app
+# Varnish/Squid are recommended for big apps
+gem 'rack-cache', '~> 1.2'
@jeremy

jeremy Sep 30, 2012

Owner

Should be commented out?

Owner

spastorino commented Sep 30, 2012

I'm OK with the change, I prefer moving all the rack-cache related code to rack-cache itself or to rack-cache-rails though. We could merge this first and then start working on that. What do you think @guilleiguaran ?.

@rtomayko is it possible to add a railtie and all the glue code we need to rack-cache itself? or should we start thinking about a rack-cache-rails gem?, which has all the glue code + rack-cache as a dependency ala sass-rails

Contributor

rtomayko commented Sep 30, 2012

@rtomayko is it possible to add a railtie and all the glue code we need to rack-cache itself? or should we start thinking about a rack-cache-rails gem?, which has all the glue code + rack-cache as a dependency ala sass-rails

Seems reasonable to me. If it's all new code I can get a release out quickly.

Contributor

barttenbrinke commented Oct 1, 2012

I never really understood why rails dependent on it by default. After issues like rtomayko/rack-cache#52 I have disabled rack-cache disabled for my biggest projects by default anyway.
This would be a much cleaner solution then the current initializer hack i'm using.

Owner

rafaelfranca commented Oct 2, 2012

Seems fine to me 👍

spastorino added a commit that referenced this pull request Oct 2, 2012

Merge pull request #7794 from guilleiguaran/extract-rack-cache
Use Rack::Cache middleware only if is in Gemfile

@spastorino spastorino merged commit 037e50e into rails:master Oct 2, 2012

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