Merge pull request #2 from ProductPlan/feat/touch-in-own-transaction

feat: do touches in separate transaction

Author: irphilli

.rubocop.yml

@@ -25,6 +25,9 @@ Metrics/AbcSize:
 Metrics/MethodLength:
   Enabled: false
 
+Metrics/ModuleLength:
+  Enabled: false
+
 Layout/LineLength:
   Enabled: false
 

README.md

@@ -65,7 +65,7 @@ class Pet < ActiveRecord::Base
 end
 ```
 ### Deadlock Prevention
-`batch_touching` will sort the consolidated SQL updates by model name. The predictable order for updates should help mitigate potential database deadlocking.
+`batch_touching` will sort the consolidated SQL updates by model name, and then commit touches in their own transaction. The separate transaction in a predictable order for updates should help mitigate potential database deadlocking.
 
 For example, if two transactions happen to touch records in the following order, there is a potential for a deadlock:
 

lib/activerecord/batch_touching.rb

@@ -22,9 +22,17 @@ module BatchTouchingAbstractAdapter
     #     Person.first.touch
     #   end
     def transaction(requires_new: nil, isolation: nil, joinable: true, &block)
-      super(requires_new: requires_new, isolation: isolation, joinable: joinable) do
+      result = super(requires_new: requires_new, isolation: isolation, joinable: joinable) do
         BatchTouching.start(requires_new: requires_new, &block)
       end
+
+      if BatchTouching.should_apply_touches?
+        super() do
+          BatchTouching.apply_touches
+        end
+      end
+
+      result
     end
   end
 
@@ -83,12 +91,12 @@ def disabled?
       def start(requires_new:)
         states.push State.new
         yield.tap do
-          apply_touches if states.length == 1
+          gather_touches if states.length == 1
         end
       ensure
         merge_transactions unless $! && requires_new
 
-        # Decrement nesting even if +apply_touches+ raised an error. To ensure the stack of States
+        # Decrement nesting even if +gather_touches+ raised an error. To ensure the stack of States
         # is empty after the top-level transaction exits.
         states.pop
       end
@@ -99,8 +107,34 @@ def batch_touching?
         states.present? && !disabled?
       end
 
+      def should_apply_touches?
+        batched_touches.present?
+      end
+
+      def apply_touches
+        batched_touches&.each do |(klass, columns), records|
+          records.reject!(&:destroyed?)
+          touch_records klass, columns, records, batched_touches_time
+        end
+
+        reset!
+      end
+
       private
 
+      def reset!
+        Thread.current[:batch_touching_batch] = nil
+        Thread.current[:batch_touching_time] = nil
+      end
+
+      def batched_touches
+        Thread.current[:batch_touching_batch]
+      end
+
+      def batched_touches_time
+        Thread.current[:batch_touching_time]
+      end
+
       def states
         Thread.current[:batch_touching_states] ||= []
       end
@@ -115,8 +149,8 @@ def merge_transactions
         states[-2].merge!(current_state) if states.length > 1
       end
 
-      # Apply the touches that were batched. We're in a transaction already so there's no need to open one.
-      def apply_touches
+      # Gather all the touches that were batched.
+      def gather_touches
         current_time = ActiveRecord::Base.current_time_from_proper_timezone
         already_touched = Set.new
         all_states = State.new
@@ -131,10 +165,10 @@ def apply_touches
 
         # Sort by class name. Having a consistent order can help mitigate deadlocks.
         sorted_records = all_states.records.keys.sort_by { |k| k.first.name }.to_h { |k| [k, all_states.records[k]] }
-        sorted_records.each do |(klass, columns), records|
-          records.reject!(&:destroyed?)
-          touch_records klass, columns, records, current_time
-        end
+
+        # Save results to a thread so that we can reference these later in their own transaction.
+        Thread.current[:batch_touching_batch] = sorted_records
+        Thread.current[:batch_touching_time] = current_time
       end
 
       # Perform DB update to touch records

spec/activerecord/batch_touching_spec.rb

@@ -358,6 +358,20 @@ def touch_owner
     end
   end
 
+  it "performs expected touches across multiple transactions" do
+    expect_updates [{ "pets" => { ids: pet1 } }, { "owners" => { ids: owner } }] do
+      ActiveRecord::Base.transaction do
+        pet1.touch
+      end
+    end
+
+    expect_updates [{ "pets" => { ids: pet2 } }, { "owners" => { ids: owner } }] do
+      ActiveRecord::Base.transaction do
+        pet2.touch
+      end
+    end
+  end
+
   context "with dependent deletes" do
     let(:post) { Post.create }
     let(:user) { User.create }