From b92dc5b01cfbbb44d6625e933e3a2167f61d077e Mon Sep 17 00:00:00 2001 From: eileencodes Date: Tue, 10 Nov 2020 12:12:38 -0500 Subject: [PATCH] Fix preventing_writes for granular swapping When we implemented granular connection swapping we treated `prevent_writes` similarly to `role` and `shard`. However, it's a little different. `prevent_writes` is a feature of a conneciton whereas `role` and `shared` are used to lookup a connection. A writing connection that wants to prevent writes doesn't get stored in the pool as such, `prevent_writes` is added as a way to change behavior of a connection. Because of this when `preventing_writes?` calls `ActiveRecord::Base.current_preventing_writes` the lookup is on the wrong class if we're using granular swapping. Instead we need to lookup the `prevent_writes` from the class that called `connected_to` rather than `ActiveRecord::Base`. To do that I've done the following: 1) Added access to the `connection_specification_name` as `owner_name` on the pool so the connection can access it. 2) Call `safe_constantize` on the `owner_name` since the `connected_to_stack` stores the class as a class and not a string. 3) Now we can call `safe_constantize` on the klass that we got out of the pool to be able to look up the correct `prevent_writes` from the stack. 4) NullPool's get a nil connection specification name because if they're not connected they can't prevent writes. This required changing some tests but we don't support multi-db for a case where you're establishing a connection directly from the adapter. I benchmarked this and the two versions were pretty close, although this is a little bit slower. I tried a few other ways of fixing this. I first tried adding `prevent_writes` to the connection in `retrieve_connection` based on the `current_preventing_writes` but then that behavior only worked correctly for ActiveRecord connections. If we already had a connection and `retrieve_connection` isn't called then the tests testing that behavior would fail. This was the best way to not introduce confusing differences between global and granular connections. Fixes #40559 --- .../abstract/connection_pool.rb | 7 +- .../connection_adapters/abstract_adapter.rb | 24 +++++-- .../sqlite3_adapter_prevent_writes_test.rb | 8 +-- .../connection_swapping_nested_test.rb | 65 +++++++++++++++++++ 4 files changed, 90 insertions(+), 14 deletions(-) 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 da48ddc33ee33..d6f9e74eae547 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb @@ -23,6 +23,10 @@ class NullPool # :nodoc: include ConnectionAdapters::AbstractPool attr_accessor :schema_cache + + def owner_name + nil + end end # Connection pool base class for managing Active Record database @@ -356,7 +360,7 @@ def run include ConnectionAdapters::AbstractPool attr_accessor :automatic_reconnect, :checkout_timeout - attr_reader :db_config, :size, :reaper, :pool_config + attr_reader :db_config, :size, :reaper, :pool_config, :owner_name delegate :schema_cache, :schema_cache=, to: :pool_config @@ -371,6 +375,7 @@ def initialize(pool_config) @pool_config = pool_config @db_config = pool_config.db_config + @owner_name = pool_config.connection_specification_name @checkout_timeout = db_config.checkout_timeout @idle_timeout = db_config.idle_timeout diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb index 70b4fbd2667ab..a537953c1a8a9 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb @@ -113,14 +113,20 @@ def use_metadata_table? # Determines whether writes are currently being prevents. # - # Returns true if the connection is a replica, or if +prevent_writes+ - # is set to true. + # Returns true if the connection is a replica. + # + # If the application is using legacy handling, returns + # true if `connection_handler.prevent_writes` is set. + # + # If the application is using the new connection handling + # will return true based on `current_preventing_writes`. def preventing_writes? - if ActiveRecord::Base.legacy_connection_handling - replica? || ActiveRecord::Base.connection_handler.prevent_writes - else - replica? || ActiveRecord::Base.current_preventing_writes - end + return true if replica? + return ActiveRecord::Base.connection_handler.prevent_writes if ActiveRecord::Base.legacy_connection_handling + return false if owner_name.nil? + + klass = self.owner_name.safe_constantize + klass&.current_preventing_writes end def migrations_paths # :nodoc: @@ -196,6 +202,10 @@ def lease @owner = Thread.current end + def owner_name # :nodoc: + @pool.owner_name + end + def schema_cache @pool.get_schema_cache(self) end diff --git a/activerecord/test/cases/adapters/sqlite3/sqlite3_adapter_prevent_writes_test.rb b/activerecord/test/cases/adapters/sqlite3/sqlite3_adapter_prevent_writes_test.rb index 53727d9efc139..746bfb7285aca 100644 --- a/activerecord/test/cases/adapters/sqlite3/sqlite3_adapter_prevent_writes_test.rb +++ b/activerecord/test/cases/adapters/sqlite3/sqlite3_adapter_prevent_writes_test.rb @@ -11,9 +11,7 @@ class SQLite3AdapterPreventWritesTest < ActiveRecord::SQLite3TestCase self.use_transactional_tests = false def setup - @conn = Base.sqlite3_connection database: ":memory:", - adapter: "sqlite3", - timeout: 100 + @conn = ActiveRecord::Base.connection end def test_errors_when_an_insert_query_is_called_while_preventing_writes @@ -100,9 +98,7 @@ def setup @old_value = ActiveRecord::Base.legacy_connection_handling ActiveRecord::Base.legacy_connection_handling = true - @conn = Base.sqlite3_connection database: ":memory:", - adapter: "sqlite3", - timeout: 100 + @conn = ActiveRecord::Base.connection @connection_handler = ActiveRecord::Base.connection_handler end diff --git a/activerecord/test/cases/connection_adapters/connection_swapping_nested_test.rb b/activerecord/test/cases/connection_adapters/connection_swapping_nested_test.rb index d53026adb5b02..e080c01c7edba 100644 --- a/activerecord/test/cases/connection_adapters/connection_swapping_nested_test.rb +++ b/activerecord/test/cases/connection_adapters/connection_swapping_nested_test.rb @@ -349,6 +349,71 @@ def test_connected_to_many ActiveRecord::Base.establish_connection(:arunit) ENV["RAILS_ENV"] = previous_env end + + def test_prevent_writes_can_be_changed_granularly + previous_env, ENV["RAILS_ENV"] = ENV["RAILS_ENV"], "default_env" + + # replica: true is purposefully left out so we can test the pools behavior + config = { + "default_env" => { + "primary" => { "adapter" => "sqlite3", "database" => "test/db/primary.sqlite3" }, + "primary_replica" => { "adapter" => "sqlite3", "database" => "test/db/primary.sqlite3" }, + "secondary" => { "adapter" => "sqlite3", "database" => "test/db/secondary.sqlite3" }, + "secondary_replica" => { "adapter" => "sqlite3", "database" => "test/db/secondary_replica.sqlite3" } + } + } + + @prev_configs, ActiveRecord::Base.configurations = ActiveRecord::Base.configurations, config + + PrimaryBase.connects_to database: { writing: :primary, reading: :primary_replica } + SecondaryBase.connects_to database: { writing: :secondary, reading: :secondary_replica } + + # Switch everything to writing + ActiveRecord::Base.connected_to(role: :writing) do + assert_not_predicate ActiveRecord::Base.connection, :preventing_writes? + assert_not_predicate PrimaryBase.connection, :preventing_writes? + assert_not_predicate SecondaryBase.connection, :preventing_writes? + + # Switch only primary to reading + PrimaryBase.connected_to(role: :reading) do + assert_predicate PrimaryBase.connection, :preventing_writes? + assert_not_predicate SecondaryBase.connection, :preventing_writes? + + # Switch global to reading + ActiveRecord::Base.connected_to(role: :reading) do + assert_predicate PrimaryBase.connection, :preventing_writes? + assert_predicate SecondaryBase.connection, :preventing_writes? + + # Switch only secondary to writing + SecondaryBase.connected_to(role: :writing) do + assert_predicate PrimaryBase.connection, :preventing_writes? + assert_not_predicate SecondaryBase.connection, :preventing_writes? + end + + # Ensure restored to global reading + assert_predicate PrimaryBase.connection, :preventing_writes? + assert_predicate SecondaryBase.connection, :preventing_writes? + end + + # Switch everything to writing + ActiveRecord::Base.connected_to(role: :writing) do + assert_not_predicate PrimaryBase.connection, :preventing_writes? + assert_not_predicate SecondaryBase.connection, :preventing_writes? + end + + assert_predicate PrimaryBase.connection, :preventing_writes? + assert_not_predicate SecondaryBase.connection, :preventing_writes? + end + + # Ensure restored to global writing + assert_not_predicate PrimaryBase.connection, :preventing_writes? + assert_not_predicate SecondaryBase.connection, :preventing_writes? + end + ensure + ActiveRecord::Base.configurations = @prev_configs + ActiveRecord::Base.establish_connection(:arunit) + ENV["RAILS_ENV"] = previous_env + end end end end