Merge pull request rails#51012 from Shopify/fix-thread-safety-in-mysq…

…l-adapters

Properly synchronize `Mysql2Adapter#active?` and `TrilogyAdapter#active?`

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,

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

activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb

@@ -131,23 +131,27 @@ def quote_string(string)
       #++
 
       def active?
-        !!@raw_connection&.ping
+        connected? && @lock.synchronize { @raw_connection&.ping } || false
       end
 
       alias :reset! :reconnect!
 
       # 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

activerecord/lib/active_record/connection_adapters/trilogy_adapter.rb

@@ -140,26 +140,26 @@ def quote_string(string)
       end
 
       def active?
-        connection&.ping || false
+        connected? && @lock.synchronize { @raw_connection&.ping } || false
       rescue ::Trilogy::Error
         false
       end
 
       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
 

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?

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