From 69396b75c91cdbbf141331356ef7c41bbfea1950 Mon Sep 17 00:00:00 2001 From: eileencodes Date: Wed, 7 Sep 2022 14:37:24 -0400 Subject: [PATCH] Make connection_pool_list take an explicit argument Following on #45924 I realized that `all_connection_pools` and `connection_pool_list` don't make much sense as separate methods and should follow the same deprecation as the other methods on the handler here. So this PR deprecates `all_connection_pools` in favor of `connection_pool_list` with an explicit argument of the role or `:all`. Passing `nil` will throw a deprecation warning to get applications to be explicit about behavior they expect. --- activerecord/CHANGELOG.md | 6 ++ .../abstract/connection_handler.rb | 58 ++++++++++--------- .../lib/active_record/connection_handling.rb | 2 +- activerecord/lib/active_record/query_cache.rb | 4 +- .../lib/active_record/test_fixtures.rb | 2 +- .../test/cases/asynchronous_queries_test.rb | 10 ++-- .../connection_handlers_multi_db_test.rb | 16 ++++- ...nection_handlers_multi_pool_config_test.rb | 34 +++++------ activerecord/test/cases/query_cache_test.rb | 4 +- 9 files changed, 78 insertions(+), 58 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 97f22f9db1..4a8c9314db 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,9 @@ +* Deprecate `all_connection_pools` and make `connection_pool_list` more explicit. + + Following on #45924 `all_connection_pools` is now deprecated. `connection_pool_list` will either take an explicit role or applications can opt into the new behavior by passing `:all`. + + *Eileen M. Uchitelle* + * Fix connection handler methods to operate on all pools. `active_connections?`, `clear_active_connections!`, `clear_reloadable_connections!`, `clear_all_connections!`, and `flush_idle_connections!` now operate on all pools by default. Previously they would default to using the `current_role` or `:writing` role unless specified. diff --git a/activerecord/lib/active_record/connection_adapters/abstract/connection_handler.rb b/activerecord/lib/active_record/connection_adapters/abstract/connection_handler.rb index 712a0af3e9..bb27ff48d2 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/connection_handler.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_handler.rb @@ -93,11 +93,25 @@ def connection_pool_names # :nodoc: end def all_connection_pools + ActiveSupport::Deprecation.warn(<<-MSG.squish) + The `all_connection_pools` method is deprecated in favor of `connection_pool_list`. + Call `connection_pool_list(:all)` to get the same behavior as `all_connection_pools`. + MSG connection_name_to_pool_manager.values.flat_map { |m| m.pool_configs.map(&:pool) } end - def connection_pool_list(role = ActiveRecord::Base.current_role) - connection_name_to_pool_manager.values.flat_map { |m| m.pool_configs(role).map(&:pool) } + # Returns the pools for a connection handler and given role. If `:all` is passed, + # all pools belonging to the connection handler will be returned. + def connection_pool_list(role = nil) + if role.nil? + deprecation_for_pool_handling(__method__) + role = ActiveRecord::Base.current_role + connection_name_to_pool_manager.values.flat_map { |m| m.pool_configs(role).map(&:pool) } + elsif role == :all + connection_name_to_pool_manager.values.flat_map { |m| m.pool_configs.map(&:pool) } + else + connection_name_to_pool_manager.values.flat_map { |m| m.pool_configs(role).map(&:pool) } + end end alias :connection_pools :connection_pool_list @@ -145,12 +159,10 @@ def establish_connection(config, owner_name: Base, role: ActiveRecord::Base.curr def active_connections?(role = nil) if role.nil? deprecation_for_pool_handling(__method__) - connection_pool_list(ActiveRecord::Base.current_role).any?(&:active_connection?) - elsif role == :all - all_connection_pools.any?(&:active_connection?) - else - connection_pool_list(role).any?(&:active_connection?) + role = ActiveRecord::Base.current_role end + + connection_pool_list(role).any?(&:active_connection?) end # Returns any connections in use by the current thread back to the pool, @@ -159,12 +171,10 @@ def active_connections?(role = nil) def clear_active_connections!(role = nil) if role.nil? deprecation_for_pool_handling(__method__) - connection_pool_list(ActiveRecord::Base.current_role).each(&:release_connection) - elsif role == :all - all_connection_pools.each(&:release_connection) - else - connection_pool_list(role).each(&:release_connection) + role = ActiveRecord::Base.current_role end + + connection_pool_list(role).each(&:release_connection) end # Clears the cache which maps classes. @@ -173,23 +183,19 @@ def clear_active_connections!(role = nil) def clear_reloadable_connections!(role = nil) if role.nil? deprecation_for_pool_handling(__method__) - connection_pool_list(ActiveRecord::Base.current_role).each(&:clear_reloadable_connections!) - elsif role == :all - all_connection_pools.each(&:clear_reloadable_connections!) - else - connection_pool_list(role).each(&:clear_reloadable_connections!) + role = ActiveRecord::Base.current_role end + + connection_pool_list(role).each(&:clear_reloadable_connections!) end def clear_all_connections!(role = nil) if role.nil? deprecation_for_pool_handling(__method__) - connection_pool_list(ActiveRecord::Base.current_role).each(&:disconnect!) - elsif role == :all - all_connection_pools.each(&:disconnect!) - else - connection_pool_list(role).each(&:disconnect!) + role = ActiveRecord::Base.current_role end + + connection_pool_list(role).each(&:disconnect!) end # Disconnects all currently idle connections. @@ -198,12 +204,10 @@ def clear_all_connections!(role = nil) def flush_idle_connections!(role = nil) if role.nil? deprecation_for_pool_handling(__method__) - connection_pool_list(ActiveRecord::Base.current_role).each(&:flush!) - elsif role == :all - all_connection_pools.each(&:flush!) - else - connection_pool_list(role).each(&:flush!) + role = ActiveRecord::Base.current_role end + + connection_pool_list(role).each(&:flush!) end # Locate the connection of the nearest super class. This can be an diff --git a/activerecord/lib/active_record/connection_handling.rb b/activerecord/lib/active_record/connection_handling.rb index c5fc2f89ff..559a172fd6 100644 --- a/activerecord/lib/active_record/connection_handling.rb +++ b/activerecord/lib/active_record/connection_handling.rb @@ -242,7 +242,7 @@ def connected_to?(role:, shard: ActiveRecord::Base.default_shard) # Clears the query cache for all connections associated with the current thread. def clear_query_caches_for_current_thread - connection_handler.all_connection_pools.each do |pool| + connection_handler.connection_pool_list(:all).each do |pool| pool.connection.clear_query_cache if pool.active_connection? end end diff --git a/activerecord/lib/active_record/query_cache.rb b/activerecord/lib/active_record/query_cache.rb index b825c202c3..42c909519e 100644 --- a/activerecord/lib/active_record/query_cache.rb +++ b/activerecord/lib/active_record/query_cache.rb @@ -27,14 +27,14 @@ def uncached(&block) def self.run pools = [] - pools.concat(ActiveRecord::Base.connection_handler.all_connection_pools.reject { |p| p.query_cache_enabled }.each { |p| p.enable_query_cache! }) + pools.concat(ActiveRecord::Base.connection_handler.connection_pool_list(:all).reject { |p| p.query_cache_enabled }.each { |p| p.enable_query_cache! }) pools end def self.complete(pools) pools.each { |pool| pool.disable_query_cache! } - ActiveRecord::Base.connection_handler.all_connection_pools.each do |pool| + ActiveRecord::Base.connection_handler.connection_pool_list(:all).each do |pool| pool.release_connection if pool.active_connection? && !pool.connection.transaction_open? end end diff --git a/activerecord/lib/active_record/test_fixtures.rb b/activerecord/lib/active_record/test_fixtures.rb index 3b816f0dda..be05030ec7 100644 --- a/activerecord/lib/active_record/test_fixtures.rb +++ b/activerecord/lib/active_record/test_fixtures.rb @@ -168,7 +168,7 @@ def teardown_fixtures def enlist_fixture_connections setup_shared_connection_pool - ActiveRecord::Base.connection_handler.connection_pool_list.map(&:connection) + ActiveRecord::Base.connection_handler.connection_pool_list(:writing).map(&:connection) end private diff --git a/activerecord/test/cases/asynchronous_queries_test.rb b/activerecord/test/cases/asynchronous_queries_test.rb index 8a958afe91..66ac797508 100644 --- a/activerecord/test/cases/asynchronous_queries_test.rb +++ b/activerecord/test/cases/asynchronous_queries_test.rb @@ -158,7 +158,7 @@ def test_null_configuration_uses_a_single_null_executor_by_default assert_nil async_pool1 assert_nil async_pool2 - assert_equal 2, handler.all_connection_pools.count + assert_equal 2, handler.connection_pool_list(:all).count ensure clean_up_connection_handler ActiveRecord.async_query_executor = old_value @@ -190,7 +190,7 @@ def test_one_global_thread_pool_is_used_when_set_with_default_concurrency assert_equal 16, async_pool2.max_queue assert_equal :caller_runs, async_pool2.fallback_policy - assert_equal 2, handler.all_connection_pools.count + assert_equal 2, handler.connection_pool_list(:all).count assert_equal async_pool1, async_pool2 ensure clean_up_connection_handler @@ -227,7 +227,7 @@ def test_concurrency_can_be_set_on_global_thread_pool assert_equal 32, async_pool2.max_queue assert_equal :caller_runs, async_pool2.fallback_policy - assert_equal 2, handler.all_connection_pools.count + assert_equal 2, handler.connection_pool_list(:all).count assert_equal async_pool1, async_pool2 ensure clean_up_connection_handler @@ -281,7 +281,7 @@ def test_multi_thread_pool_executor_configuration assert_equal 20, async_pool2.max_queue assert_equal :caller_runs, async_pool2.fallback_policy - assert_equal 2, handler.all_connection_pools.count + assert_equal 2, handler.connection_pool_list(:all).count assert_not_equal async_pool1, async_pool2 ensure clean_up_connection_handler @@ -316,7 +316,7 @@ def test_multi_thread_pool_is_used_only_by_configurations_that_enable_it assert_equal 40, async_pool1.max_queue assert_equal :caller_runs, async_pool1.fallback_policy - assert_equal 2, handler.all_connection_pools.count + assert_equal 2, handler.connection_pool_list(:all).count assert_not_equal async_pool1, async_pool2 ensure clean_up_connection_handler diff --git a/activerecord/test/cases/connection_adapters/connection_handlers_multi_db_test.rb b/activerecord/test/cases/connection_adapters/connection_handlers_multi_db_test.rb index ac7e16d13f..45cbb9f427 100644 --- a/activerecord/test/cases/connection_adapters/connection_handlers_multi_db_test.rb +++ b/activerecord/test/cases/connection_adapters/connection_handlers_multi_db_test.rb @@ -319,9 +319,19 @@ def test_connects_to_returns_array_of_established_connections end end - def test_connection_pools - assert_equal([@rw_pool], @handler.connection_pools(:writing)) - assert_equal([@ro_pool], @handler.connection_pools(:reading)) + def test_connection_pool_list + assert_equal([@rw_pool], @handler.connection_pool_list(:writing)) + assert_equal([@ro_pool], @handler.connection_pool_list(:reading)) + + assert_deprecated do + @handler.connection_pool_list + end + end + + def test_all_connection_pools + assert_deprecated do + assert_equal([@rw_pool, @ro_pool], @handler.all_connection_pools) + end end def test_retrieve_connection diff --git a/activerecord/test/cases/connection_adapters/connection_handlers_multi_pool_config_test.rb b/activerecord/test/cases/connection_adapters/connection_handlers_multi_pool_config_test.rb index 5103796754..54c2abb207 100644 --- a/activerecord/test/cases/connection_adapters/connection_handlers_multi_pool_config_test.rb +++ b/activerecord/test/cases/connection_adapters/connection_handlers_multi_pool_config_test.rb @@ -11,7 +11,7 @@ class ConnectionHandlersMultiPoolConfigTest < ActiveRecord::TestCase fixtures :people def setup - @writing_handler = ConnectionHandler.new + @handler = ConnectionHandler.new end def teardown @@ -30,17 +30,17 @@ def test_establish_connection_with_pool_configs @prev_configs, ActiveRecord::Base.configurations = ActiveRecord::Base.configurations, config - @writing_handler.establish_connection(:primary) - @writing_handler.establish_connection(:primary, shard: :pool_config_two) + @handler.establish_connection(:primary) + @handler.establish_connection(:primary, shard: :pool_config_two) - default_pool = @writing_handler.retrieve_connection_pool("primary", shard: :default) - other_pool = @writing_handler.retrieve_connection_pool("primary", shard: :pool_config_two) + default_pool = @handler.retrieve_connection_pool("primary", shard: :default) + other_pool = @handler.retrieve_connection_pool("primary", shard: :pool_config_two) assert_not_nil default_pool assert_not_equal default_pool, other_pool # :default if passed with no key - assert_equal default_pool, @writing_handler.retrieve_connection_pool("primary") + assert_equal default_pool, @handler.retrieve_connection_pool("primary") ensure ActiveRecord::Base.configurations = @prev_configs ActiveRecord::Base.establish_connection(:arunit) @@ -58,14 +58,14 @@ def test_remove_connection @prev_configs, ActiveRecord::Base.configurations = ActiveRecord::Base.configurations, config - @writing_handler.establish_connection(:primary) - @writing_handler.establish_connection(:primary, shard: :pool_config_two) + @handler.establish_connection(:primary) + @handler.establish_connection(:primary, shard: :pool_config_two) # remove default - @writing_handler.remove_connection_pool("primary") + @handler.remove_connection_pool("primary") - assert_nil @writing_handler.retrieve_connection_pool("primary") - assert_not_nil @writing_handler.retrieve_connection_pool("primary", shard: :pool_config_two) + assert_nil @handler.retrieve_connection_pool("primary") + assert_not_nil @handler.retrieve_connection_pool("primary", shard: :pool_config_two) ensure ActiveRecord::Base.configurations = @prev_configs ActiveRecord::Base.establish_connection(:arunit) @@ -83,15 +83,15 @@ def test_connected? @prev_configs, ActiveRecord::Base.configurations = ActiveRecord::Base.configurations, config - @writing_handler.establish_connection(:primary) - @writing_handler.establish_connection(:primary, shard: :pool_config_two) + @handler.establish_connection(:primary) + @handler.establish_connection(:primary, shard: :pool_config_two) # connect to default - @writing_handler.connection_pool_list.first.checkout + @handler.connection_pool_list(:writing).first.checkout - assert @writing_handler.connected?("primary") - assert @writing_handler.connected?("primary", shard: :default) - assert_not @writing_handler.connected?("primary", shard: :pool_config_two) + assert @handler.connected?("primary") + assert @handler.connected?("primary", shard: :default) + assert_not @handler.connected?("primary", shard: :pool_config_two) ensure ActiveRecord::Base.configurations = @prev_configs ActiveRecord::Base.establish_connection(:arunit) diff --git a/activerecord/test/cases/query_cache_test.rb b/activerecord/test/cases/query_cache_test.rb index 4c02c07f43..9758fda750 100644 --- a/activerecord/test/cases/query_cache_test.rb +++ b/activerecord/test/cases/query_cache_test.rb @@ -81,7 +81,7 @@ def test_query_cache_is_applied_to_all_connections end mw = middleware { |env| - ActiveRecord::Base.connection_handler.all_connection_pools.each do |pool| + ActiveRecord::Base.connection_handler.connection_pool_list(:all).each do |pool| assert_predicate pool.connection, :query_cache_enabled end } @@ -558,7 +558,7 @@ def test_query_caching_is_local_to_the_current_thread def test_query_cache_is_enabled_on_all_connection_pools middleware { - ActiveRecord::Base.connection_handler.all_connection_pools.each do |pool| + ActiveRecord::Base.connection_handler.connection_pool_list(:all).each do |pool| assert pool.query_cache_enabled assert pool.connection.query_cache_enabled end