diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 8a7d13801318d..06b51551238a9 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,13 @@ +* 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* + * Support `:source_location` tag option for query log tags ```ruby diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb index e435f582e516f..af23562dba8b5 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb @@ -718,13 +718,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!, @@ -780,19 +781,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 ce325ac032da7..2a85a24864958 100644 --- a/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb @@ -113,7 +113,7 @@ def connected? end def active? - !!@raw_connection&.ping + connected? && @lock.synchronize { @raw_connection&.ping } || false end alias :reset! :reconnect! @@ -121,15 +121,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 @@ -144,9 +148,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 46d782a3b3a67..3eebc83cdeb12 100644 --- a/activerecord/lib/active_record/connection_adapters/trilogy_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/trilogy_adapter.rb @@ -117,7 +117,7 @@ def connected? end def active? - connection&.ping || false + connected? && @lock.synchronize { @raw_connection&.ping } || false rescue ::Trilogy::Error false end @@ -125,18 +125,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 @@ -166,23 +166,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 a3c30a3508c72..1cda381c2244c 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 ea210c5f613a7..2a73f7b7c8630 100644 --- a/activerecord/test/cases/adapters/trilogy/trilogy_adapter_test.rb +++ b/activerecord/test/cases/adapters/trilogy/trilogy_adapter_test.rb @@ -95,24 +95,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