diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 13e8292954..40eb32c059 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,11 @@ +* When a thread is killed, rollback the active transaction, instead of + committing it during the stack unwind. Previously, we could commit half- + completed work. This fix only works for Ruby 2.0+; on 1.9, we can't + distinguish a thread kill from an ordinary non-local (block) return, so must + default to committing. + + *Chris Hanks* + * A `NullRelation` should represent nothing. This fixes a bug where `Comment.where(post_id: Post.none)` returned a non-empty result. diff --git a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb index 90be835d8a..fd666c8c39 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb @@ -190,11 +190,17 @@ module ActiveRecord rollback_transaction if transaction raise ensure - begin - commit_transaction unless error - rescue Exception - transaction.rollback unless transaction.state.completed? - raise + unless error + if Thread.current.status == 'aborting' + rollback_transaction + else + begin + commit_transaction + rescue Exception + transaction.rollback unless transaction.state.completed? + raise + end + end end end diff --git a/activerecord/test/cases/transactions_test.rb b/activerecord/test/cases/transactions_test.rb index 9cfe041de2..5cccf2dda5 100644 --- a/activerecord/test/cases/transactions_test.rb +++ b/activerecord/test/cases/transactions_test.rb @@ -492,6 +492,37 @@ class TransactionTest < ActiveRecord::TestCase assert topic.frozen?, 'not frozen' end + # The behavior of killed threads having a status of "aborting" was changed + # in Ruby 2.0, so Thread#kill on 1.9 will prematurely commit the transaction + # and there's nothing we can do about it. + unless RUBY_VERSION.start_with? '1.9' + def test_rollback_when_thread_killed + queue = Queue.new + thread = Thread.new do + Topic.transaction do + @first.approved = true + @second.approved = false + @first.save + + queue.push nil + sleep + + @second.save + end + end + + queue.pop + thread.kill + thread.join + + assert @first.approved?, "First should still be changed in the objects" + assert !@second.approved?, "Second should still be changed in the objects" + + assert !Topic.find(1).approved?, "First shouldn't have been approved" + assert Topic.find(2).approved?, "Second should still be approved" + end + end + def test_restore_active_record_state_for_all_records_in_a_transaction topic_without_callbacks = Class.new(ActiveRecord::Base) do self.table_name = 'topics'