Avoid race condition with unprepared statement

In system tests, a single database connection is shared among all
the server threads. A call to unprepared_statement temporarily
modifies an instance variable on the connection object, which is
then visible to other concurrently running threads. This leads to
a situation where prepared statements may end up with the wrong
binds.

Addresses #36763

activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb

@@ -15,28 +15,30 @@ def to_sql(arel_or_sql_string, binds = [])
       end
 
       def to_sql_and_binds(arel_or_sql_string, binds = []) # :nodoc:
-        if arel_or_sql_string.respond_to?(:ast)
-          unless binds.empty?
-            raise "Passing bind parameters with an arel AST is forbidden. " \
-              "The values must be stored on the AST directly"
-          end
+        @lock.synchronize do
+          if arel_or_sql_string.respond_to?(:ast)
+            unless binds.empty?
+              raise "Passing bind parameters with an arel AST is forbidden. " \
+                "The values must be stored on the AST directly"
+            end
 
-          if prepared_statements
-            sql, binds = visitor.compile(arel_or_sql_string.ast, collector)
+            if prepared_statements
+              sql, binds = visitor.compile(arel_or_sql_string.ast, collector)
 
-            if binds.length > bind_params_length
-              unprepared_statement do
-                sql, binds = to_sql_and_binds(arel_or_sql_string)
-                visitor.preparable = false
+              if binds.length > bind_params_length
+                unprepared_statement do
+                  sql, binds = to_sql_and_binds(arel_or_sql_string)
+                  visitor.preparable = false
+                end
               end
+            else
+              sql = visitor.compile(arel_or_sql_string.ast, collector)
             end
+            [sql.freeze, binds]
           else
-            sql = visitor.compile(arel_or_sql_string.ast, collector)
+            visitor.preparable = false if prepared_statements
+            [arel_or_sql_string.dup.freeze, binds]
           end
-          [sql.freeze, binds]
-        else
-          visitor.preparable = false if prepared_statements
-          [arel_or_sql_string.dup.freeze, binds]
         end
       end
       private :to_sql_and_binds
@@ -57,17 +59,19 @@ def cacheable_query(klass, arel) # :nodoc:
 
       # Returns an ActiveRecord::Result instance.
       def select_all(arel, name = nil, binds = [], preparable: nil)
-        arel = arel_from_relation(arel)
-        sql, binds = to_sql_and_binds(arel, binds)
+        @lock.synchronize do
+          arel = arel_from_relation(arel)
+          sql, binds = to_sql_and_binds(arel, binds)
 
-        if preparable.nil?
-          preparable = prepared_statements ? visitor.preparable : false
-        end
+          if preparable.nil?
+            preparable = prepared_statements ? visitor.preparable : false
+          end
 
-        if prepared_statements && preparable
-          select_prepared(sql, name, binds)
-        else
-          select(sql, name, binds)
+          if prepared_statements && preparable
+            select_prepared(sql, name, binds)
+          else
+            select(sql, name, binds)
+          end
         end
       end
 

activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb

@@ -94,35 +94,35 @@ def clear_query_cache
       end
 
       def select_all(arel, name = nil, binds = [], preparable: nil)
-        if @query_cache_enabled && !locked?(arel)
-          arel = arel_from_relation(arel)
-          sql, binds = to_sql_and_binds(arel, binds)
+        @lock.synchronize do
+          if @query_cache_enabled && !locked?(arel)
+            arel = arel_from_relation(arel)
+            sql, binds = to_sql_and_binds(arel, binds)
 
-          if preparable.nil?
-            preparable = prepared_statements ? visitor.preparable : false
-          end
+            if preparable.nil?
+              preparable = prepared_statements ? visitor.preparable : false
+            end
 
-          cache_sql(sql, name, binds) { super(sql, name, binds, preparable: preparable) }
-        else
-          super
+            cache_sql(sql, name, binds) { super(sql, name, binds, preparable: preparable) }
+          else
+            super
+          end
         end
       end
 
       private
         def cache_sql(sql, name, binds)
-          @lock.synchronize do
-            result =
-              if @query_cache[sql].key?(binds)
-                ActiveSupport::Notifications.instrument(
-                  "sql.active_record",
-                  cache_notification_info(sql, name, binds)
-                )
-                @query_cache[sql][binds]
-              else
-                @query_cache[sql][binds] = yield
-              end
-            result.dup
-          end
+          result =
+            if @query_cache[sql].key?(binds)
+              ActiveSupport::Notifications.instrument(
+                "sql.active_record",
+                cache_notification_info(sql, name, binds)
+              )
+              @query_cache[sql][binds]
+            else
+              @query_cache[sql][binds] = yield
+            end
+          result.dup
         end
 
         # Database adapters can override this method to

activerecord/lib/active_record/connection_adapters/abstract_adapter.rb

@@ -258,10 +258,12 @@ def seconds_idle # :nodoc:
       end
 
       def unprepared_statement
-        old_prepared_statements, @prepared_statements = @prepared_statements, false
-        yield
-      ensure
-        @prepared_statements = old_prepared_statements
+        @lock.synchronize do
+          old_prepared_statements, @prepared_statements = @prepared_statements, false
+          yield
+        ensure
+          @prepared_statements = old_prepared_statements
+        end
       end
 
       # Returns the human-readable name of the adapter. Use mixed case - one

activerecord/lib/active_record/connection_adapters/mysql/database_statements.rb

@@ -6,10 +6,12 @@ module MySQL
       module DatabaseStatements
         # Returns an ActiveRecord::Result instance.
         def select_all(*) # :nodoc:
-          result = if ExplainRegistry.collect? && prepared_statements
-            unprepared_statement { super }
-          else
-            super
+          result = @lock.synchronize do
+            if ExplainRegistry.collect? && prepared_statements
+              unprepared_statement { super }
+            else
+              super
+            end
           end
           @connection.abandon_results!
           result

activerecord/lib/active_record/core.rb

@@ -168,11 +168,14 @@ def find(*ids) # :nodoc:
 
         key = primary_key
 
-        statement = cached_find_by_statement(key) { |params|
-          where(key => params.bind).limit(1)
-        }
+        record = connection.lock.synchronize do
+          statement = cached_find_by_statement(key) { |params|
+            where(key => params.bind).limit(1)
+          }
+
+          statement.execute([id], connection)&.first
+        end
 
-        record = statement.execute([id], connection)&.first
         unless record
           raise RecordNotFound.new("Couldn't find #{name} with '#{key}'=#{id}", name, key, id)
         end
@@ -194,16 +197,18 @@ def find_by(*args) # :nodoc:
 
         keys = hash.keys
 
-        statement = cached_find_by_statement(keys) { |params|
-          wheres = keys.each_with_object({}) { |param, o|
-            o[param] = params.bind
+        connection.lock.synchronize do
+          statement = cached_find_by_statement(keys) { |params|
+            wheres = keys.each_with_object({}) { |param, o|
+              o[param] = params.bind
+            }
+            where(wheres).limit(1)
           }
-          where(wheres).limit(1)
-        }
-        begin
-          statement.execute(hash.values, connection)&.first
-        rescue TypeError
-          raise ActiveRecord::StatementInvalid
+          begin
+            statement.execute(hash.values, connection)&.first
+          rescue TypeError
+            raise ActiveRecord::StatementInvalid
+          end
         end
       end