Merge pull request #13656 from chanks/rollback_transactions_in_killed_threads

Data corruption risk: Roll back open transactions when the running thread is killed.

3 files changed, 50 insertions, 5 deletions

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'