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

Rack::Runtime can only be disabled in an initializer #16433

Closed
csuhta opened this Issue Aug 8, 2014 · 11 comments

Comments

Projects
None yet
5 participants
@csuhta

csuhta commented Aug 8, 2014

This is an extension of #1043

Normally you load and unload Rack middlware inside config/application.rb or an environment config.
However, Rack::Runtime can't be deleted inside config/application.rb because an internal part of railties assumes that it's there.

Here's the an example stacktrace you get if you try to config.middleware.delete Rack::Runtime inside config/application.rb:

/Users/Csuhta/Projects/project-name/vendor/bundle/ruby/2.1.0/gems/actionpack-4.1.4/lib/action_dispatch/middleware/stack.rb:125:in `assert_index': No such middleware to insert before: "Rack::Runtime" (RuntimeError)
    from /Users/Csuhta/Projects/project-name/vendor/bundle/ruby/2.1.0/gems/actionpack-4.1.4/lib/action_dispatch/middleware/stack.rb:88:in `insert'
    from /Users/Csuhta/Projects/project-name/vendor/bundle/ruby/2.1.0/gems/railties-4.1.4/lib/rails/configuration.rb:68:in `block in merge_into'
    from /Users/Csuhta/Projects/project-name/vendor/bundle/ruby/2.1.0/gems/railties-4.1.4/lib/rails/configuration.rb:67:in `each'
    from /Users/Csuhta/Projects/project-name/vendor/bundle/ruby/2.1.0/gems/railties-4.1.4/lib/rails/configuration.rb:67:in `merge_into'
    from /Users/Csuhta/Projects/project-name/vendor/bundle/ruby/2.1.0/gems/railties-4.1.4/lib/rails/engine.rb:497:in `app'
    from /Users/Csuhta/Projects/project-name/vendor/bundle/ruby/2.1.0/gems/railties-4.1.4/lib/rails/application/finisher.rb:36:in `block in <module:Finisher>'
    from /Users/Csuhta/Projects/project-name/vendor/bundle/ruby/2.1.0/gems/railties-4.1.4/lib/rails/initializable.rb:30:in `instance_exec'
    from /Users/Csuhta/Projects/project-name/vendor/bundle/ruby/2.1.0/gems/railties-4.1.4/lib/rails/initializable.rb:30:in `run'
    from /Users/Csuhta/Projects/project-name/vendor/bundle/ruby/2.1.0/gems/railties-4.1.4/lib/rails/initializable.rb:55:in `block in run_initializers'
    from /usr/local/var/rbenv/versions/2.1.2/lib/ruby/2.1.0/tsort.rb:226:in `block in tsort_each'
    from /usr/local/var/rbenv/versions/2.1.2/lib/ruby/2.1.0/tsort.rb:348:in `block (2 levels) in each_strongly_connected_component'
    from /usr/local/var/rbenv/versions/2.1.2/lib/ruby/2.1.0/tsort.rb:427:in `each_strongly_connected_component_from'
    from /usr/local/var/rbenv/versions/2.1.2/lib/ruby/2.1.0/tsort.rb:347:in `block in each_strongly_connected_component'
    from /usr/local/var/rbenv/versions/2.1.2/lib/ruby/2.1.0/tsort.rb:345:in `each'
    from /usr/local/var/rbenv/versions/2.1.2/lib/ruby/2.1.0/tsort.rb:345:in `call'
    from /usr/local/var/rbenv/versions/2.1.2/lib/ruby/2.1.0/tsort.rb:345:in `each_strongly_connected_component'
    from /usr/local/var/rbenv/versions/2.1.2/lib/ruby/2.1.0/tsort.rb:224:in `tsort_each'
    from /usr/local/var/rbenv/versions/2.1.2/lib/ruby/2.1.0/tsort.rb:205:in `tsort_each'
    from /Users/Csuhta/Projects/project-name/vendor/bundle/ruby/2.1.0/gems/railties-4.1.4/lib/rails/initializable.rb:54:in `run_initializers'
    from /Users/Csuhta/Projects/project-name/vendor/bundle/ruby/2.1.0/gems/railties-4.1.4/lib/rails/application.rb:300:in `initialize!'
    from /Users/Csuhta/Projects/project-name/vendor/bundle/ruby/2.1.0/gems/railties-4.1.4/lib/rails/railtie.rb:194:in `public_send'
    from /Users/Csuhta/Projects/project-name/vendor/bundle/ruby/2.1.0/gems/railties-4.1.4/lib/rails/railtie.rb:194:in `method_missing'
    from /Users/Csuhta/Projects/project-name/config/environment.rb:5:in `<top (required)>'
    from config.ru:3:in `require'
    from config.ru:3:in `block in <main>'
    from /Users/Csuhta/Projects/project-name/vendor/bundle/ruby/2.1.0/gems/rack-1.5.2/lib/rack/builder.rb:55:in `instance_eval'
    from /Users/Csuhta/Projects/project-name/vendor/bundle/ruby/2.1.0/gems/rack-1.5.2/lib/rack/builder.rb:55:in `initialize'
    from config.ru:1:in `new'
    from config.ru:1:in `<main>'
    from /Users/Csuhta/Projects/project-name/vendor/bundle/ruby/2.1.0/gems/unicorn-4.8.3/lib/unicorn.rb:48:in `eval'
    from /Users/Csuhta/Projects/project-name/vendor/bundle/ruby/2.1.0/gems/unicorn-4.8.3/lib/unicorn.rb:48:in `block in builder'
    from /Users/Csuhta/Projects/project-name/vendor/bundle/ruby/2.1.0/gems/unicorn-4.8.3/lib/unicorn/http_server.rb:764:in `call'
    from /Users/Csuhta/Projects/project-name/vendor/bundle/ruby/2.1.0/gems/unicorn-4.8.3/lib/unicorn/http_server.rb:764:in `build_app!'
    from /Users/Csuhta/Projects/project-name/vendor/bundle/ruby/2.1.0/gems/unicorn-4.8.3/lib/unicorn/http_server.rb:137:in `start'
    from /Users/Csuhta/Projects/project-name/vendor/bundle/ruby/2.1.0/gems/unicorn-4.8.3/bin/unicorn:126:in `<top (required)>'
    from /Users/Csuhta/Projects/project-name/vendor/bundle/ruby/2.1.0/bin/unicorn:23:in `load'
    from /Users/Csuhta/Projects/project-name/vendor/bundle/ruby/2.1.0/bin/unicorn:23:in `<main>'

The solution is to write an initializer such as

# config/initalizers/disable_rack_runtime.rb
Rails.application.config.middleware.delete Rack::Runtime

but this is counter-inutative and against the instructions in the Rails Guide.

I feel that Rack::Runtime shouldn't be considered essential by any part of Rails or assumed to exist.

@guilleiguaran

This comment has been minimized.

Show comment
Hide comment
@guilleiguaran

guilleiguaran Aug 8, 2014

Member

I'll investigate this

Member

guilleiguaran commented Aug 8, 2014

I'll investigate this

@rafaelfranca rafaelfranca self-assigned this Aug 8, 2014

@guilleiguaran guilleiguaran added this to the 4.2.0 milestone Aug 8, 2014

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Aug 8, 2014

Member

@guilleiguaran actually this is a bug so it should be on 4.0.x too.

Member

rafaelfranca commented Aug 8, 2014

@guilleiguaran actually this is a bug so it should be on 4.0.x too.

@rafaelfranca rafaelfranca modified the milestones: 4.2.0, 4.0.9 Aug 8, 2014

@guilleiguaran

This comment has been minimized.

Show comment
Hide comment
@guilleiguaran
Member

guilleiguaran commented Aug 8, 2014

@rafaelfranca sure 👍

@guilleiguaran

This comment has been minimized.

Show comment
Hide comment
@guilleiguaran

guilleiguaran Aug 8, 2014

Member

Yes, I just saw the slides.

We need to discuss if Rack::Runtime should be a default or not.

Member

guilleiguaran commented Aug 8, 2014

Yes, I just saw the slides.

We need to discuss if Rack::Runtime should be a default or not.

@csuhta

This comment has been minimized.

Show comment
Hide comment
@csuhta

csuhta Aug 8, 2014

I find Rack::Runtime to be unhelpful and opaque. Simply getting a number of seconds from an HTTP header is not any sort of information you can really act on or investigate efficiently. We have real app metrics like NewRelic and friends.

csuhta commented Aug 8, 2014

I find Rack::Runtime to be unhelpful and opaque. Simply getting a number of seconds from an HTTP header is not any sort of information you can really act on or investigate efficiently. We have real app metrics like NewRelic and friends.

@rafaelfranca rafaelfranca modified the milestones: 4.0.10, 4.1.7 Aug 21, 2014

@bronzle

This comment has been minimized.

Show comment
Hide comment
@bronzle

bronzle Sep 4, 2014

Contributor

My first thought on this is to save delete operations in a separate array in MiddlewareStackProxy and run those operations last, this would preserve the order of all other operations, ensuring default middleware can be referenced, and optionally deleted after everything else is setup. I have a branch ready to push if that sounds ok.

Contributor

bronzle commented Sep 4, 2014

My first thought on this is to save delete operations in a separate array in MiddlewareStackProxy and run those operations last, this would preserve the order of all other operations, ensuring default middleware can be referenced, and optionally deleted after everything else is setup. I have a branch ready to push if that sounds ok.

@tgxworld

This comment has been minimized.

Show comment
Hide comment
@tgxworld

tgxworld Feb 19, 2015

Contributor

@guilleiguaran This issue can be closed 😄 btw, has a decision been made on whether Rack::Runtime should be in the default stack?

Contributor

tgxworld commented Feb 19, 2015

@guilleiguaran This issue can be closed 😄 btw, has a decision been made on whether Rack::Runtime should be in the default stack?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Feb 19, 2015

Member

@tgxworld we didn't discuss this topic yet.

Member

rafaelfranca commented Feb 19, 2015

@tgxworld we didn't discuss this topic yet.

@csuhta

This comment has been minimized.

Show comment
Hide comment
@csuhta

csuhta Feb 19, 2015

@rafaelfranca Do you want me to open a new issue about keeping/removing Rack::Runtime with a summary?

csuhta commented Feb 19, 2015

@rafaelfranca Do you want me to open a new issue about keeping/removing Rack::Runtime with a summary?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Feb 19, 2015

Member

Not an issue, but it would be great a thread in the Rails core mailing
list.

On Thu, Feb 19, 2015, 14:37 Corey Csuhta notifications@github.com wrote:

@rafaelfranca https://github.com/rafaelfranca Do you want me to open a
new issue about keeping/removing Rack::Runtime?


Reply to this email directly or view it on GitHub
#16433 (comment).

Member

rafaelfranca commented Feb 19, 2015

Not an issue, but it would be great a thread in the Rails core mailing
list.

On Thu, Feb 19, 2015, 14:37 Corey Csuhta notifications@github.com wrote:

@rafaelfranca https://github.com/rafaelfranca Do you want me to open a
new issue about keeping/removing Rack::Runtime?


Reply to this email directly or view it on GitHub
#16433 (comment).

gsamokovarov added a commit to gsamokovarov/rails that referenced this issue Dec 24, 2017

Delayed middleware delete does not allow move operations
While trying to fix rails#16433, we made the middleware deletions always
happen at the end. While this works for the case of deleting the
Rack::Runtime middleware, it makes operations like the following
misbehave.

```ruby
gem "bundler", "< 1.16"

begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", github: "rails/rails"
end

require "action_controller/railtie"

class TestApp < Rails::Application
  config.root = __dir__
  secrets.secret_key_base = "secret_key_base"

  config.logger = Logger.new($stdout)
  Rails.logger  = config.logger

  middleware.insert_after ActionDispatch::Session::CookieStore, ::Rails::Rack::Logger, config.log_tags
  middleware.delete ::Rails::Rack::Logger
end

require "minitest/autorun"
require "rack/test"

class BugTest < Minitest::Test
  include Rack::Test::Methods

  def test_returns_success
    get "/"
    assert last_response.ok?
  end

  private
    def app
      Rails.application
    end
end
```

In the case ☝️  the ::Rails::Rack::Logger would be deleted instead of
moved, because the order of middleware stack building execution will be:

```ruby
[:insert, ActionDispatch::Session::CookieStore, [::Rails::Rack::Logger]]
[:delete, ::Rails::Rack::Logger, [config.log_tags]]
```

This is pretty surprising and hard to reason about behaviour, unless you
go spelunking into the Rails configuration code.

I have a few solutions in mind and all of them have their drawbacks.

1. Introduce a `Rails::Configuration::MiddlewareStackProxy#delete!` that
delays the deleted operations. This will make `#delete` to be executed
in order. The drawback here is backwards incompatible behavior and a new
public method.

2. Just revert to the old operations. This won't allow people to delete
the `Rack::Runtime` middleware.

3. Legitimize the middleware moving with the new `#move_after` and
`#move_before` methods. This does not breaks any backwards
compatibility, but includes 2 new methods to the middleware stack.

I have implemented `3.` in this pull request.

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