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.

3 files changed, 70 insertions, 52 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
diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
index 96083e6519..85d6fbe8b3 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
@@ -74,7 +74,7 @@ module ActiveRecord
       SIMPLE_INT = /\A\d+\z/
 
       attr_accessor :visitor, :pool
-      attr_reader :schema_cache, :owner, :logger, :prepared_statements
+      attr_reader :schema_cache, :owner, :logger, :prepared_statements, :lock
       alias :in_use? :owner
 
       def self.type_cast_config_to_integer(config)
diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
index 0ad114165e..7ae9bd9a5f 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
@@ -239,7 +239,9 @@ module ActiveRecord
 
       # Is this connection alive and ready for queries?
       def active?
-        @connection.query "SELECT 1"
+        @lock.synchronize do
+          @connection.query "SELECT 1"
+        end
         true
       rescue PG::Error
         false
@@ -247,26 +249,32 @@ module ActiveRecord
 
       # Close then reopen the connection.
       def reconnect!
-        super
-        @connection.reset
-        configure_connection
+        @lock.synchronize do
+          super
+          @connection.reset
+          configure_connection
+        end
       end
 
       def reset!
-        clear_cache!
-        reset_transaction
-        unless @connection.transaction_status == ::PG::PQTRANS_IDLE
-          @connection.query "ROLLBACK"
+        @lock.synchronize do
+          clear_cache!
+          reset_transaction
+          unless @connection.transaction_status == ::PG::PQTRANS_IDLE
+            @connection.query "ROLLBACK"
+          end
+          @connection.query "DISCARD ALL"
+          configure_connection
         end
-        @connection.query "DISCARD ALL"
-        configure_connection
       end
 
       # Disconnects from the database if already connected. Otherwise, this
       # method does nothing.
       def disconnect!
-        super
-        @connection.close rescue nil
+        @lock.synchronize do
+          super
+          @connection.close rescue nil
+        end
       end
 
       def native_database_types #:nodoc: