Add comprehensive locking around DB transactions

Transactional-fixture using tests with racing threads and inter-thread
synchronisation inside transaction blocks will now deadlock... but
without this, they would just crash.

In 5.0, the threads didn't share a connection at all, so it would've
worked... but with the main thread inside the fixture transaction, they
wouldn't've been able to see each other.

So: as far as I can tell, the set of operations this "breaks" never had
a compelling use case. Meanwhile, it provides an increased level of
coherency to the operational feel of transactional fixtures.

If this does cause anyone problems, they're probably best off disabling
transactional fixtures on the affected tests, and managing transactions
themselves.

1 files changed, 49 insertions, 39 deletions

diff --git a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
index 6bb072dd73..19b7821494 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
@@ -149,57 +149,67 @@ module ActiveRecord
       end
 
       def begin_transaction(options = {})
-        run_commit_callbacks = !current_transaction.joinable?
-        transaction =
-          if @stack.empty?
-            RealTransaction.new(@connection, options, run_commit_callbacks: run_commit_callbacks)
-          else
-            SavepointTransaction.new(@connection, "active_record_#{@stack.size}", options,
-                                     run_commit_callbacks: run_commit_callbacks)
-          end
+        @connection.lock.synchronize do
+          run_commit_callbacks = !current_transaction.joinable?
+          transaction =
+            if @stack.empty?
+              RealTransaction.new(@connection, options, run_commit_callbacks: run_commit_callbacks)
+            else
+              SavepointTransaction.new(@connection, "active_record_#{@stack.size}", options,
+                                       run_commit_callbacks: run_commit_callbacks)
+            end
 
-        @stack.push(transaction)
-        transaction
+          @stack.push(transaction)
+          transaction
+        end
       end
 
       def commit_transaction
-        transaction = @stack.last
+        @connection.lock.synchronize do
+          transaction = @stack.last
 
-        begin
-          transaction.before_commit_records
-        ensure
-          @stack.pop
-        end
+          begin
+            transaction.before_commit_records
+          ensure
+            @stack.pop
+          end
 
-        transaction.commit
-        transaction.commit_records
+          transaction.commit
+          transaction.commit_records
+        end
       end
 
       def rollback_transaction(transaction = nil)
-        transaction ||= @stack.pop
-        transaction.rollback
-        transaction.rollback_records
+        @connection.lock.synchronize do
+          transaction ||= @stack.pop
+          transaction.rollback
+          transaction.rollback_records
+        end
       end
 
       def within_new_transaction(options = {})
-        transaction = begin_transaction options
-        yield
-      rescue Exception => error
-        if transaction
-          rollback_transaction
-          after_failure_actions(transaction, error)
-        end
-        raise
-      ensure
-        unless error
-          if Thread.current.status == "aborting"
-            rollback_transaction if transaction
-          else
-            begin
-              commit_transaction
-            rescue Exception
-              rollback_transaction(transaction) unless transaction.state.completed?
-              raise
+        @connection.lock.synchronize do
+          begin
+            transaction = begin_transaction options
+            yield
+          rescue Exception => error
+            if transaction
+              rollback_transaction
+              after_failure_actions(transaction, error)
+            end
+            raise
+          ensure
+            unless error
+              if Thread.current.status == "aborting"
+                rollback_transaction if transaction
+              else
+                begin
+                  commit_transaction
+                rescue Exception
+                  rollback_transaction(transaction) unless transaction.state.completed?
+                  raise
+                end
+              end
             end
           end
         end