Merge pull request #14735 from byroot/idempotent-counter-caches

Idempotent counter caches, fix concurrency issues with counter caches

2 files changed, 26 insertions, 4 deletions

diff --git a/activerecord/lib/active_record/associations/builder/belongs_to.rb b/activerecord/lib/active_record/associations/builder/belongs_to.rb
index 5ccaa55a32..11be92ae01 100644
--- a/activerecord/lib/active_record/associations/builder/belongs_to.rb
+++ b/activerecord/lib/active_record/associations/builder/belongs_to.rb
@@ -37,13 +37,14 @@ module ActiveRecord::Associations::Builder
           end
         end
 
-        def belongs_to_counter_cache_before_destroy(reflection)
+        def belongs_to_counter_cache_after_destroy(reflection)
           foreign_key = reflection.foreign_key.to_sym
           unless destroyed_by_association && destroyed_by_association.foreign_key.to_sym == foreign_key
             record = send reflection.name
-            if record && !self.destroyed?
+            if record && self.actually_destroyed?
               cache_column = reflection.counter_cache_column
               record.class.decrement_counter(cache_column, record.id)
+              self.clear_destroy_state
             end
           end
         end
@@ -77,8 +78,8 @@ module ActiveRecord::Associations::Builder
         record.belongs_to_counter_cache_after_create(reflection)
       }
 
-      model.before_destroy lambda { |record|
-        record.belongs_to_counter_cache_before_destroy(reflection)
+      model.after_destroy lambda { |record|
+        record.belongs_to_counter_cache_after_destroy(reflection)
       }
 
       model.after_update lambda { |record|
diff --git a/activerecord/lib/active_record/counter_cache.rb b/activerecord/lib/active_record/counter_cache.rb
index dcbdf75627..a5897edf03 100644
--- a/activerecord/lib/active_record/counter_cache.rb
+++ b/activerecord/lib/active_record/counter_cache.rb
@@ -118,5 +118,26 @@ module ActiveRecord
         update_counters(id, counter_name => -1)
       end
     end
+
+    protected
+
+      def actually_destroyed?
+        @_actually_destroyed
+      end
+
+      def clear_destroy_state
+        @_actually_destroyed = nil
+      end
+
+    private
+
+      def destroy_row
+        affected_rows = super
+
+        @_actually_destroyed = affected_rows > 0
+
+        affected_rows
+      end
+
   end
 end