diff --git a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb index e1f6410cfc..757ea75759 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb @@ -317,25 +317,31 @@ module ActiveRecord end raise ensure - if !error && transaction - if Thread.current.status == "aborting" - rollback_transaction + if transaction + if error + # @connection still holds an open transaction, so we must not + # put it back in the pool for reuse + @connection.throw_away! unless transaction.state.rolledback? else - if !completed && transaction.written - ActiveSupport::Deprecation.warn(<<~EOW) - Using `return`, `break` or `throw` to exit a transaction block is - deprecated without replacement. If the `throw` came from - `Timeout.timeout(duration)`, pass an exception class as a second - argument so it doesn't use `throw` to abort its block. This results - in the transaction being committed, but in the next release of Rails - it will rollback. - EOW - end - begin - commit_transaction - rescue Exception - rollback_transaction(transaction) unless transaction.state.completed? - raise + if Thread.current.status == "aborting" + rollback_transaction + else + if !completed && transaction.written + ActiveSupport::Deprecation.warn(<<~EOW) + Using `return`, `break` or `throw` to exit a transaction block is + deprecated without replacement. If the `throw` came from + `Timeout.timeout(duration)`, pass an exception class as a second + argument so it doesn't use `throw` to abort its block. This results + in the transaction being committed, but in the next release of Rails + it will rollback. + EOW + end + begin + commit_transaction + rescue Exception + rollback_transaction(transaction) unless transaction.state.completed? + raise + 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 e3ca0f85b3..70b4fbd266 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb @@ -501,6 +501,12 @@ module ActiveRecord # this should be overridden by concrete adapters end + # Removes the connection from the pool and disconnect it. + def throw_away! + pool.remove self + disconnect! + end + # Clear any caching the database adapter may be doing. def clear_cache! @lock.synchronize { @statements.clear } if @statements diff --git a/activerecord/test/cases/transactions_test.rb b/activerecord/test/cases/transactions_test.rb index f782126ba2..6c0142627e 100644 --- a/activerecord/test/cases/transactions_test.rb +++ b/activerecord/test/cases/transactions_test.rb @@ -30,6 +30,53 @@ class TransactionTest < ActiveRecord::TestCase assert_equal title_change, topic.changes["title"] end + if !in_memory_db? + def test_rollback_dirty_changes_even_with_raise_during_rollback_removes_from_pool + topic = topics(:fifth) + + connection = Topic.connection + + Topic.connection.class_eval do + alias :real_exec_rollback_db_transaction :exec_rollback_db_transaction + define_method(:exec_rollback_db_transaction) do + raise + end + end + + ActiveRecord::Base.transaction do + topic.update(title: "Rails is broken") + raise ActiveRecord::Rollback + end + + assert_not connection.active? + assert_not Topic.connection_pool.connections.include?(connection) + end + + def test_rollback_dirty_changes_even_with_raise_during_rollback_doesnt_commit_transaction + topic = topics(:fifth) + + Topic.connection.class_eval do + alias :real_exec_rollback_db_transaction :exec_rollback_db_transaction + define_method(:exec_rollback_db_transaction) do + raise + end + end + + ActiveRecord::Base.transaction do + topic.update(title: "Rails is broken") + raise ActiveRecord::Rollback + end + + topic.reload + + ActiveRecord::Base.transaction do + topic.update(content: "Ruby on Rails - modified") + end + + assert_equal "The Fifth Topic of the day", topic.reload.title + end + end + def test_rollback_dirty_changes_multiple_saves topic = topics(:fifth)