Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Defer reloading until #close is called on request body #137

Closed
wants to merge 3 commits into from

2 participants

@jfirebaugh

https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/2873

Defer reloading until #close is called on request body [#2873 state:resolved]

Reloading in the ActionDispatch::Callback middleware's after hook
was problematic with streaming responses such as the following:

self.response_body = lambda do |response, output|
  # code here which refers to application models
end

A new middleware, ActionDispatch::Reloader, provides an appropriate
callback hook and is responsible for lock management. It is included
in the stack only in the !config.cache_classes case.

Based on the implementation on the 2-3-stable branch and patches
by Hongli Lai hongli@phusion.nl. His patches included locking
around the request cycle; this is now handled by Rack::Lock
(https://github.com/rack/rack/issues/issue/87/).

@josevalim
Owner

Hey, thanks for the pull request. However, I am not quite comfortable with this approach. Using .extend in runtime is slow (as it busts Ruby's method cache) and the patch also introduces a new API that is quite similar to AS::Callbacks.

The best way to solve the first problem is to have a callbacks system in the response, something like response.close_callback { }, and the block would be called once close! is invoked. Then the current AD::Callback middleware would simply do something like:

if response.respond_to?(:close) && response.respond_to?(:close_callback)
  response.close_callback { _run_after_callbacks }
else
  _run_after_callbacks
end

But we would need to break _run_call_callbacks in AD::Callback in two. The only problem with this approach, afaik, is that it is non deterministic when stuff gets unloaded. Maybe we could use a proxy to wrap objects that do not respond to close! and ensure it gets called in all cases. Not sure of the performance hit as well though.

@jfirebaugh

Is performance really a concern here? This is development mode functionality and reloading is almost certainly going to bust the method cache anyway.

Can you explain your second concern? I'm not sure what duplication you are referring to.

@josevalim
Owner

Oh, snap! You are right, if this is only for development, there is no need to worry with performance.

The second problem was about introducing a new class. I would vote to have AD::Callbacks.to_reload instead of AD::Reload.to_reload. We already have the information if we should run it in each request or not inside the middleware: https://github.com/bigfix/rails/blob/732669640c8ed4c9955bfeb8f58fa87c1631029a/actionpack/lib/action_dispatch/middleware/callbacks.rb#L47

One last thing is that the "railties/lib/rails/console/app.rb" change is wrong. If we had AD::Callbacks and AD::Reloader, both should be called, because stuff like reloading I18n or setting observers occurs in AD::Callbacks and after your change, it won't happen again. Not your fault, as we probably don't have tests for this area (Rails 2.3 legacy :()

@jfirebaugh

I added some commits as you suggest but I'm not really happy with this approach. It has several code smells:

  • AD::Callbacks has 3-4 responsibilities: preparation, reloading, before/after callbacks -- SRP violation.
  • A boolean flag (prepare_each_request/development_mode) that affects control flow in a non-trivial manner.
  • Increased coupling in the assumption that development mode = !config.cache_classes = prepare_each_request.

bigfix@a83b36a#commitcomment-221642

@josevalim
Owner

I wouldn't say it has 3-4 responsibilities, because we wouldn't create 1 class for each callback. But I definitely agree it has two: execute request callbacks and execute reload-like callbacks. before and after callbacks fits the first description, to_prepare and to_reload the second. So I am ok if we split these in two middlewares, one being AD::Reload, just be careful with:

1) to_prepare is like before() for Reload and to_reload is after(). The difference though, is that to_prepare is executed when the middleware is initialized. This is not necessarily good and we should remove it as the middleware will never be initialized in production once it is part of AD::Reload, so we will need to add an initializer here (https://github.com/bigfix/rails/blob/master/railties/lib/rails/application/finisher.rb#L37). That should work like this:

initializer :run_prepare_callbacks do
  ActionDispatch::Reloader.prepare!
end

2) Backward compatibility. Maybe AD::Callbacks.to_prepare can delegate to AD::Reload.to_prepare. We can worry about adding a deprecation warning later.

Thanks for the patience and the feedback. I believe we are coming up with the right solution. :D

@jfirebaugh

Yeah, I like that approach. Thanks for your help José.

I will probably rewrite the git history to remove some of the false starts -- is that OK?

@josevalim
Owner

Sure, that is perfect. I am going to zzzz now, I will check what you push in ~8h. :D

John Firebaugh added some commits
John Firebaugh Introduce ActionDispatch::Reloader
Based on the implementation on the 2-3-stable branch, patches by Hongli
Lai <hongli@phusion.nl>, and helpful suggestions from José Valim.

Hongli Lai's patches included locking around the request cycle; this is
now handled by Rack::Lock (https://github.com/rack/rack/issues/issue/87/).

[#2873]
57c7ed0
John Firebaugh Replace AD::Callbacks.to_prepare with AD::Reloader.to_prepare b39f753
John Firebaugh Use AD::Reloader.to_cleanup for reloading [#2873 state:resolved] 3b90dc0
@jfirebaugh

Committed in e683ab7

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 20, 2010
  1. Introduce ActionDispatch::Reloader

    John Firebaugh authored
    Based on the implementation on the 2-3-stable branch, patches by Hongli
    Lai <hongli@phusion.nl>, and helpful suggestions from José Valim.
    
    Hongli Lai's patches included locking around the request cycle; this is
    now handled by Rack::Lock (https://github.com/rack/rack/issues/issue/87/).
    
    [#2873]
This page is out of date. Refresh to see the latest.
View
1  actionpack/lib/action_dispatch.rb
@@ -53,6 +53,7 @@ module ActionDispatch
autoload :Flash
autoload :Head
autoload :ParamsParser
+ autoload :Reloader
autoload :RemoteIp
autoload :Rescue
autoload :ShowExceptions
View
31 actionpack/lib/action_dispatch/middleware/callbacks.rb
@@ -1,32 +1,14 @@
module ActionDispatch
# Provide callbacks to be executed before and after the request dispatch.
- #
- # It also provides a to_prepare callback, which is performed in all requests
- # in development by only once in production and notification callback for async
- # operations.
- #
class Callbacks
include ActiveSupport::Callbacks
define_callbacks :call, :rescuable => true
- define_callbacks :prepare, :scope => :name
- # Add a preparation callback. Preparation callbacks are run before every
- # request in development mode, and before the first request in production mode.
- #
- # If a symbol with a block is given, the symbol is used as an identifier.
- # That allows to_prepare to be called again with the same identifier to
- # replace the existing callback. Passing an identifier is a suggested
- # practice if the code adding a preparation block may be reloaded.
def self.to_prepare(*args, &block)
- first_arg = args.first
- if first_arg.is_a?(Symbol) && block_given?
- remove_method :"__#{first_arg}" if method_defined?(:"__#{first_arg}")
- define_method :"__#{first_arg}", &block
- set_callback(:prepare, :"__#{first_arg}")
- else
- set_callback(:prepare, *args, &block)
- end
+ ActiveSupport::Deprecation.warn "ActionDispatch::Callbacks.to_prepare is deprecated. " <<
+ "Please use ActionDispatch::Reloader.to_prepare instead."
+ ActionDispatch::Reloader.to_prepare(*args, &block)
end
def self.before(*args, &block)
@@ -37,14 +19,13 @@ def self.after(*args, &block)
set_callback(:call, :after, *args, &block)
end
- def initialize(app, prepare_each_request = false)
- @app, @prepare_each_request = app, prepare_each_request
- _run_prepare_callbacks
+ def initialize(app, unused = nil)
+ ActiveSupport::Deprecation.warn "Passing a second argument to ActionDispatch::Callbacks.new is deprecated." unless unused.nil?
+ @app = app
end
def call(env)
_run_call_callbacks do
- _run_prepare_callbacks if @prepare_each_request
@app.call(env)
end
end
View
82 actionpack/lib/action_dispatch/middleware/reloader.rb
@@ -0,0 +1,82 @@
+module ActionDispatch
+ # ActionDispatch::Reloader provides to_prepare and to_cleanup callbacks.
+ # These are analogs of ActionDispatch::Callback's before and after
+ # callbacks, with the difference that to_cleanup is not called until the
+ # request is fully complete -- that is, after #close has been called on
+ # the request body. This is important for streaming responses such as the
+ # following:
+ #
+ # self.response_body = lambda { |response, output|
+ # # code here which refers to application models
+ # }
+ #
+ # Cleanup callbacks will not be called until after the response_body lambda
+ # is evaluated, ensuring that it can refer to application models and other
+ # classes before they are unloaded.
+ #
+ # By default, ActionDispatch::Reloader is included in the middleware stack
+ # only in the development environment.
+ #
+ class Reloader
+ include ActiveSupport::Callbacks
+
+ define_callbacks :prepare, :scope => :name
+ define_callbacks :cleanup, :scope => :name
+
+ # Add a preparation callback. Preparation callbacks are run before each
+ # request.
+ #
+ # If a symbol with a block is given, the symbol is used as an identifier.
+ # That allows to_prepare to be called again with the same identifier to
+ # replace the existing callback. Passing an identifier is a suggested
+ # practice if the code adding a preparation block may be reloaded.
+ def self.to_prepare(*args, &block)
+ first_arg = args.first
+ if first_arg.is_a?(Symbol) && block_given?
+ remove_method :"__#{first_arg}" if method_defined?(:"__#{first_arg}")
+ define_method :"__#{first_arg}", &block
+ set_callback(:prepare, :"__#{first_arg}")
+ else
+ set_callback(:prepare, *args, &block)
+ end
+ end
+
+ # Add a cleanup callback. Cleanup callbacks are run after each request is
+ # complete (after #close is called on the response body).
+ def self.to_cleanup(&block)
+ set_callback(:cleanup, &block)
+ end
+
+ def self.prepare!
+ new(nil).send(:_run_prepare_callbacks)
+ end
+
+ def self.cleanup!
+ new(nil).send(:_run_cleanup_callbacks)
+ end
+
+ def self.reload!
+ prepare!
+ cleanup!
+ end
+
+ def initialize(app)
+ @app = app
+ end
+
+ module CleanupOnClose
+ def close
+ super if defined?(super)
+ ensure
+ ActionDispatch::Reloader.cleanup!
+ end
+ end
+
+ def call(env)
+ _run_prepare_callbacks
+ response = @app.call(env)
+ response[2].extend(CleanupOnClose)
+ response
+ end
+ end
+end
View
72 actionpack/test/dispatch/callbacks_test.rb
@@ -1,6 +1,6 @@
require 'abstract_unit'
-class DispatcherTest < Test::Unit::TestCase
+class DispatcherTest < ActiveSupport::TestCase
class Foo
cattr_accessor :a, :b
end
@@ -13,65 +13,9 @@ def call(env)
def setup
Foo.a, Foo.b = 0, 0
- ActionDispatch::Callbacks.reset_callbacks(:prepare)
ActionDispatch::Callbacks.reset_callbacks(:call)
end
- def test_prepare_callbacks_with_cache_classes
- a = b = c = nil
- ActionDispatch::Callbacks.to_prepare { |*args| a = b = c = 1 }
- ActionDispatch::Callbacks.to_prepare { |*args| b = c = 2 }
- ActionDispatch::Callbacks.to_prepare { |*args| c = 3 }
-
- # Ensure to_prepare callbacks are not run when defined
- assert_nil a || b || c
-
- # Run callbacks
- dispatch
-
- assert_equal 1, a
- assert_equal 2, b
- assert_equal 3, c
-
- # Make sure they are only run once
- a = b = c = nil
- dispatch
- assert_nil a || b || c
- end
-
- def test_prepare_callbacks_without_cache_classes
- a = b = c = nil
- ActionDispatch::Callbacks.to_prepare { |*args| a = b = c = 1 }
- ActionDispatch::Callbacks.to_prepare { |*args| b = c = 2 }
- ActionDispatch::Callbacks.to_prepare { |*args| c = 3 }
-
- # Ensure to_prepare callbacks are not run when defined
- assert_nil a || b || c
-
- # Run callbacks
- dispatch(false)
-
- assert_equal 1, a
- assert_equal 2, b
- assert_equal 3, c
-
- # Make sure they are run again
- a = b = c = nil
- dispatch(false)
- assert_equal 1, a
- assert_equal 2, b
- assert_equal 3, c
- end
-
- def test_to_prepare_with_identifier_replaces
- ActionDispatch::Callbacks.to_prepare(:unique_id) { |*args| Foo.a, Foo.b = 1, 1 }
- ActionDispatch::Callbacks.to_prepare(:unique_id) { |*args| Foo.a = 2 }
-
- dispatch
- assert_equal 2, Foo.a
- assert_equal 0, Foo.b
- end
-
def test_before_and_after_callbacks
ActionDispatch::Callbacks.before { |*args| Foo.a += 1; Foo.b += 1 }
ActionDispatch::Callbacks.after { |*args| Foo.a += 1; Foo.b += 1 }
@@ -85,10 +29,20 @@ def test_before_and_after_callbacks
assert_equal 4, Foo.b
end
+ def test_to_prepare_deprecation
+ prepared = false
+ assert_deprecated do
+ ActionDispatch::Callbacks.to_prepare { prepared = true }
+ end
+
+ ActionDispatch::Reloader.prepare!
+ assert prepared
+ end
+
private
- def dispatch(cache_classes = true, &block)
- @dispatcher ||= ActionDispatch::Callbacks.new(block || DummyApp.new, !cache_classes)
+ def dispatch(&block)
+ @dispatcher ||= ActionDispatch::Callbacks.new(block || DummyApp.new)
@dispatcher.call({'rack.input' => StringIO.new('')})
end
View
139 actionpack/test/dispatch/reloader_test.rb
@@ -0,0 +1,139 @@
+require 'abstract_unit'
+
+class ReloaderTest < Test::Unit::TestCase
+ Reloader = ActionDispatch::Reloader
+
+ def test_prepare_callbacks
+ a = b = c = nil
+ Reloader.to_prepare { |*args| a = b = c = 1 }
+ Reloader.to_prepare { |*args| b = c = 2 }
+ Reloader.to_prepare { |*args| c = 3 }
+
+ # Ensure to_prepare callbacks are not run when defined
+ assert_nil a || b || c
+
+ # Run callbacks
+ call_and_return_body
+
+ assert_equal 1, a
+ assert_equal 2, b
+ assert_equal 3, c
+ end
+
+ def test_to_prepare_with_identifier_replaces
+ a = b = 0
+ Reloader.to_prepare(:unique_id) { |*args| a = b = 1 }
+ Reloader.to_prepare(:unique_id) { |*args| a = 2 }
+
+ call_and_return_body
+ assert_equal 2, a
+ assert_equal 0, b
+ end
+
+ class MyBody < Array
+ def initialize(&block)
+ @on_close = block
+ end
+
+ def foo
+ "foo"
+ end
+
+ def bar
+ "bar"
+ end
+
+ def close
+ @on_close.call if @on_close
+ end
+ end
+
+ def test_returned_body_object_always_responds_to_close
+ body = call_and_return_body
+ assert body.respond_to?(:close)
+ end
+
+ def test_returned_body_object_behaves_like_underlying_object
+ body = call_and_return_body do
+ b = MyBody.new
+ b << "hello"
+ b << "world"
+ [200, { "Content-Type" => "text/html" }, b]
+ end
+ assert_equal 2, body.size
+ assert_equal "hello", body[0]
+ assert_equal "world", body[1]
+ assert_equal "foo", body.foo
+ assert_equal "bar", body.bar
+ end
+
+ def test_it_calls_close_on_underlying_object_when_close_is_called_on_body
+ close_called = false
+ body = call_and_return_body do
+ b = MyBody.new do
+ close_called = true
+ end
+ [200, { "Content-Type" => "text/html" }, b]
+ end
+ body.close
+ assert close_called
+ end
+
+ def test_returned_body_object_responds_to_all_methods_supported_by_underlying_object
+ body = call_and_return_body do
+ [200, { "Content-Type" => "text/html" }, MyBody.new]
+ end
+ assert body.respond_to?(:size)
+ assert body.respond_to?(:each)
+ assert body.respond_to?(:foo)
+ assert body.respond_to?(:bar)
+ end
+
+ def test_cleanup_callbacks_are_called_when_body_is_closed
+ cleaned = false
+ Reloader.to_cleanup { cleaned = true }
+
+ body = call_and_return_body
+ assert !cleaned
+
+ body.close
+ assert cleaned
+ end
+
+ def test_prepare_callbacks_arent_called_when_body_is_closed
+ prepared = false
+ Reloader.to_prepare { prepared = true }
+
+ body = call_and_return_body
+ prepared = false
+
+ body.close
+ assert !prepared
+ end
+
+ def test_manual_reloading
+ prepared = cleaned = false
+ Reloader.to_prepare { prepared = true }
+ Reloader.to_cleanup { cleaned = true }
+
+ Reloader.prepare!
+ assert prepared
+ assert !cleaned
+
+ prepared = cleaned = false
+ Reloader.cleanup!
+ assert !prepared
+ assert cleaned
+
+ prepared = cleaned = false
+ Reloader.reload!
+ assert prepared
+ assert cleaned
+ end
+
+ private
+ def call_and_return_body(&block)
+ @reloader ||= Reloader.new(block || proc {[200, {}, 'response']})
+ @reloader.call({'rack.input' => StringIO.new('')})[2]
+ end
+end
View
10 activerecord/lib/active_record/railtie.rb
@@ -69,11 +69,9 @@ class Railtie < Rails::Railtie
end
initializer "active_record.set_dispatch_hooks", :before => :set_clear_dependencies_hook do |app|
- unless app.config.cache_classes
- ActiveSupport.on_load(:active_record) do
- ActionDispatch::Callbacks.after do
- ActiveRecord::Base.clear_reloadable_connections!
- end
+ ActiveSupport.on_load(:active_record) do
+ ActionDispatch::Reloader.to_cleanup do
+ ActiveRecord::Base.clear_reloadable_connections!
end
end
end
@@ -82,7 +80,7 @@ class Railtie < Rails::Railtie
ActiveSupport.on_load(:active_record) do
instantiate_observers
- ActionDispatch::Callbacks.to_prepare(:activerecord_instantiate_observers) do
+ ActionDispatch::Reloader.to_prepare(:activerecord_instantiate_observers) do
ActiveRecord::Base.instantiate_observers
end
end
View
2  activesupport/lib/active_support/file_update_checker.rb
@@ -8,7 +8,7 @@ module ActiveSupport
# I18n.reload!
# end
#
- # ActionDispatch::Callbacks.to_prepare do
+ # ActionDispatch::Reloader.to_prepare do
# i18n_reloader.execute_if_updated
# end
#
View
2  activesupport/lib/active_support/i18n_railtie.rb
@@ -19,7 +19,7 @@ def self.reloader
# on to_prepare callbacks. This will only happen on the config.after_initialize
# callback below.
initializer "i18n.callbacks" do
- ActionDispatch::Callbacks.to_prepare do
+ ActionDispatch::Reloader.to_prepare do
I18n::Railtie.reloader.execute_if_updated
end
end
View
3  railties/lib/rails/application.rb
@@ -156,7 +156,8 @@ def default_middleware_stack
middleware.use ::ActionDispatch::ShowExceptions, config.consider_all_requests_local if config.action_dispatch.show_exceptions
middleware.use ::ActionDispatch::RemoteIp, config.action_dispatch.ip_spoofing_check, config.action_dispatch.trusted_proxies
middleware.use ::Rack::Sendfile, config.action_dispatch.x_sendfile_header
- middleware.use ::ActionDispatch::Callbacks, !config.cache_classes
+ middleware.use ::ActionDispatch::Reloader unless config.cache_classes
+ middleware.use ::ActionDispatch::Callbacks
middleware.use ::ActionDispatch::Cookies
if config.session_store
View
8 railties/lib/rails/application/bootstrap.rb
@@ -51,11 +51,9 @@ module Bootstrap
end
initializer :set_clear_dependencies_hook do
- unless config.cache_classes
- ActionDispatch::Callbacks.after do
- ActiveSupport::DescendantsTracker.clear
- ActiveSupport::Dependencies.clear
- end
+ ActionDispatch::Reloader.to_cleanup do
+ ActiveSupport::DescendantsTracker.clear
+ ActiveSupport::Dependencies.clear
end
end
View
8 railties/lib/rails/application/finisher.rb
@@ -21,7 +21,7 @@ module Finisher
initializer :add_to_prepare_blocks do
config.to_prepare_blocks.each do |block|
- ActionDispatch::Callbacks.to_prepare(&block)
+ ActionDispatch::Reloader.to_prepare(&block)
end
end
@@ -37,6 +37,10 @@ module Finisher
build_middleware_stack
end
+ initializer :run_prepare_callbacks do
+ ActionDispatch::Reloader.prepare!
+ end
+
initializer :eager_load! do
if config.cache_classes && !$rails_rake_task
ActiveSupport.run_load_hooks(:before_eager_load, self)
@@ -52,7 +56,7 @@ module Finisher
initializer :set_routes_reloader do |app|
reloader = lambda { app.routes_reloader.execute_if_updated }
reloader.call
- ActionDispatch::Callbacks.to_prepare(&reloader)
+ ActionDispatch::Reloader.to_prepare(&reloader)
end
# Disable dependency loading during request cycle
View
3  railties/lib/rails/console/app.rb
@@ -26,7 +26,6 @@ def new_session
# reloads the environment
def reload!(print=true)
puts "Reloading..." if print
- # This triggers the to_prepare callbacks
- ActionDispatch::Callbacks.new(Proc.new {}, false).call({})
+ ActionDispatch::Reloader.reload!
true
end
View
8 railties/test/application/console_test.rb
@@ -26,14 +26,14 @@ def test_new_session_should_return_integration_session
assert_instance_of ActionDispatch::Integration::Session, session
end
- def test_reload_should_fire_preparation_callbacks
+ def test_reload_should_fire_preparation_and_cleanup_callbacks
load_environment
a = b = c = nil
# TODO: These should be defined on the initializer
- ActionDispatch::Callbacks.to_prepare { a = b = c = 1 }
- ActionDispatch::Callbacks.to_prepare { b = c = 2 }
- ActionDispatch::Callbacks.to_prepare { c = 3 }
+ ActionDispatch::Reloader.to_prepare { a = b = c = 1 }
+ ActionDispatch::Reloader.to_prepare { b = c = 2 }
+ ActionDispatch::Reloader.to_cleanup { c = 3 }
# Hide Reloading... output
silence_stream(STDOUT) { reload! }
View
7 railties/test/application/middleware_test.rb
@@ -27,6 +27,7 @@ def app
"ActionDispatch::ShowExceptions",
"ActionDispatch::RemoteIp",
"Rack::Sendfile",
+ "ActionDispatch::Reloader",
"ActionDispatch::Callbacks",
"ActiveRecord::ConnectionAdapters::ConnectionManagement",
"ActiveRecord::QueryCache",
@@ -81,6 +82,12 @@ def app
assert !middleware.include?("ActionDispatch::ShowExceptions")
end
+ test "removes ActionDispatch::Reloader if cache_classes is true" do
+ add_to_config "config.cache_classes = true"
+ boot!
+ assert !middleware.include?("ActionDispatch::Reloader")
+ end
+
test "use middleware" do
use_frameworks []
add_to_config "config.middleware.use Rack::Config"
Something went wrong with that request. Please try again.