From eb760da85898017a1ea300a8f08c32d0412879d3 Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Thu, 8 Feb 2024 15:10:16 +0100 Subject: [PATCH] Merge pull request #51012 from Shopify/fix-thread-safety-in-mysql-adapters Properly synchronize `Mysql2Adapter#active?` and `TrilogyAdapter#active?` --- activerecord/CHANGELOG.md | 11 +++++ .../connection_adapters/abstract_adapter.rb | 29 +++++++------ .../connection_adapters/mysql2_adapter.rb | 26 +++++++----- .../connection_adapters/trilogy_adapter.rb | 32 ++++++-------- activerecord/test/cases/adapter_test.rb | 42 +++++++++++++++++++ .../adapters/trilogy/trilogy_adapter_test.rb | 10 ++--- 6 files changed, 100 insertions(+), 50 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 2b418c3907df5..c3c77572a0815 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,14 @@ +* Properly synchronize `Mysql2Adapter#active?` and `TrilogyAdapter#active?` + + As well as `disconnect!` and `verify!`. + + This generally isn't a big problem as connections must not be shared between + threads, but is required when running transactional tests or system tests + and could lead to a SEGV. + + *Jean Boussier* + + * Fix counter caches when the foreign key is composite. If the model holding the counter cache had a composite primary key, diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb index d2392eeb3682a..765903aa36b2a 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb @@ -711,13 +711,14 @@ def reconnect!(restore_transactions: false) end end - # Disconnects from the database if already connected. Otherwise, this # method does nothing. def disconnect! - clear_cache!(new_connection: true) - reset_transaction - @raw_connection_dirty = false + @lock.synchronize do + clear_cache!(new_connection: true) + reset_transaction + @raw_connection_dirty = false + end end # Immediately forget this connection ever existed. Unlike disconnect!, @@ -773,19 +774,17 @@ def requires_reloading? # is no longer active, then this method will reconnect to the database. def verify! unless active? - if @unconfigured_connection - @lock.synchronize do - if @unconfigured_connection - @raw_connection = @unconfigured_connection - @unconfigured_connection = nil - configure_connection - @verified = true - return - end + @lock.synchronize do + if @unconfigured_connection + @raw_connection = @unconfigured_connection + @unconfigured_connection = nil + configure_connection + @verified = true + return end - end - reconnect!(restore_transactions: true) + reconnect!(restore_transactions: true) + end end @verified = true diff --git a/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb b/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb index da59f37af73e7..5c1219ea804b2 100644 --- a/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb @@ -131,7 +131,7 @@ def quote_string(string) #++ def active? - !!@raw_connection&.ping + connected? && @lock.synchronize { @raw_connection&.ping } || false end alias :reset! :reconnect! @@ -139,15 +139,19 @@ def active? # Disconnects from the database if already connected. # Otherwise, this method does nothing. def disconnect! - super - @raw_connection&.close - @raw_connection = nil + @lock.synchronize do + super + @raw_connection&.close + @raw_connection = nil + end end def discard! # :nodoc: - super - @raw_connection&.automatic_close = false - @raw_connection = nil + @lock.synchronize do + super + @raw_connection&.automatic_close = false + @raw_connection = nil + end end private @@ -162,9 +166,11 @@ def connect end def reconnect - @raw_connection&.close - @raw_connection = nil - connect + @lock.synchronize do + @raw_connection&.close + @raw_connection = nil + connect + end end def configure_connection diff --git a/activerecord/lib/active_record/connection_adapters/trilogy_adapter.rb b/activerecord/lib/active_record/connection_adapters/trilogy_adapter.rb index df35a6b4f40c3..28d62d1d5e5ca 100644 --- a/activerecord/lib/active_record/connection_adapters/trilogy_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/trilogy_adapter.rb @@ -140,7 +140,7 @@ def quote_string(string) end def active? - connection&.ping || false + connected? && @lock.synchronize { @raw_connection&.ping } || false rescue ::Trilogy::Error false end @@ -148,18 +148,18 @@ def active? alias reset! reconnect! def disconnect! - super - unless connection.nil? - connection.close - self.connection = nil + @lock.synchronize do + super + @raw_connection&.close + @raw_connection = nil end end def discard! - super - unless connection.nil? - connection.discard! - self.connection = nil + @lock.synchronize do + super + @raw_connection&.discard! + @raw_connection = nil end end @@ -189,23 +189,15 @@ def error_number(exception) exception.error_code if exception.respond_to?(:error_code) end - def connection - @raw_connection - end - - def connection=(conn) - @raw_connection = conn - end - def connect - self.connection = self.class.new_client(@config) + @raw_connection = self.class.new_client(@config) rescue ConnectionNotEstablished => ex raise ex.set_pool(@pool) end def reconnect - connection&.close - self.connection = nil + @raw_connection&.close + @raw_connection = nil connect end diff --git a/activerecord/test/cases/adapter_test.rb b/activerecord/test/cases/adapter_test.rb index bf82d9cb98a25..48880098608bd 100644 --- a/activerecord/test/cases/adapter_test.rb +++ b/activerecord/test/cases/adapter_test.rb @@ -809,6 +809,48 @@ def kill_connection_from_server(connection_id) end end end + + class AdapterThreadSafetyTest < ActiveRecord::TestCase + setup do + @threads = [] + @connection = ActiveRecord::Base.connection_pool.checkout + end + + teardown do + @threads.each(&:kill) + end + + test "#active? is synchronized" do + threads(2, 25) { @connection.select_all("SELECT 1") } + threads(2, 25) { @connection.verify! } + threads(2, 25) { @connection.disconnect! } + + join + end + + test "#verify! is synchronized" do + threads(2, 25) { @connection.verify! } + threads(2, 25) { @connection.disconnect! } + + join + end + + private + def join + @threads.shuffle.each(&:join) + end + + def threads(count, times) + @threads += count.times.map do + Thread.new do + times.times do + yield + Thread.pass + end + end + end + end + end end if ActiveRecord::Base.connection.supports_advisory_locks? diff --git a/activerecord/test/cases/adapters/trilogy/trilogy_adapter_test.rb b/activerecord/test/cases/adapters/trilogy/trilogy_adapter_test.rb index 4035f512a4e24..945196631984b 100644 --- a/activerecord/test/cases/adapters/trilogy/trilogy_adapter_test.rb +++ b/activerecord/test/cases/adapters/trilogy/trilogy_adapter_test.rb @@ -97,24 +97,24 @@ class TrilogyAdapterTest < ActiveRecord::TrilogyTestCase end test "#active? answers false with connection and exception" do - @conn.send(:connection).stub(:ping, -> { raise ::Trilogy::BaseError.new }) do + @conn.instance_variable_get(:@raw_connection).stub(:ping, -> { raise ::Trilogy::BaseError.new }) do assert_equal false, @conn.active? end end test "#reconnect answers new connection with existing connection" do - old_connection = @conn.send(:connection) + old_connection = @conn.instance_variable_get(:@raw_connection) @conn.reconnect! - connection = @conn.send(:connection) + connection = @conn.instance_variable_get(:@raw_connection) assert_instance_of Trilogy, connection assert_not_equal old_connection, connection end test "#reset answers new connection with existing connection" do - old_connection = @conn.send(:connection) + old_connection = @conn.instance_variable_get(:@raw_connection) @conn.reset! - connection = @conn.send(:connection) + connection = @conn.instance_variable_get(:@raw_connection) assert_instance_of Trilogy, connection assert_not_equal old_connection, connection