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

Idempotent counter caches, fix concurrency issues with counter caches

3 files changed, 47 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
diff --git a/activerecord/test/cases/associations/belongs_to_associations_test.rb b/activerecord/test/cases/associations/belongs_to_associations_test.rb
index bcd2b6dfaa..3b484a0d64 100644
--- a/activerecord/test/cases/associations/belongs_to_associations_test.rb
+++ b/activerecord/test/cases/associations/belongs_to_associations_test.rb
@@ -502,6 +502,27 @@ class BelongsToAssociationsTest < ActiveRecord::TestCase
     assert_equal 4, topic.replies.size
   end
 
+  def test_concurrent_counter_cache_double_destroy
+    topic = Topic.create :title => "Zoom-zoom-zoom"
+
+    5.times do
+      topic.replies.create(:title => "re: zoom", :content => "speedy quick!")
+    end
+
+    assert_equal 5, topic.reload[:replies_count]
+    assert_equal 5, topic.replies.size
+
+    reply = topic.replies.first
+    reply_clone = Reply.find(reply.id)
+
+    reply.destroy
+    assert_equal 4, topic.reload[:replies_count]
+
+    reply_clone.destroy
+    assert_equal 4, topic.reload[:replies_count]
+    assert_equal 4, topic.replies.size
+  end
+
   def test_custom_counter_cache
     reply = Reply.create(:title => "re: zoom", :content => "speedy quick!")
     assert_equal 0, reply[:replies_count]