Merge pull request #23807 from matthewd/executor

Publish AS::Executor and AS::Reloader APIs

7 files changed, 72 insertions, 129 deletions

diff --git a/activerecord/lib/active_record.rb b/activerecord/lib/active_record.rb
index ab3846ae65..baa497dc98 100644
--- a/activerecord/lib/active_record.rb
+++ b/activerecord/lib/active_record.rb
@@ -137,7 +137,6 @@ module ActiveRecord
 
     eager_autoload do
       autoload :AbstractAdapter
-      autoload :ConnectionManagement, "active_record/connection_adapters/abstract/connection_pool"
     end
   end
 
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb
index ccd2899489..e389d818fd 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb
@@ -951,24 +951,5 @@ module ActiveRecord
         owner_to_pool && owner_to_pool[owner.name]
       end
     end
-
-    class ConnectionManagement
-      def initialize(app)
-        @app = app
-      end
-
-      def call(env)
-        testing = env['rack.test']
-
-        status, headers, body = @app.call(env)
-        proxy = ::Rack::BodyProxy.new(body) do
-          ActiveRecord::Base.clear_active_connections! unless testing
-        end
-        [status, headers, proxy]
-      rescue Exception
-        ActiveRecord::Base.clear_active_connections! unless testing
-        raise
-      end
-    end
   end
 end
diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
index 5ef434734a..fcc1ef9d5f 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
@@ -27,7 +27,6 @@ module ActiveRecord
 
     autoload_at 'active_record/connection_adapters/abstract/connection_pool' do
       autoload :ConnectionHandler
-      autoload :ConnectionManagement
     end
 
     autoload_under 'abstract' do
diff --git a/activerecord/lib/active_record/query_cache.rb b/activerecord/lib/active_record/query_cache.rb
index dcb2bd3d84..f451ed1764 100644
--- a/activerecord/lib/active_record/query_cache.rb
+++ b/activerecord/lib/active_record/query_cache.rb
@@ -23,34 +23,26 @@ module ActiveRecord
       end
     end
 
-    def initialize(app)
-      @app = app
-    end
-
-    def call(env)
-      connection    = ActiveRecord::Base.connection
-      enabled       = connection.query_cache_enabled
-      connection_id = ActiveRecord::Base.connection_id
-      connection.enable_query_cache!
-
-      response = @app.call(env)
-      response[2] = Rack::BodyProxy.new(response[2]) do
-        restore_query_cache_settings(connection_id, enabled)
+    def self.install_executor_hooks(executor = ActiveSupport::Executor)
+      executor.to_run do
+        connection    = ActiveRecord::Base.connection
+        enabled       = connection.query_cache_enabled
+        connection_id = ActiveRecord::Base.connection_id
+        connection.enable_query_cache!
+
+        @restore_query_cache_settings = lambda do
+          ActiveRecord::Base.connection_id = connection_id
+          ActiveRecord::Base.connection.clear_query_cache
+          ActiveRecord::Base.connection.disable_query_cache! unless enabled
+        end
       end
 
-      response
-    rescue Exception => e
-      restore_query_cache_settings(connection_id, enabled)
-      raise e
-    end
-
-    private
+      executor.to_complete do
+        @restore_query_cache_settings.call if defined?(@restore_query_cache_settings)
 
-    def restore_query_cache_settings(connection_id, enabled)
-      ActiveRecord::Base.connection_id = connection_id
-      ActiveRecord::Base.connection.clear_query_cache
-      ActiveRecord::Base.connection.disable_query_cache! unless enabled
+        # FIXME: This should be skipped when env['rack.test']
+        ActiveRecord::Base.clear_active_connections!
+      end
     end
-
   end
 end
diff --git a/activerecord/lib/active_record/railtie.rb b/activerecord/lib/active_record/railtie.rb
index f4200e96b7..4c074c93ed 100644
--- a/activerecord/lib/active_record/railtie.rb
+++ b/activerecord/lib/active_record/railtie.rb
@@ -16,12 +16,6 @@ module ActiveRecord
     config.app_generators.orm :active_record, :migration => true,
                                               :timestamps => true
 
-    config.app_middleware.insert_after ::ActionDispatch::Callbacks,
-      ActiveRecord::QueryCache
-
-    config.app_middleware.insert_after ::ActionDispatch::Callbacks,
-      ActiveRecord::ConnectionAdapters::ConnectionManagement
-
     config.action_dispatch.rescue_responses.merge!(
       'ActiveRecord::RecordNotFound'   => :not_found,
       'ActiveRecord::StaleObjectError' => :conflict,
@@ -153,11 +147,9 @@ end_warning
       end
     end
 
-    initializer "active_record.set_reloader_hooks" do |app|
-      hook = app.config.reload_classes_only_on_change ? :to_prepare : :to_cleanup
-
+    initializer "active_record.set_reloader_hooks" do
       ActiveSupport.on_load(:active_record) do
-        ActionDispatch::Reloader.send(hook) do
+        ActiveSupport::Reloader.before_class_unload do
           if ActiveRecord::Base.connected?
             ActiveRecord::Base.clear_cache!
             ActiveRecord::Base.clear_reloadable_connections!
@@ -166,6 +158,12 @@ end_warning
       end
     end
 
+    initializer "active_record.set_executor_hooks" do
+      ActiveSupport.on_load(:active_record) do
+        ActiveRecord::QueryCache.install_executor_hooks
+      end
+    end
+
     initializer "active_record.add_watchable_files" do |app|
       path = app.paths["db"].first
       config.watchable_files.concat ["#{path}/schema.rb", "#{path}/structure.sql"]
diff --git a/activerecord/test/cases/connection_management_test.rb b/activerecord/test/cases/connection_management_test.rb
index d43668e57c..c4c2c69d1c 100644
--- a/activerecord/test/cases/connection_management_test.rb
+++ b/activerecord/test/cases/connection_management_test.rb
@@ -19,7 +19,7 @@ module ActiveRecord
       def setup
         @env = {}
         @app = App.new
-        @management = ConnectionManagement.new(@app)
+        @management = middleware(@app)
 
         # make sure we have an active connection
         assert ActiveRecord::Base.connection
@@ -27,17 +27,12 @@ module ActiveRecord
       end
 
       def test_app_delegation
-        manager = ConnectionManagement.new(@app)
+        manager = middleware(@app)
 
         manager.call @env
         assert_equal [@env], @app.calls
       end
 
-      def test_connections_are_active_after_call
-        @management.call(@env)
-        assert ActiveRecord::Base.connection_handler.active_connections?
-      end
-
       def test_body_responds_to_each
         _, _, body = @management.call(@env)
         bits = []
@@ -52,45 +47,40 @@ module ActiveRecord
       end
 
       def test_active_connections_are_not_cleared_on_body_close_during_test
-        @env['rack.test'] = true
-        _, _, body = @management.call(@env)
-        body.close
-        assert ActiveRecord::Base.connection_handler.active_connections?
+        executor.wrap do
+          _, _, body = @management.call(@env)
+          body.close
+          assert ActiveRecord::Base.connection_handler.active_connections?
+        end
       end
 
       def test_connections_closed_if_exception
         app       = Class.new(App) { def call(env); raise NotImplementedError; end }.new
-        explosive = ConnectionManagement.new(app)
+        explosive = middleware(app)
         assert_raises(NotImplementedError) { explosive.call(@env) }
         assert !ActiveRecord::Base.connection_handler.active_connections?
       end
 
       def test_connections_not_closed_if_exception_and_test
-        @env['rack.test'] = true
-        app               = Class.new(App) { def call(env); raise; end }.new
-        explosive         = ConnectionManagement.new(app)
-        assert_raises(RuntimeError) { explosive.call(@env) }
-        assert ActiveRecord::Base.connection_handler.active_connections?
-      end
-
-      def test_connections_closed_if_exception_and_explicitly_not_test
-        @env['rack.test'] = false
-        app       = Class.new(App) { def call(env); raise NotImplementedError; end }.new
-        explosive = ConnectionManagement.new(app)
-        assert_raises(NotImplementedError) { explosive.call(@env) }
-        assert !ActiveRecord::Base.connection_handler.active_connections?
+        executor.wrap do
+          app               = Class.new(App) { def call(env); raise; end }.new
+          explosive         = middleware(app)
+          assert_raises(RuntimeError) { explosive.call(@env) }
+          assert ActiveRecord::Base.connection_handler.active_connections?
+        end
       end
 
       test "doesn't clear active connections when running in a test case" do
-        @env['rack.test'] = true
-        @management.call(@env)
-        assert ActiveRecord::Base.connection_handler.active_connections?
+        executor.wrap do
+          @management.call(@env)
+          assert ActiveRecord::Base.connection_handler.active_connections?
+        end
       end
 
       test "proxy is polite to its body and responds to it" do
         body = Class.new(String) { def to_path; "/path"; end }.new
         app = lambda { |_| [200, {}, body] }
-        response_body = ConnectionManagement.new(app).call(@env)[2]
+        response_body = middleware(app).call(@env)[2]
         assert response_body.respond_to?(:to_path)
         assert_equal "/path", response_body.to_path
       end
@@ -98,9 +88,23 @@ module ActiveRecord
       test "doesn't mutate the original response" do
         original_response = [200, {}, 'hi']
         app = lambda { |_| original_response }
-        ConnectionManagement.new(app).call(@env)[2]
+        middleware(app).call(@env)[2]
         assert_equal 'hi', original_response.last
       end
+
+      private
+        def executor
+          @executor ||= Class.new(ActiveSupport::Executor).tap do |exe|
+            ActiveRecord::QueryCache.install_executor_hooks(exe)
+          end
+        end
+
+        def middleware(app)
+          lambda do |env|
+            a, b, c = executor.wrap { app.call(env) }
+            [a, b, Rack::BodyProxy.new(c) { }]
+          end
+        end
     end
   end
 end
diff --git a/activerecord/test/cases/query_cache_test.rb b/activerecord/test/cases/query_cache_test.rb
index d84653e4c9..e53239cdee 100644
--- a/activerecord/test/cases/query_cache_test.rb
+++ b/activerecord/test/cases/query_cache_test.rb
@@ -16,7 +16,7 @@ class QueryCacheTest < ActiveRecord::TestCase
   def test_exceptional_middleware_clears_and_disables_cache_on_error
     assert !ActiveRecord::Base.connection.query_cache_enabled, 'cache off'
 
-    mw = ActiveRecord::QueryCache.new lambda { |env|
+    mw = middleware { |env|
       Task.find 1
       Task.find 1
       assert_equal 1, ActiveRecord::Base.connection.query_cache.length
@@ -31,7 +31,7 @@ class QueryCacheTest < ActiveRecord::TestCase
   def test_exceptional_middleware_leaves_enabled_cache_alone
     ActiveRecord::Base.connection.enable_query_cache!
 
-    mw = ActiveRecord::QueryCache.new lambda { |env|
+    mw = middleware { |env|
       raise "lol borked"
     }
     assert_raises(RuntimeError) { mw.call({}) }
@@ -42,7 +42,7 @@ class QueryCacheTest < ActiveRecord::TestCase
   def test_exceptional_middleware_assigns_original_connection_id_on_error
     connection_id = ActiveRecord::Base.connection_id
 
-    mw = ActiveRecord::QueryCache.new lambda { |env|
+    mw = middleware { |env|
       ActiveRecord::Base.connection_id = self.object_id
       raise "lol borked"
     }
@@ -53,7 +53,7 @@ class QueryCacheTest < ActiveRecord::TestCase
 
   def test_middleware_delegates
     called = false
-    mw = ActiveRecord::QueryCache.new lambda { |env|
+    mw = middleware { |env|
       called = true
       [200, {}, nil]
     }
@@ -62,7 +62,7 @@ class QueryCacheTest < ActiveRecord::TestCase
   end
 
   def test_middleware_caches
-    mw = ActiveRecord::QueryCache.new lambda { |env|
+    mw = middleware { |env|
       Task.find 1
       Task.find 1
       assert_equal 1, ActiveRecord::Base.connection.query_cache.length
@@ -74,50 +74,13 @@ class QueryCacheTest < ActiveRecord::TestCase
   def test_cache_enabled_during_call
     assert !ActiveRecord::Base.connection.query_cache_enabled, 'cache off'
 
-    mw = ActiveRecord::QueryCache.new lambda { |env|
+    mw = middleware { |env|
       assert ActiveRecord::Base.connection.query_cache_enabled, 'cache on'
       [200, {}, nil]
     }
     mw.call({})
   end
 
-  def test_cache_on_during_body_write
-    streaming = Class.new do
-      def each
-        yield ActiveRecord::Base.connection.query_cache_enabled
-      end
-    end
-
-    mw = ActiveRecord::QueryCache.new lambda { |env|
-      [200, {}, streaming.new]
-    }
-    body = mw.call({}).last
-    body.each { |x| assert x, 'cache should be on' }
-    body.close
-    assert !ActiveRecord::Base.connection.query_cache_enabled, 'cache disabled'
-  end
-
-  def test_cache_off_after_close
-    mw = ActiveRecord::QueryCache.new lambda { |env| [200, {}, nil] }
-    body = mw.call({}).last
-
-    assert ActiveRecord::Base.connection.query_cache_enabled, 'cache enabled'
-    body.close
-    assert !ActiveRecord::Base.connection.query_cache_enabled, 'cache disabled'
-  end
-
-  def test_cache_clear_after_close
-    mw = ActiveRecord::QueryCache.new lambda { |env|
-      Post.first
-      [200, {}, nil]
-    }
-    body = mw.call({}).last
-
-    assert !ActiveRecord::Base.connection.query_cache.empty?, 'cache not empty'
-    body.close
-    assert ActiveRecord::Base.connection.query_cache.empty?, 'cache should be empty'
-  end
-
   def test_cache_passing_a_relation
     post = Post.first
     Post.cache do
@@ -244,6 +207,13 @@ class QueryCacheTest < ActiveRecord::TestCase
       assert_equal 0, Post.where(title: 'rollback').to_a.count
     end
   end
+
+  private
+    def middleware(&app)
+      executor = Class.new(ActiveSupport::Executor)
+      ActiveRecord::QueryCache.install_executor_hooks executor
+      lambda { |env| executor.wrap { app.call(env) } }
+    end
 end
 
 class QueryCacheExpiryTest < ActiveRecord::TestCase