Merge pull request #16284 from arthurnn/transactions

Transactions refactoring

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

@@ -203,62 +203,30 @@ module ActiveRecord
           if options[:isolation]
             raise ActiveRecord::TransactionIsolationError, "cannot set isolation when joining a transaction"
           end
-
           yield
         else
-          within_new_transaction(options) { yield }
+          transaction_manager.within_new_transaction(options) { yield }
         end
       rescue ActiveRecord::Rollback
         # rollbacks are silently swallowed
       end
 
-      def within_new_transaction(options = {}) #:nodoc:
+      attr_reader :transaction_manager #:nodoc:
-        transaction = begin_transaction(options)
-        yield
-      rescue Exception => error
-        rollback_transaction if transaction
-        raise
-      ensure
-        begin
-          commit_transaction unless error
-        rescue Exception
-          rollback_transaction
-          raise
-        end
-      end
 
-      def open_transactions
+      delegate :within_new_transaction, :open_transactions, :current_transaction, :begin_transaction, :commit_transaction, :rollback_transaction, to: :transaction_manager
-        @transaction.number
-      end
-
-      def current_transaction #:nodoc:
-        @transaction
-      end
 
       def transaction_open?
-        @transaction.open?
+        current_transaction.open?
-      end
-
-      def begin_transaction(options = {}) #:nodoc:
-        @transaction = @transaction.begin(options)
-      end
-
-      def commit_transaction #:nodoc:
-        @transaction = @transaction.commit
-      end
-
-      def rollback_transaction #:nodoc:
-        @transaction = @transaction.rollback
       end
 
       def reset_transaction #:nodoc:
-        @transaction = ClosedTransaction.new(self)
+        @transaction_manager = TransactionManager.new(self)
       end
 
       # Register a record with the current transaction so that its after_commit and after_rollback callbacks
       # can be called.
       def add_transaction_record(record)
-        @transaction.add_record(record)
+        current_transaction.add_record(record)
       end
 
       # Begins the transaction (and turns off auto-committing).

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

@@ -1,5 +1,63 @@
 module ActiveRecord
   module ConnectionAdapters
+    class TransactionManager #:nodoc:
+      def initialize(connection)
+        @stack = []
+        @connection = connection
+      end
+
+      def begin_transaction(options = {})
+        transaction =
+          if @stack.empty?
+            RealTransaction.new(@connection, current_transaction, options)
+          else
+            SavepointTransaction.new(@connection, current_transaction, options)
+          end
+
+        @stack.push(transaction)
+        transaction
+      end
+
+      def commit_transaction
+        @stack.pop.commit
+      end
+
+      def rollback_transaction
+        @stack.pop.rollback
+      end
+
+      def within_new_transaction(options = {})
+        transaction = begin_transaction options
+        yield
+      rescue Exception => error
+        transaction.rollback if transaction
+        raise
+      ensure
+        begin
+          transaction.commit unless error
+        rescue Exception
+          transaction.rollback
+          raise
+        ensure
+          @stack.pop if transaction
+        end
+      end
+
+      def open_transactions
+        @stack.size
+      end
+
+      def current_transaction
+        @stack.last || closed_transaction
+      end
+
+      private
+
+        def closed_transaction
+          @closed_transaction ||= ClosedTransaction.new(@connection)
+        end
+    end
+
     class Transaction #:nodoc:
       attr_reader :connection
 
@@ -11,6 +69,10 @@ module ActiveRecord
       def state
         @state
       end
+
+      def savepoint_name
+        nil
+      end
     end
 
     class TransactionState
@@ -78,45 +140,28 @@ module ActiveRecord
 
         @parent    = parent
         @records   = []
-        @finishing = false
         @joinable  = options.fetch(:joinable, true)
       end
 
-      # This state is necessary so that we correctly handle stuff that might
-      # happen in a commit/rollback. But it's kinda distasteful. Maybe we can
-      # find a better way to structure it in the future.
-      def finishing?
-        @finishing
-      end
 
       def joinable?
-        @joinable && !finishing?
+        @joinable
       end
 
       def number
-        if finishing?
+        parent.number + 1
-          parent.number
-        else
-          parent.number + 1
-        end
       end
 
       def begin(options = {})
-        if finishing?
+        SavepointTransaction.new(connection, self, options)
-          parent.begin
-        else
-          SavepointTransaction.new(connection, self, options)
-        end
       end
 
       def rollback
-        @finishing = true
         perform_rollback
         parent
       end
 
       def commit
-        @finishing = true
         perform_commit
         parent
       end
@@ -183,24 +228,29 @@ module ActiveRecord
     end
 
     class SavepointTransaction < OpenTransaction #:nodoc:
+      attr_reader :savepoint_name
+
       def initialize(connection, parent, options = {})
         if options[:isolation]
           raise ActiveRecord::TransactionIsolationError, "cannot set transaction isolation in a nested transaction"
         end
 
         super
-        connection.create_savepoint
+
+        # Savepoint name only counts the Savepoint transactions, so we need to subtract 1
+        @savepoint_name = "active_record_#{number - 1}"
+        connection.create_savepoint(@savepoint_name)
       end
 
       def perform_rollback
-        connection.rollback_to_savepoint
+        connection.rollback_to_savepoint(@savepoint_name)
         rollback_records
       end
 
       def perform_commit
         @state.set_state(:committed)
         @state.parent = parent.state
-        connection.release_savepoint
+        connection.release_savepoint(@savepoint_name)
       end
     end
   end

activerecord/lib/active_record/connection_adapters/abstract_adapter.rb

@@ -45,6 +45,7 @@ module ActiveRecord
     end
 
     autoload_at 'active_record/connection_adapters/abstract/transaction' do
+      autoload :TransactionManager
       autoload :ClosedTransaction
       autoload :RealTransaction
       autoload :SavepointTransaction
@@ -357,7 +358,7 @@ module ActiveRecord
       end
 
       def current_savepoint_name
-        "active_record_#{open_transactions}"
+        current_transaction.savepoint_name
       end
 
       # Check the connection back in to the connection pool

activerecord/test/cases/transactions_test.rb

@@ -424,6 +424,26 @@ class TransactionTest < ActiveRecord::TestCase
     end
   end
 
+  def test_savepoints_name
+    Topic.transaction do
+      assert_nil Topic.connection.current_savepoint_name
+      assert_nil Topic.connection.current_transaction.savepoint_name
+
+      Topic.transaction(requires_new: true) do
+        assert_equal "active_record_1", Topic.connection.current_savepoint_name
+        assert_equal "active_record_1", Topic.connection.current_transaction.savepoint_name
+
+        Topic.transaction(requires_new: true) do
+          assert_equal "active_record_2", Topic.connection.current_savepoint_name
+          assert_equal "active_record_2", Topic.connection.current_transaction.savepoint_name
+        end
+
+        assert_equal "active_record_1", Topic.connection.current_savepoint_name
+        assert_equal "active_record_1", Topic.connection.current_transaction.savepoint_name
+      end
+    end
+  end
+
   def test_rollback_when_commit_raises
     Topic.connection.expects(:begin_db_transaction)
     Topic.connection.expects(:commit_db_transaction).raises('OH NOES')