Skip to content
This repository
Browse code

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.

[#2873 state:committed]

Signed-off-by: Jeremy Kemper <jeremy@bitsweat.net>
  • Loading branch information...
commit 14b6ab0f013f01541fca9fd42b33da96313e0a2e 1 parent 1cf32ad
Hongli Lai authored August 11, 2009 jeremy committed August 15, 2009
27  actionpack/lib/action_controller/dispatcher.rb
@@ -2,13 +2,12 @@ module ActionController
2 2
   # Dispatches requests to the appropriate controller and takes care of
3 3
   # reloading the app after each request when Dependencies.load? is true.
4 4
   class Dispatcher
  5
+    @@cache_classes = true
  6
+
5 7
     class << self
6 8
       def define_dispatcher_callbacks(cache_classes)
  9
+        @@cache_classes = cache_classes
7 10
         unless cache_classes
8  
-          unless self.middleware.include?(Reloader)
9  
-            self.middleware.insert_after(Failsafe, Reloader)
10  
-          end
11  
-
12 11
           ActionView::Helpers::AssetTagHelper.cache_asset_timestamps = false
13 12
         end
14 13
 
@@ -79,7 +78,7 @@ def cleanup_application
79 78
     # DEPRECATE: Remove arguments, since they are only used by CGI
80 79
     def initialize(output = $stdout, request = nil, response = nil)
81 80
       @output = output
82  
-      @app = @@middleware.build(lambda { |env| self.dup._call(env) })
  81
+      build_middleware_stack if @@cache_classes
83 82
     end
84 83
 
85 84
     def dispatch
@@ -103,7 +102,18 @@ def dispatch_cgi(cgi, session_options)
103 102
     end
104 103
 
105 104
     def call(env)
106  
-      @app.call(env)
  105
+      if @@cache_classes
  106
+        @app.call(env)
  107
+      else
  108
+        Reloader.run do
  109
+          # When class reloading is turned on, we will want to rebuild the
  110
+          # middleware stack every time we process a request. If we don't
  111
+          # rebuild the middleware stack, then the stack may contain references
  112
+          # to old classes metal classes, which will b0rk class reloading.
  113
+          build_middleware_stack
  114
+          @app.call(env)
  115
+        end
  116
+      end
107 117
     end
108 118
 
109 119
     def _call(env)
@@ -114,5 +124,10 @@ def _call(env)
114 124
     def flush_logger
115 125
       Base.logger.flush
116 126
     end
  127
+
  128
+    private
  129
+      def build_middleware_stack
  130
+        @app = @@middleware.build(lambda { |env| self.dup._call(env) })
  131
+      end
117 132
   end
118 133
 end
20  actionpack/lib/action_controller/reloader.rb
@@ -2,7 +2,8 @@
2 2
 
3 3
 module ActionController
4 4
   class Reloader
5  
-    @@lock = Mutex.new
  5
+    @@default_lock = Mutex.new
  6
+    cattr_accessor :default_lock
6 7
 
7 8
     class BodyWrapper
8 9
       def initialize(body, lock)
@@ -26,16 +27,11 @@ def respond_to?(symbol, include_private = false)
26 27
       end
27 28
     end
28 29
 
29  
-    def initialize(app, lock = @@lock)
30  
-      @app = app
31  
-      @lock = lock
32  
-    end
33  
-
34  
-    def call(env)
35  
-      @lock.lock
36  
-      Dispatcher.reload_application
  30
+    def self.run(lock = @@default_lock)
  31
+      lock.lock
37 32
       begin
38  
-        status, headers, body = @app.call(env)
  33
+        Dispatcher.reload_application
  34
+        status, headers, body = yield
39 35
         # We do not want to call 'cleanup_application' in an ensure block
40 36
         # because the returned Rack response body may lazily generate its data. This
41 37
         # is for example the case if one calls
@@ -48,9 +44,9 @@ def call(env)
48 44
         # completely finished. So we wrap the body in a BodyWrapper class so that
49 45
         # when the Rack handler calls #close during the end of the request, we get to
50 46
         # run our cleanup code.
51  
-        [status, headers, BodyWrapper.new(body, @lock)]
  47
+        [status, headers, BodyWrapper.new(body, lock)]
52 48
       rescue Exception
53  
-        @lock.unlock
  49
+        lock.unlock
54 50
         raise
55 51
       end
56 52
     end
16  actionpack/test/abstract_unit.rb
@@ -43,3 +43,19 @@
43 43
 CACHED_VIEW_PATHS = ActionView::Base.cache_template_loading? ?
44 44
                       ActionController::Base.view_paths :
45 45
                       ActionController::Base.view_paths.map {|path| ActionView::Template::EagerPath.new(path.to_s)}
  46
+
  47
+class DummyMutex
  48
+  def lock
  49
+    @locked = true
  50
+  end
  51
+
  52
+  def unlock
  53
+    @locked = false
  54
+  end
  55
+
  56
+  def locked?
  57
+    @locked
  58
+  end
  59
+end
  60
+
  61
+ActionController::Reloader.default_lock = DummyMutex.new
61  actionpack/test/controller/dispatcher_test.rb
@@ -2,25 +2,17 @@
2 2
 
3 3
 class DispatcherTest < Test::Unit::TestCase
4 4
   Dispatcher = ActionController::Dispatcher
  5
+  Reloader   = ActionController::Reloader
5 6
 
6 7
   def setup
7 8
     ENV['REQUEST_METHOD'] = 'GET'
8  
-
9  
-    Dispatcher.middleware = ActionController::MiddlewareStack.new do |middleware|
10  
-      middlewares = File.expand_path(File.join(File.dirname(__FILE__), "../../lib/action_controller/middlewares.rb"))
11  
-      middleware.instance_eval(File.read(middlewares))
12  
-    end
13  
-
14  
-    # Clear callbacks as they are redefined by Dispatcher#define_dispatcher_callbacks
15  
-    Dispatcher.instance_variable_set("@prepare_dispatch_callbacks", ActiveSupport::Callbacks::CallbackChain.new)
16  
-    Dispatcher.instance_variable_set("@before_dispatch_callbacks", ActiveSupport::Callbacks::CallbackChain.new)
17  
-    Dispatcher.instance_variable_set("@after_dispatch_callbacks", ActiveSupport::Callbacks::CallbackChain.new)
18  
-
  9
+    reset_dispatcher
19 10
     Dispatcher.stubs(:require_dependency)
20 11
   end
21 12
 
22 13
   def teardown
23 14
     ENV.delete 'REQUEST_METHOD'
  15
+    reset_dispatcher
24 16
   end
25 17
 
26 18
   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
41 33
     dispatch
42 34
   end
43 35
 
  36
+  def test_builds_middleware_stack_only_during_initialization_if_not_in_loading_mode
  37
+    dispatcher = create_dispatcher
  38
+    assert_not_nil dispatcher.instance_variable_get(:"@app")
  39
+    dispatcher.instance_variable_set(:"@app", lambda { |env| })
  40
+    dispatcher.expects(:build_middleware_stack).never
  41
+    dispatcher.call(nil)
  42
+    dispatcher.call(nil)
  43
+  end
  44
+
  45
+  def test_rebuilds_middleware_stack_on_every_request_if_in_loading_mode
  46
+    dispatcher = create_dispatcher(false)
  47
+    dispatcher.instance_variable_set(:"@app", lambda { |env| })
  48
+    dispatcher.expects(:build_middleware_stack).twice
  49
+    dispatcher.call(nil)
  50
+    Reloader.default_lock.unlock
  51
+    dispatcher.call(nil)
  52
+  end
  53
+
  54
+  def test_doesnt_wrap_call_in_reloader_if_not_in_loading_mode
  55
+    Reloader.expects(:run).never
  56
+    dispatch
  57
+  end
  58
+
  59
+  def test_wraps_call_in_reloader_if_in_loading_mode
  60
+    Reloader.expects(:run).once
  61
+    dispatch(false)
  62
+  end
  63
+
44 64
   # Stub out dispatch error logger
45 65
   class << Dispatcher
46 66
     def log_failsafe_exception(status, exception); end
@@ -99,6 +119,25 @@ def dispatch(cache_classes = true)
99 119
       Dispatcher.new.call({'rack.input' => StringIO.new('')})
100 120
     end
101 121
 
  122
+    def create_dispatcher(cache_classes = true)
  123
+      Dispatcher.define_dispatcher_callbacks(cache_classes)
  124
+      Dispatcher.new
  125
+    end
  126
+
  127
+    def reset_dispatcher
  128
+      Dispatcher.middleware = ActionController::MiddlewareStack.new do |middleware|
  129
+        middlewares = File.expand_path(File.join(File.dirname(__FILE__), "../../lib/action_controller/middlewares.rb"))
  130
+        middleware.instance_eval(File.read(middlewares))
  131
+      end
  132
+
  133
+      # Clear callbacks as they are redefined by Dispatcher#define_dispatcher_callbacks
  134
+      Dispatcher.instance_variable_set("@prepare_dispatch_callbacks", ActiveSupport::Callbacks::CallbackChain.new)
  135
+      Dispatcher.instance_variable_set("@before_dispatch_callbacks", ActiveSupport::Callbacks::CallbackChain.new)
  136
+      Dispatcher.instance_variable_set("@after_dispatch_callbacks", ActiveSupport::Callbacks::CallbackChain.new)
  137
+
  138
+      Dispatcher.define_dispatcher_callbacks(true)
  139
+    end
  140
+
102 141
     def assert_subclasses(howmany, klass, message = klass.subclasses.inspect)
103 142
       assert_equal howmany, klass.subclasses.size, message
104 143
     end
79  actionpack/test/controller/reloader_test.rb
@@ -22,87 +22,62 @@ def close
22 22
     end
23 23
   end
24 24
 
25  
-  class MyLock
26  
-    def lock
27  
-      @locked = true
28  
-    end
29  
-
30  
-    def unlock
31  
-      @locked = false
32  
-    end
33  
-
34  
-    def locked?
35  
-      @locked
36  
-    end
37  
-  end
38  
-
39 25
   def setup
40 26
     @lock = Mutex.new
41 27
   end
42 28
 
43  
-  def setup_and_return_body(app = lambda { |env| })
  29
+  def test_it_reloads_the_application_before_yielding
44 30
     Dispatcher.expects(:reload_application)
45  
-    reloader = Reloader.new(app, @lock)
46  
-    headers, status, body = reloader.call({ })
47  
-    body
48  
-  end
49  
-
50  
-  def test_it_reloads_the_application_before_the_request
51  
-    Dispatcher.expects(:reload_application)
52  
-    reloader = Reloader.new(lambda { |env|
  31
+    Reloader.run(@lock) do
53 32
       [200, { "Content-Type" => "text/html" }, [""]]
54  
-    }, @lock)
55  
-    reloader.call({ })
  33
+    end
56 34
   end
57 35
 
58  
-  def test_it_locks_before_calling_app
59  
-    lock = MyLock.new
  36
+  def test_it_locks_before_yielding
  37
+    lock = DummyMutex.new
60 38
     Dispatcher.expects(:reload_application)
61  
-    reloader = Reloader.new(lambda { |env|
  39
+    Reloader.run(lock) do
  40
+      assert lock.locked?
62 41
       [200, { "Content-Type" => "text/html" }, [""]]
63  
-    }, lock)
64  
-    assert !lock.locked?
65  
-    reloader.call({ })
  42
+    end
66 43
     assert lock.locked?
67 44
   end
68 45
 
69 46
   def test_it_unlocks_upon_calling_close_on_body
70  
-    lock = MyLock.new
  47
+    lock = DummyMutex.new
71 48
     Dispatcher.expects(:reload_application)
72  
-    reloader = Reloader.new(lambda { |env|
  49
+    headers, status, body = Reloader.run(lock) do
73 50
       [200, { "Content-Type" => "text/html" }, [""]]
74  
-    }, lock)
75  
-    headers, status, body = reloader.call({ })
  51
+    end
76 52
     body.close
77 53
     assert !lock.locked?
78 54
   end
79 55
 
80 56
   def test_it_unlocks_if_app_object_raises_exception
81  
-    lock = MyLock.new
  57
+    lock = DummyMutex.new
82 58
     Dispatcher.expects(:reload_application)
83  
-    reloader = Reloader.new(lambda { |env|
84  
-      raise "oh no!"
85  
-    }, lock)
86 59
     assert_raise(RuntimeError) do
87  
-      reloader.call({ })
  60
+      Reloader.run(lock) do
  61
+        raise "oh no!"
  62
+      end
88 63
     end
89 64
     assert !lock.locked?
90 65
   end
91 66
 
92 67
   def test_returned_body_object_always_responds_to_close
93  
-    body = setup_and_return_body(lambda { |env|
  68
+    status, headers, body = Reloader.run(@lock) do
94 69
       [200, { "Content-Type" => "text/html" }, [""]]
95  
-    })
  70
+    end
96 71
     assert body.respond_to?(:close)
97 72
   end
98 73
 
99 74
   def test_returned_body_object_behaves_like_underlying_object
100  
-    body = setup_and_return_body(lambda { |env|
  75
+    status, headers, body = Reloader.run(@lock) do
101 76
       b = MyBody.new
102 77
       b << "hello"
103 78
       b << "world"
104 79
       [200, { "Content-Type" => "text/html" }, b]
105  
-    })
  80
+    end
106 81
     assert_equal 2, body.size
107 82
     assert_equal "hello", body[0]
108 83
     assert_equal "world", body[1]
@@ -112,20 +87,20 @@ def test_returned_body_object_behaves_like_underlying_object
112 87
 
113 88
   def test_it_calls_close_on_underlying_object_when_close_is_called_on_body
114 89
     close_called = false
115  
-    body = setup_and_return_body(lambda { |env|
  90
+    status, headers, body = Reloader.run(@lock) do
116 91
       b = MyBody.new do
117 92
         close_called = true
118 93
       end
119 94
       [200, { "Content-Type" => "text/html" }, b]
120  
-    })
  95
+    end
121 96
     body.close
122 97
     assert close_called
123 98
   end
124 99
 
125 100
   def test_returned_body_object_responds_to_all_methods_supported_by_underlying_object
126  
-    body = setup_and_return_body(lambda { |env|
  101
+    status, headers, body = Reloader.run(@lock) do
127 102
       [200, { "Content-Type" => "text/html" }, MyBody.new]
128  
-    })
  103
+    end
129 104
     assert body.respond_to?(:size)
130 105
     assert body.respond_to?(:each)
131 106
     assert body.respond_to?(:foo)
@@ -134,16 +109,16 @@ def test_returned_body_object_responds_to_all_methods_supported_by_underlying_ob
134 109
 
135 110
   def test_it_doesnt_clean_up_the_application_after_call
136 111
     Dispatcher.expects(:cleanup_application).never
137  
-    body = setup_and_return_body(lambda { |env|
  112
+    status, headers, body = Reloader.run(@lock) do
138 113
       [200, { "Content-Type" => "text/html" }, MyBody.new]
139  
-    })
  114
+    end
140 115
   end
141 116
 
142 117
   def test_it_cleans_up_the_application_when_close_is_called_on_body
143 118
     Dispatcher.expects(:cleanup_application)
144  
-    body = setup_and_return_body(lambda { |env|
  119
+    status, headers, body = Reloader.run(@lock) do
145 120
       [200, { "Content-Type" => "text/html" }, MyBody.new]
146  
-    })
  121
+    end
147 122
     body.close
148 123
   end
149 124
 end

0 notes on commit 14b6ab0

Please sign in to comment.
Something went wrong with that request. Please try again.