Merge pull request #45280 from cbothner/run-transactional-callbacks-on-instances-most-likely-to-match-database-state

Run transactional callbacks on instances most likely to match DB state

activerecord/CHANGELOG.md

@@ -1,3 +1,34 @@
+*   Run transactional callbacks on the freshest instance to save a given 
+    record within a transaction.
+
+    When multiple Active Record instances change the same record within a 
+    transaction, Rails runs `after_commit` or `after_rollback` callbacks for 
+    only one of them. `config.active_record.run_commit_callbacks_on_first_saved_instances_in_transaction` 
+    was added to specify how Rails chooses which instance receives the 
+    callbacks. The framework defaults were changed to use the new logic.
+
+    When `config.active_record.run_commit_callbacks_on_first_saved_instances_in_transaction` 
+    is `true`, transactional callbacks are run on the first instance to save, 
+    even though its instance state may be stale.
+
+    When it is `false`, which is the new framework default starting with version 
+    7.1, transactional callbacks are run on the instances with the freshest 
+    instance state. Those instances are chosen as follows:
+
+    - In general, run transactional callbacks on the last instance to save a 
+      given record within the transaction.
+    - There are two exceptions:
+        - If the record is created within the transaction, then updated by 
+          another instance, `after_create_commit` callbacks will be run on the 
+          second instance. This is instead of the `after_update_commit` 
+          callbacks that would naively be run based on that instance’s state.
+        - If the record is destroyed within the transaction, then 
+          `after_destroy_commit` callbacks will be fired on the last destroyed 
+          instance, even if a stale instance subsequently performed an update 
+          (which will have affected 0 rows).
+
+    *Cameron Bothner and Mitch Vollebregt*
+
 *   Resolve issue where a relation cache_version could be left stale.
 
     Previously, when `reset` was called on a relation object it did not reset the cache_versions

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

@@ -147,12 +147,12 @@ module ActiveRecord
 
       def rollback_records
         return unless records
+
         ite = records.uniq(&:__id__)
-        already_run_callbacks = {}
+        instances_to_run_callbacks_on = prepare_instances_to_run_callbacks_on(ite)
+
         while record = ite.shift
-          trigger_callbacks = record.trigger_transactional_callbacks?
-          should_run_callbacks = !already_run_callbacks[record] && trigger_callbacks
-          already_run_callbacks[record] ||= trigger_callbacks
+          should_run_callbacks = record.__id__ == instances_to_run_callbacks_on[record].__id__
           record.rolledback!(force_restore_state: full_rollback?, should_run_callbacks: should_run_callbacks)
         end
       ensure
@@ -167,15 +167,18 @@ module ActiveRecord
 
       def commit_records
         return unless records
+
         ite = records.uniq(&:__id__)
-        already_run_callbacks = {}
-        while record = ite.shift
-          if @run_commit_callbacks
-            trigger_callbacks = record.trigger_transactional_callbacks?
-            should_run_callbacks = !already_run_callbacks[record] && trigger_callbacks
-            already_run_callbacks[record] ||= trigger_callbacks
+
+        if @run_commit_callbacks
+          instances_to_run_callbacks_on = prepare_instances_to_run_callbacks_on(ite)
+
+          while record = ite.shift
+            should_run_callbacks = record.__id__ == instances_to_run_callbacks_on[record].__id__
             record.committed!(should_run_callbacks: should_run_callbacks)
-          else
+          end
+        else
+          while record = ite.shift
             # if not running callbacks, only adds the record to the parent transaction
             connection.add_transaction_record(record)
           end
@@ -188,6 +191,33 @@ module ActiveRecord
       def joinable?; @joinable; end
       def closed?; false; end
       def open?; !closed?; end
+
+      private
+        def prepare_instances_to_run_callbacks_on(records)
+          records.each_with_object({}) do |record, candidates|
+            next unless record.trigger_transactional_callbacks?
+
+            earlier_saved_candidate = candidates[record]
+
+            next if earlier_saved_candidate && record.class.run_commit_callbacks_on_first_saved_instances_in_transaction
+
+            # If the candidate instance destroyed itself in the database, then
+            # instances which were added to the transaction afterwards, and which
+            # think they updated themselves, are wrong. They should not replace
+            # our candidate as an instance to run callbacks on
+            next if earlier_saved_candidate&.destroyed? && !record.destroyed?
+
+            # If the candidate instance was created inside of this transaction,
+            # then instances which were subsequently loaded from the database
+            # and updated need that state transferred to them so that
+            # the after_create_commit callbacks are run
+            record._new_record_before_last_commit = true if earlier_saved_candidate&._new_record_before_last_commit
+
+            # The last instance to save itself is likeliest to have internal
+            # state that matches what's committed to the database
+            candidates[record] = record
+          end
+        end
     end
 
     class RestartParentTransaction < Transaction

activerecord/lib/active_record/core.rb

@@ -81,6 +81,8 @@ module ActiveRecord
 
       class_attribute :has_many_inversing, instance_accessor: false, default: false
 
+      class_attribute :run_commit_callbacks_on_first_saved_instances_in_transaction, instance_accessor: false, default: true
+
       class_attribute :default_connection_handler, instance_writer: false
 
       class_attribute :default_role, instance_writer: false

activerecord/lib/active_record/transactions.rb

@@ -13,6 +13,8 @@ module ActiveRecord
                        scope: [:kind, :name]
     end
 
+    attr_accessor :_new_record_before_last_commit # :nodoc:
+
     # = Active Record Transactions
     #
     # \Transactions are protective blocks where SQL statements are only permanent

activerecord/test/cases/transaction_callbacks_test.rb

@@ -770,3 +770,143 @@ class CallbacksOnActionAndConditionTest < ActiveRecord::TestCase
     assert_equal [], topic.history
   end
 end
+
+class CallbacksOnMultipleInstancesInATransactionTest < ActiveRecord::TestCase
+  class TopicWithTitleHistory < ActiveRecord::Base
+    self.table_name = :topics
+
+    def self.clear_history
+      @@history = []
+    end
+
+    def self.history
+      @@history ||= []
+    end
+
+    after_create_commit { |record| record.class.history << "Created (title = #{record.title.inspect})" }
+    after_update_commit { |record| record.class.history << "Updated (title = #{record.title.inspect})" }
+    after_destroy_commit { |record| record.class.history << "Destroyed (title = #{record.title.inspect})" }
+  end
+
+  def test_created_callback_called_on_last_to_save_of_separate_instances_in_a_transaction
+    with_run_commit_callbacks_on_first_saved_instances_in_transaction(false) do
+      TopicWithTitleHistory.clear_history
+
+      TopicWithTitleHistory.transaction do
+        topic = TopicWithTitleHistory.create!(title: "A")
+        TopicWithTitleHistory.find(topic.id).update!(title: "B")
+      end
+
+      assert_equal ['Created (title = "B")'], TopicWithTitleHistory.history
+    end
+  end
+
+  def test_created_callback_called_on_first_to_save_in_transaction_with_old_configuration
+    with_run_commit_callbacks_on_first_saved_instances_in_transaction(true) do
+      TopicWithTitleHistory.clear_history
+
+      TopicWithTitleHistory.transaction do
+        topic = TopicWithTitleHistory.create!(title: "A")
+        TopicWithTitleHistory.find(topic.id).update!(title: "B")
+      end
+
+      assert_equal ['Created (title = "A")'], TopicWithTitleHistory.history
+    end
+  end
+
+  def test_updated_callback_called_on_last_to_save_of_separate_instances_in_a_transaction
+    with_run_commit_callbacks_on_first_saved_instances_in_transaction(false) do
+      topic = TopicWithTitleHistory.create!(title: "one")
+      TopicWithTitleHistory.clear_history
+
+      TopicWithTitleHistory.transaction do
+        TopicWithTitleHistory.find(topic.id).update!(title: "two")
+        TopicWithTitleHistory.find(topic.id).update!(title: "three")
+      end
+
+      assert_equal ['Updated (title = "three")'], TopicWithTitleHistory.history
+    end
+  end
+
+  def test_updated_callback_called_on_first_to_save_in_transaction_with_old_configuration
+    with_run_commit_callbacks_on_first_saved_instances_in_transaction(true) do
+      topic = TopicWithTitleHistory.create!(title: "one")
+      TopicWithTitleHistory.clear_history
+
+      TopicWithTitleHistory.transaction do
+        TopicWithTitleHistory.find(topic.id).update!(title: "two")
+        TopicWithTitleHistory.find(topic.id).update!(title: "three")
+      end
+
+      assert_equal ['Updated (title = "two")'], TopicWithTitleHistory.history
+    end
+  end
+
+  def test_destroyed_callback_called_on_destroyed_instance_when_preceded_in_transaction_by_save_from_separate_instance
+    with_run_commit_callbacks_on_first_saved_instances_in_transaction(false) do
+      topic = TopicWithTitleHistory.create!(title: "one")
+      TopicWithTitleHistory.clear_history
+
+      TopicWithTitleHistory.transaction do
+        TopicWithTitleHistory.find(topic.id).update!(title: "two")
+        TopicWithTitleHistory.find(topic.id).destroy!
+      end
+
+      assert_equal ['Destroyed (title = "two")'], TopicWithTitleHistory.history
+    end
+  end
+
+  def test_updated_callback_called_on_first_to_save_when_followed_in_transaction_by_destroy_from_separate_instance_with_old_configuration
+    with_run_commit_callbacks_on_first_saved_instances_in_transaction(true) do
+      topic = TopicWithTitleHistory.create!(title: "one")
+      TopicWithTitleHistory.clear_history
+
+      TopicWithTitleHistory.transaction do
+        TopicWithTitleHistory.find(topic.id).update!(title: "two")
+        TopicWithTitleHistory.find(topic.id).destroy!
+      end
+
+      assert_equal ['Updated (title = "two")'], TopicWithTitleHistory.history
+    end
+  end
+
+  def test_destroyed_callbacks_called_on_destroyed_instance_even_when_followed_by_update_from_separate_instances_in_a_transaction
+    with_run_commit_callbacks_on_first_saved_instances_in_transaction(false) do
+      topic = TopicWithTitleHistory.create!(title: "one")
+      TopicWithTitleHistory.clear_history
+
+      TopicWithTitleHistory.transaction do
+        TopicWithTitleHistory.find(topic.id).destroy!
+        topic.update!(title: "two")
+      end
+
+      assert_equal ['Destroyed (title = "one")'], TopicWithTitleHistory.history
+    end
+  end
+
+  def test_destroyed_callbacks_called_on_first_saved_instance_in_transaction_with_old_configuration
+    with_run_commit_callbacks_on_first_saved_instances_in_transaction(true) do
+      topic = TopicWithTitleHistory.create!(title: "one")
+      TopicWithTitleHistory.clear_history
+
+      TopicWithTitleHistory.transaction do
+        TopicWithTitleHistory.find(topic.id).destroy!
+        topic.update!(title: "two")
+      end
+
+      assert_equal ['Destroyed (title = "one")'], TopicWithTitleHistory.history
+    end
+  end
+
+  def with_run_commit_callbacks_on_first_saved_instances_in_transaction(value, model: ActiveRecord::Base)
+    old = model.run_commit_callbacks_on_first_saved_instances_in_transaction
+    model.run_commit_callbacks_on_first_saved_instances_in_transaction = value
+    yield
+  ensure
+    model.run_commit_callbacks_on_first_saved_instances_in_transaction = old
+    if model != ActiveRecord::Base && !old
+      # reset the class_attribute
+      model.singleton_class.remove_method(:run_commit_callbacks_on_first_saved_instances_in_transaction)
+    end
+  end
+end

guides/source/configuring.md

@@ -937,6 +937,26 @@ The default value depends on the `config.load_defaults` target version:
 | (original)            | `false`              |
 | 7.0                   | `true`               |
 
+#### `config.active_record.run_commit_callbacks_on_first_saved_instances_in_transaction`
+
+When multiple Active Record instances change the same record within a transaction, Rails runs `after_commit` or `after_rollback` callbacks for only one of them. This option specifies how Rails chooses which instance receives the callbacks.
+
+When `true`, transactional callbacks are run on the first instance to save, even though its instance state may be stale.
+
+When `false`, transactional callbacks are run on the instances with the freshest instance state. Those instances are chosen as follows:
+
+- In general, run transactional callbacks on the last instance to save a given record within the transaction.
+- There are two exceptions:
+    - If the record is created within the transaction, then updated by another instance, `after_create_commit` callbacks will be run on the second instance. This is instead of the `after_update_commit` callbacks that would naively be run based on that instance’s state.
+    - If the record is destroyed within the transaction, then `after_destroy_commit` callbacks will be fired on the last destroyed instance, even if a stale instance subsequently performed an update (which will have affected 0 rows).
+
+The default value depends on the `config.load_defaults` target version:
+
+| Starting with version | The default value is |
+| --------------------- | -------------------- |
+| (original)            | `true`               |
+| 7.1                   | `false`              |
+
 #### `config.active_record.query_log_tags_enabled`
 
 Specifies whether or not to enable adapter-level query comments. Defaults to

railties/lib/rails/application/configuration.rb

@@ -262,6 +262,10 @@ module Rails
             self.log_file_size = (100 * 1024 * 1024)
           end
 
+          if respond_to?(:active_record)
+            active_record.run_commit_callbacks_on_first_saved_instances_in_transaction = false
+          end
+
           if respond_to?(:action_dispatch)
             action_dispatch.default_headers = {
               "X-Frame-Options" => "SAMEORIGIN",

railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_1.rb.tt

@@ -28,3 +28,10 @@
 # Do not treat an `ActionController::Parameters` instance
 # as equal to an equivalent `Hash` by default.
 # Rails.application.config.action_controller.allow_deprecated_parameters_hash_equality = false
+
+# No longer run after_commit callbacks on the first of multiple Active Record
+# instances to save changes to the same database row within a transaction.
+# Instead, run these callbacks on the instance most likely to have internal
+# state which matches what was committed to the database, typically the last
+# instance to save.
+# Rails.application.config.active_record.run_commit_callbacks_on_first_saved_instances_in_transaction = false

railties/test/application/configuration_test.rb

@@ -2382,6 +2382,32 @@ module ApplicationTests
       assert_equal true, ActiveRecord.verify_foreign_keys_for_fixtures
     end
 
+    test "ActiveRecord::Base.run_commit_callbacks_on_first_saved_instances_in_transaction is false by default for new apps" do
+      app "development"
+
+      assert_equal false, ActiveRecord::Base.run_commit_callbacks_on_first_saved_instances_in_transaction
+    end
+
+    test "ActiveRecord::Base.run_commit_callbacks_on_first_saved_instances_in_transaction is true by default for upgraded apps" do
+      remove_from_config '.*config\.load_defaults.*\n'
+
+      app "development"
+
+      assert_equal true, ActiveRecord::Base.run_commit_callbacks_on_first_saved_instances_in_transaction
+    end
+
+    test "ActiveRecord::Base.run_commit_callbacks_on_first_saved_instances_in_transaction can be configured via config.active_record.run_commit_callbacks_on_first_saved_instances_in_transaction" do
+      remove_from_config '.*config\.load_defaults.*\n'
+
+      app_file "config/initializers/new_framework_defaults_7_0.rb", <<-RUBY
+        Rails.application.config.active_record.run_commit_callbacks_on_first_saved_instances_in_transaction = false
+      RUBY
+
+      app "development"
+
+      assert_equal false, ActiveRecord::Base.run_commit_callbacks_on_first_saved_instances_in_transaction
+    end
+
     test "ActiveSupport::MessageEncryptor.use_authenticated_message_encryption is true by default for new apps" do
       app "development"