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.

[rails#2873 state:committed]

Signed-off-by: Jeremy Kemper <jeremy@bitsweat.net>

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 = true
+
     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 @@ def cleanup_application
     # 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 @@ def dispatch_cgi(cgi, session_options)
     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 @@ def _call(env)
     def flush_logger
       Base.logger.flush
     end
+
+    private
+      def build_middleware_stack
+        @app = @@middleware.build(lambda { |env| self.dup._call(env) })
+      end
   end
 end

actionpack/lib/action_controller/reloader.rb

@@ -2,7 +2,8 @@
 
 module ActionController
   class Reloader
-    @@lock = Mutex.new
+    @@default_lock = Mutex.new
+    cattr_accessor :default_lock
 
     class BodyWrapper
       def initialize(body, lock)
@@ -26,16 +27,11 @@ def respond_to?(symbol, include_private = false)
       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 @@ def call(env)
         # 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

actionpack/test/abstract_unit.rb

@@ -43,3 +43,19 @@
 CACHED_VIEW_PATHS = ActionView::Base.cache_template_loading? ?
                       ActionController::Base.view_paths :
                       ActionController::Base.view_paths.map {|path| ActionView::Template::EagerPath.new(path.to_s)}
+
+class DummyMutex
+  def lock
+    @locked = true
+  end
+
+  def unlock
+    @locked = false
+  end
+
+  def locked?
+    @locked
+  end
+end
+
+ActionController::Reloader.default_lock = DummyMutex.new

actionpack/test/controller/dispatcher_test.rb

@@ -2,25 +2,17 @@
 
 class DispatcherTest < Test::Unit::TestCase
   Dispatcher = ActionController::Dispatcher
+  Reloader   = ActionController::Reloader
 
   def setup
     ENV['REQUEST_METHOD'] = 'GET'
-
-    Dispatcher.middleware = ActionController::MiddlewareStack.new do |middleware|
-      middlewares = File.expand_path(File.join(File.dirname(__FILE__), "../../lib/action_controller/middlewares.rb"))
-      middleware.instance_eval(File.read(middlewares))
-    end
-
-    # Clear callbacks as they are redefined by Dispatcher#define_dispatcher_callbacks
-    Dispatcher.instance_variable_set("@prepare_dispatch_callbacks", ActiveSupport::Callbacks::CallbackChain.new)
-    Dispatcher.instance_variable_set("@before_dispatch_callbacks", ActiveSupport::Callbacks::CallbackChain.new)
-    Dispatcher.instance_variable_set("@after_dispatch_callbacks", ActiveSupport::Callbacks::CallbackChain.new)
-
+    reset_dispatcher
     Dispatcher.stubs(:require_dependency)
   end
 
   def teardown
     ENV.delete 'REQUEST_METHOD'
+    reset_dispatcher
   end
 
   def test_clears_dependencies_after_dispatch_if_in_loading_mode
@@ -41,6 +33,34 @@ def test_leaves_dependencies_after_dispatch_if_not_in_loading_mode
     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 { |env| })
+    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 { |env| })
+    dispatcher.expects(:build_middleware_stack).twice
+    dispatcher.call(nil)
+    Reloader.default_lock.unlock
+    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
     def log_failsafe_exception(status, exception); end
@@ -99,6 +119,25 @@ def dispatch(cache_classes = true)
       Dispatcher.new.call({'rack.input' => StringIO.new('')})
     end
 
+    def create_dispatcher(cache_classes = true)
+      Dispatcher.define_dispatcher_callbacks(cache_classes)
+      Dispatcher.new
+    end
+
+    def reset_dispatcher
+      Dispatcher.middleware = ActionController::MiddlewareStack.new do |middleware|
+        middlewares = File.expand_path(File.join(File.dirname(__FILE__), "../../lib/action_controller/middlewares.rb"))
+        middleware.instance_eval(File.read(middlewares))
+      end
+
+      # Clear callbacks as they are redefined by Dispatcher#define_dispatcher_callbacks
+      Dispatcher.instance_variable_set("@prepare_dispatch_callbacks", ActiveSupport::Callbacks::CallbackChain.new)
+      Dispatcher.instance_variable_set("@before_dispatch_callbacks", ActiveSupport::Callbacks::CallbackChain.new)
+      Dispatcher.instance_variable_set("@after_dispatch_callbacks", ActiveSupport::Callbacks::CallbackChain.new)
+
+      Dispatcher.define_dispatcher_callbacks(true)
+    end
+
     def assert_subclasses(howmany, klass, message = klass.subclasses.inspect)
       assert_equal howmany, klass.subclasses.size, message
     end

actionpack/test/controller/reloader_test.rb

@@ -22,87 +22,62 @@ def close
     end
   end
 
-  class MyLock
-    def lock
-      @locked = true
-    end
-
-    def unlock
-      @locked = false
-    end
-
-    def locked?
-      @locked
-    end
-  end
-
   def setup
     @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
-    lock = MyLock.new
+  def test_it_locks_before_yielding
+    lock = DummyMutex.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
+    lock = DummyMutex.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
 
   def test_it_unlocks_if_app_object_raises_exception
-    lock = MyLock.new
+    lock = DummyMutex.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 +87,20 @@ def test_returned_body_object_behaves_like_underlying_object
 
   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 +109,16 @@ def test_returned_body_object_responds_to_all_methods_supported_by_underlying_ob
 
   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