From c21f735508e421b595a53e79058ebc89e91c6803 Mon Sep 17 00:00:00 2001 From: Hongli Lai (Phusion) Date: Tue, 11 Aug 2009 13:39:48 +0200 Subject: [PATCH] Fix reloading of metal pieces. - Do not hold references to old metal objects after metal classes have been reloaded. - Obtain the reloader lock before building the middleware stack, so that reloading of metal pieces works in the face of multithreading. --- actionpack/lib/action_controller/dispatcher.rb | 27 ++++++++-- actionpack/lib/action_controller/reloader.rb | 22 ++++----- actionpack/test/controller/dispatcher_test.rb | 35 ++++++++++++++ actionpack/test/controller/reloader_test.rb | 59 ++++++++++-------------- 4 files changed, 89 insertions(+), 54 deletions(-) diff --git a/actionpack/lib/action_controller/dispatcher.rb b/actionpack/lib/action_controller/dispatcher.rb index 07931e4..ec05b71 100644 --- a/actionpack/lib/action_controller/dispatcher.rb +++ b/actionpack/lib/action_controller/dispatcher.rb @@ -2,13 +2,12 @@ module ActionController # Dispatches requests to the appropriate controller and takes care of # reloading the app after each request when Dependencies.load? is true. class Dispatcher + @@cache_classes = false + class << self def define_dispatcher_callbacks(cache_classes) + @@cache_classes = cache_classes unless cache_classes - unless self.middleware.include?(Reloader) - self.middleware.insert_after(Failsafe, Reloader) - end - ActionView::Helpers::AssetTagHelper.cache_asset_timestamps = false end @@ -79,7 +78,7 @@ module ActionController # DEPRECATE: Remove arguments, since they are only used by CGI def initialize(output = $stdout, request = nil, response = nil) @output = output - @app = @@middleware.build(lambda { |env| self.dup._call(env) }) + build_middleware_stack if @@cache_classes end def dispatch @@ -103,7 +102,18 @@ module ActionController end def call(env) - @app.call(env) + if @@cache_classes + @app.call(env) + else + Reloader.run do + # When class reloading is turned on, we will want to rebuild the + # middleware stack every time we process a request. If we don't + # rebuild the middleware stack, then the stack may contain references + # to old classes metal classes, which will b0rk class reloading. + build_middleware_stack + @app.call(env) + end + end end def _call(env) @@ -114,5 +124,10 @@ module ActionController def flush_logger Base.logger.flush end + + private + def build_middleware_stack + @app = @@middleware.build(lambda { |env| self.dup._call(env) }) + end end end diff --git a/actionpack/lib/action_controller/reloader.rb b/actionpack/lib/action_controller/reloader.rb index e47d5ce..83bc9a9 100644 --- a/actionpack/lib/action_controller/reloader.rb +++ b/actionpack/lib/action_controller/reloader.rb @@ -2,7 +2,8 @@ require 'thread' module ActionController class Reloader - @@lock = Mutex.new + @@default_lock = Mutex.new + cattr_accessor :default_lock class BodyWrapper def initialize(body, lock) @@ -25,17 +26,12 @@ module ActionController symbol == :close || @body.respond_to?(symbol, include_private) end end - - def initialize(app, lock = @@lock) - @app = app - @lock = lock - end - - def call(env) - @lock.lock - Dispatcher.reload_application + + def self.run(lock = @@default_lock) + lock.lock begin - status, headers, body = @app.call(env) + Dispatcher.reload_application + status, headers, body = yield # We do not want to call 'cleanup_application' in an ensure block # because the returned Rack response body may lazily generate its data. This # is for example the case if one calls @@ -48,9 +44,9 @@ module ActionController # completely finished. So we wrap the body in a BodyWrapper class so that # when the Rack handler calls #close during the end of the request, we get to # run our cleanup code. - [status, headers, BodyWrapper.new(body, @lock)] + [status, headers, BodyWrapper.new(body, lock)] rescue Exception - @lock.unlock + lock.unlock raise end end diff --git a/actionpack/test/controller/dispatcher_test.rb b/actionpack/test/controller/dispatcher_test.rb index 3aa5bf9..e2ff324 100644 --- a/actionpack/test/controller/dispatcher_test.rb +++ b/actionpack/test/controller/dispatcher_test.rb @@ -2,6 +2,7 @@ require 'abstract_unit' class DispatcherTest < Test::Unit::TestCase Dispatcher = ActionController::Dispatcher + Reloader = ActionController::Reloader def setup ENV['REQUEST_METHOD'] = 'GET' @@ -21,6 +22,7 @@ class DispatcherTest < Test::Unit::TestCase def teardown ENV.delete 'REQUEST_METHOD' + Reloader.default_lock = Mutex.new end def test_clears_dependencies_after_dispatch_if_in_loading_mode @@ -40,6 +42,34 @@ class DispatcherTest < Test::Unit::TestCase dispatch end + + def test_builds_middleware_stack_only_during_initialization_if_not_in_loading_mode + dispatcher = create_dispatcher + assert_not_nil dispatcher.instance_variable_get(:"@app") + dispatcher.instance_variable_set(:"@app", lambda {}) + dispatcher.expects(:build_middleware_stack).never + dispatcher.call(nil) + dispatcher.call(nil) + end + + def test_rebuilds_middleware_stack_on_every_request_if_in_loading_mode + dispatcher = create_dispatcher(false) + dispatcher.instance_variable_set(:"@app", lambda {}) + dispatcher.expects(:build_middleware_stack).twice + dispatcher.call(nil) + Reloader.default_lock = Mutex.new + dispatcher.call(nil) + end + + def test_doesnt_wrap_call_in_reloader_if_not_in_loading_mode + Reloader.expects(:run).never + dispatch + end + + def test_wraps_call_in_reloader_if_in_loading_mode + Reloader.expects(:run).once + dispatch(false) + end # Stub out dispatch error logger class << Dispatcher @@ -98,6 +128,11 @@ class DispatcherTest < Test::Unit::TestCase Dispatcher.define_dispatcher_callbacks(cache_classes) Dispatcher.new.call({'rack.input' => StringIO.new('')}) end + + def create_dispatcher(cache_classes = true) + Dispatcher.define_dispatcher_callbacks(cache_classes) + Dispatcher.new + end def assert_subclasses(howmany, klass, message = klass.subclasses.inspect) assert_equal howmany, klass.subclasses.size, message diff --git a/actionpack/test/controller/reloader_test.rb b/actionpack/test/controller/reloader_test.rb index 9eab9e1..c3fe09e 100644 --- a/actionpack/test/controller/reloader_test.rb +++ b/actionpack/test/controller/reloader_test.rb @@ -40,39 +40,29 @@ class ReloaderTests < ActiveSupport::TestCase @lock = Mutex.new end - def setup_and_return_body(app = lambda { |env| }) + def test_it_reloads_the_application_before_yielding Dispatcher.expects(:reload_application) - reloader = Reloader.new(app, @lock) - headers, status, body = reloader.call({ }) - body - end - - def test_it_reloads_the_application_before_the_request - Dispatcher.expects(:reload_application) - reloader = Reloader.new(lambda { |env| + Reloader.run(@lock) do [200, { "Content-Type" => "text/html" }, [""]] - }, @lock) - reloader.call({ }) + end end - def test_it_locks_before_calling_app + def test_it_locks_before_yielding lock = MyLock.new Dispatcher.expects(:reload_application) - reloader = Reloader.new(lambda { |env| + Reloader.run(lock) do + assert lock.locked? [200, { "Content-Type" => "text/html" }, [""]] - }, lock) - assert !lock.locked? - reloader.call({ }) + end assert lock.locked? end def test_it_unlocks_upon_calling_close_on_body lock = MyLock.new Dispatcher.expects(:reload_application) - reloader = Reloader.new(lambda { |env| + headers, status, body = Reloader.run(lock) do [200, { "Content-Type" => "text/html" }, [""]] - }, lock) - headers, status, body = reloader.call({ }) + end body.close assert !lock.locked? end @@ -80,29 +70,28 @@ class ReloaderTests < ActiveSupport::TestCase def test_it_unlocks_if_app_object_raises_exception lock = MyLock.new Dispatcher.expects(:reload_application) - reloader = Reloader.new(lambda { |env| - raise "oh no!" - }, lock) assert_raise(RuntimeError) do - reloader.call({ }) + Reloader.run(lock) do + raise "oh no!" + end end assert !lock.locked? end def test_returned_body_object_always_responds_to_close - body = setup_and_return_body(lambda { |env| + status, headers, body = Reloader.run(@lock) do [200, { "Content-Type" => "text/html" }, [""]] - }) + end assert body.respond_to?(:close) end def test_returned_body_object_behaves_like_underlying_object - body = setup_and_return_body(lambda { |env| + status, headers, body = Reloader.run(@lock) 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] @@ -112,20 +101,20 @@ class ReloaderTests < ActiveSupport::TestCase def test_it_calls_close_on_underlying_object_when_close_is_called_on_body close_called = false - body = setup_and_return_body(lambda { |env| + status, headers, body = Reloader.run(@lock) 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 = setup_and_return_body(lambda { |env| + status, headers, body = Reloader.run(@lock) do [200, { "Content-Type" => "text/html" }, MyBody.new] - }) + end assert body.respond_to?(:size) assert body.respond_to?(:each) assert body.respond_to?(:foo) @@ -134,16 +123,16 @@ class ReloaderTests < ActiveSupport::TestCase def test_it_doesnt_clean_up_the_application_after_call Dispatcher.expects(:cleanup_application).never - body = setup_and_return_body(lambda { |env| + status, headers, body = Reloader.run(@lock) do [200, { "Content-Type" => "text/html" }, MyBody.new] - }) + end end def test_it_cleans_up_the_application_when_close_is_called_on_body Dispatcher.expects(:cleanup_application) - body = setup_and_return_body(lambda { |env| + status, headers, body = Reloader.run(@lock) do [200, { "Content-Type" => "text/html" }, MyBody.new] - }) + end body.close end end -- 1.6.0.5