Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Added test cases and a fix for rails/rails#10865 - double incrementing of counter cache. #11594

Open
wants to merge 2 commits into from

5 participants

@jimjh

Assuming Topic has_many Replies and Reply belongs_to Topic, the counter cache was incremented twice if reply is saved after reassignment, as follows:

reply.topic = topic  # triggers BelongsToAssocation#replace
reply.save           # triggers BelongsTo belongs_to_counter_cache_after_update

The problem is caused by duplicate work.

Consider the following various ways to update a foreign key:

Case 1: new, assignment, save

example

This is not affected because it is not an update and BelongsTo belongs_to_counter_cache_after_update is not triggered.

Case 2: new, assignment, save, assignment, save

example

This is not affected because the @_after_create_counter_called flag does its work in BelongsTo.

Case 3: find/create, assignment, save

(Refer to new test case in this pull request.)

In the simplistic case, the save is superfluous, but because BelongsToAssocation#replace and belongs_to_counter_cache_after_update are both triggered, the counter is incremented twice.

Case 4: find/create, append, save

(Refer to new test case in this pull request.)

reply = Reply.find(1)
topic.replies << reply
reply.save

Similarly, the save is superfluous. This is not affected because << does not trigger BelongsToAssocation#replace.

Case 5: find/create, append, assignment, save

(Refer to new test case in this pull request.)

reply.topic = t1    # not saved, but counter updated
t2.replies << t2    # save! called, previous change needs to be undone

Fix

This commit adds @_dirty_but_updated_counter_cache flag to reply if it has already triggered an increment, but has not been saved yet. Thus, when the reply is actually saved, the superfluous increment does not occur.

Note that because of Case 5, the flag cannot be a simple boolean flag.

@jimjh jimjh Added test cases and a fix for rails/rails#10865.
Assuming Topic has_many Replies and Reply belongs_to Topic, the counter cache
was incremented twice if reply is saved after reassignment, as follows:

    reply.topic = topic  # triggers BelongsToAssocation#replace
    reply.save           # triggers belongs_to_counter_cache_after_update

The problem is caused by duplicate work.

This commit adds a flag to the reply if it has already triggered an increment,
but has not been saved yet. Thus, when the reply is actually saved, the
superfluous increment does not occur.

Note the following edge case:

    reply.topic = t1    # not saved, but counter updated
    t2.replies << t2    # save! called, previous change needs to be undone
52e30db
@joshuaclayton joshuaclayton referenced this pull request in thoughtbot/factory_girl
Closed

counter_cache seems to be duplicating #566

@xaviershay xaviershay commented on the diff
...st/cases/associations/belongs_to_associations_test.rb
((7 lines not shown))
+
+ topic.replies << reply
+ assert_equal 1, topic.reload.replies_count
+
+ assert reply.save
+ assert_equal 1, topic.reload.replies_count
+ end
+
+ def test_belongs_to_counter_with_reassigning_after_find
+ topic = Topic.create("title" => "t1")
+ reply = Reply.create("title" => "r1", "content" => "r1")
+ reply = Reply.find(reply.id)
+
+ reply.topic = topic
+
+ assert reply.save

I just hit this bug using exactly this test case on v4.0.0. Thanks for the patch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rafaelfranca

@tenderlove could you review this one?

@tenderlove tenderlove was assigned
@xaviershay xaviershay commented on the diff
.../lib/active_record/associations/builder/belongs_to.rb
((36 lines not shown))
end
end
end
+
+ def _belongs_to_counter_cache_record_change(reflection, from_key, to_key)
+ model = reflection.klass
+ cache_column = reflection.counter_cache_column
+ if to_key && model.respond_to?(:increment_counter)
+ model.increment_counter(cache_column, to_key)
+ end
+ if from_key && model.respond_to?(:decrement_counter)
+ model.decrement_counter(cache_column, from_key)
+ end

Pretty sure you need to sort these operations by the primary key of the model, or some other deterministic ordering. Otherwise you could deadlock between two transactions each transferring a model in opposite directions, since this code is locking the parent records in a different order.

First version that sprung to mind, lambas might be excessive though:

ops = []
if to_key && model.respond_to?(:increment_counter)
  ops << [to_key, lambda { model.increment_counter(cache_column, to_key) }]
end
if from_key && model.respond_to?(:decrement_counter)
  ops << [from_key, lambda { model.decrement_counter(cache_column, from_key) }]
end

ops.sort_by(&:first).map(&:last).each(&:call)

Looks like this logic is duplicated in #update_counters as well (existing code, not in your change) that would have to be updated also. So my objection shouldn't block this patch, since it matches existing behaviour. We can fix up the potential dead locks in a subsequent patch.

@jimjh
jimjh added a note

When does increment_counter obtain the lock, and when is it released?

At least in MySQL, when you run an update statement it implicitly places a write lock on the row, which is released at end of transaction. Other databases are similar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tenderlove tenderlove commented on the diff
.../active_record/associations/belongs_to_association.rb
((14 lines not shown))
end
+
+ owner.instance_variable_set(:@_dirty_but_updated_counter_cache, dirty_flag)
@tenderlove Owner

Why do we need to do this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jimjh jimjh commented on the diff
.../lib/active_record/associations/builder/belongs_to.rb
((15 lines not shown))
- elsif attribute_changed?(foreign_key) && !new_record? && association.constructable?
- model = reflection.klass
- foreign_key_was = attribute_was foreign_key
- foreign_key = attribute foreign_key
-
- if foreign_key && model.respond_to?(:increment_counter)
- model.increment_counter(cache_column, foreign_key)
- end
- if foreign_key_was && model.respond_to?(:decrement_counter)
- model.decrement_counter(cache_column, foreign_key_was)
+ return
+ when !attribute_changed?(foreign_key) || new_record? || !association.constructable?
+ return
+ when dirty && dirty == { from: foreign_key_from, to: foreign_key_to }
+ @_dirty_but_updated_counter_cache = false
+ return
@jimjh
jimjh added a note

@tenderlove @_dirty_but_updated_counter_cache is used here as dirty. It indicates that the counter cache has been updated, but the record itself has not been saved. When the record is saved and belongs_to_counter_cache_after_update is triggered, it needs to know that it should not update the counter_cache again.

More details are in the description of this pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jimjh

Feedback is always appreciated.

@tomchentw

Experienced the same issue here... Is this really a bug or something?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 25, 2013
  1. @jimjh

    Added test cases and a fix for rails/rails#10865.

    jimjh authored
    Assuming Topic has_many Replies and Reply belongs_to Topic, the counter cache
    was incremented twice if reply is saved after reassignment, as follows:
    
        reply.topic = topic  # triggers BelongsToAssocation#replace
        reply.save           # triggers belongs_to_counter_cache_after_update
    
    The problem is caused by duplicate work.
    
    This commit adds a flag to the reply if it has already triggered an increment,
    but has not been saved yet. Thus, when the reply is actually saved, the
    superfluous increment does not occur.
    
    Note the following edge case:
    
        reply.topic = t1    # not saved, but counter updated
        t2.replies << t2    # save! called, previous change needs to be undone
  2. @jimjh

    fixed sloppy reflection.klass

    jimjh authored
This page is out of date. Refresh to see the latest.
View
10 activerecord/lib/active_record/associations/belongs_to_association.rb
@@ -38,20 +38,26 @@ def update_counters(record)
counter_cache_name = reflection.counter_cache_column
if counter_cache_name && owner.persisted? && different_target?(record)
+ dirty_flag = { from: nil, to: nil }
+
if record
record.class.increment_counter(counter_cache_name, record.id)
+ dirty_flag[:to] = record.id
end
if foreign_key_present?
klass.decrement_counter(counter_cache_name, target_id)
+ dirty_flag[:from] = target_id
end
+
+ owner.instance_variable_set(:@_dirty_but_updated_counter_cache, dirty_flag)
@tenderlove Owner

Why do we need to do this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
end
end
# Checks whether record is different to the current target, without loading it
def different_target?(record)
- if record.nil?
- owner[reflection.foreign_key]
+ if record.nil?
+ owner[reflection.foreign_key]
else
record.id != owner[reflection.foreign_key]
end
View
42 activerecord/lib/active_record/associations/builder/belongs_to.rb
@@ -49,24 +49,40 @@ def belongs_to_counter_cache_before_destroy(association, reflection)
end
def belongs_to_counter_cache_after_update(association, reflection)
- foreign_key = reflection.foreign_key
- cache_column = reflection.counter_cache_column
+ foreign_key = reflection.foreign_key
+ foreign_key_from = attribute_was foreign_key
+ foreign_key_to = attribute foreign_key
+ dirty = (@_dirty_but_updated_counter_cache ||= false)
- if (@_after_create_counter_called ||= false)
+ case
+ when (@_after_create_counter_called ||= false)
@_after_create_counter_called = false
- elsif attribute_changed?(foreign_key) && !new_record? && association.constructable?
- model = reflection.klass
- foreign_key_was = attribute_was foreign_key
- foreign_key = attribute foreign_key
-
- if foreign_key && model.respond_to?(:increment_counter)
- model.increment_counter(cache_column, foreign_key)
- end
- if foreign_key_was && model.respond_to?(:decrement_counter)
- model.decrement_counter(cache_column, foreign_key_was)
+ return
+ when !attribute_changed?(foreign_key) || new_record? || !association.constructable?
+ return
+ when dirty && dirty == { from: foreign_key_from, to: foreign_key_to }
+ @_dirty_but_updated_counter_cache = false
+ return
@jimjh
jimjh added a note

@tenderlove @_dirty_but_updated_counter_cache is used here as dirty. It indicates that the counter cache has been updated, but the record itself has not been saved. When the record is saved and belongs_to_counter_cache_after_update is triggered, it needs to know that it should not update the counter_cache again.

More details are in the description of this pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ else
+ _belongs_to_counter_cache_record_change(reflection, foreign_key_from, foreign_key_to)
+ if dirty # undo
+ _belongs_to_counter_cache_record_change(reflection, dirty[:to], dirty[:from])
+ @_dirty_but_updated_counter_cache = false
end
end
end
+
+ def _belongs_to_counter_cache_record_change(reflection, from_key, to_key)
+ model = reflection.klass
+ cache_column = reflection.counter_cache_column
+ if to_key && model.respond_to?(:increment_counter)
+ model.increment_counter(cache_column, to_key)
+ end
+ if from_key && model.respond_to?(:decrement_counter)
+ model.decrement_counter(cache_column, from_key)
+ end

Pretty sure you need to sort these operations by the primary key of the model, or some other deterministic ordering. Otherwise you could deadlock between two transactions each transferring a model in opposite directions, since this code is locking the parent records in a different order.

First version that sprung to mind, lambas might be excessive though:

ops = []
if to_key && model.respond_to?(:increment_counter)
  ops << [to_key, lambda { model.increment_counter(cache_column, to_key) }]
end
if from_key && model.respond_to?(:decrement_counter)
  ops << [from_key, lambda { model.decrement_counter(cache_column, from_key) }]
end

ops.sort_by(&:first).map(&:last).each(&:call)

Looks like this logic is duplicated in #update_counters as well (existing code, not in your change) that would have to be updated also. So my objection shouldn't block this patch, since it matches existing behaviour. We can fix up the potential dead locks in a subsequent patch.

@jimjh
jimjh added a note

When does increment_counter obtain the lock, and when is it released?

At least in MySQL, when you run an update statement it implicitly places a write lock on the row, which is released at end of transaction. Other databases are similar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ end
+
end
end
View
36 activerecord/test/cases/associations/belongs_to_associations_test.rb
@@ -264,6 +264,42 @@ def test_belongs_to_with_primary_key_counter
assert_equal 0, debate2.reload.replies_count
end
+ def test_belongs_to_counter_with_append_after_create
+ topic = Topic.create("title" => "t1")
+ reply = Reply.create("title" => "r1", "content" => "r1")
+
+ topic.replies << reply
+ assert_equal 1, topic.reload.replies_count
+
+ assert reply.save
+ assert_equal 1, topic.reload.replies_count
+ end
+
+ def test_belongs_to_counter_with_reassigning_after_find
+ topic = Topic.create("title" => "t1")
+ reply = Reply.create("title" => "r1", "content" => "r1")
+ reply = Reply.find(reply.id)
+
+ reply.topic = topic
+
+ assert reply.save

I just hit this bug using exactly this test case on v4.0.0. Thanks for the patch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ assert_equal 1, topic.reload.replies_count
+ end
+
+ def test_belongs_to_counter_with_append_after_reassigning
+ topic1 = Topic.create("title" => "t1")
+ topic2 = Topic.create("title" => "t2")
+ reply = Reply.create("title" => "r1", "content" => "r1")
+ reply = Reply.find(reply.id)
+
+ reply.topic = topic1
+ topic2.replies << reply
+
+ assert reply.save
+ assert_equal 0, topic1.reload.replies_count
+ assert_equal 1, topic2.reload.replies_count
+ end
+
def test_belongs_to_counter_with_reassigning
topic1 = Topic.create("title" => "t1")
topic2 = Topic.create("title" => "t2")
Something went wrong with that request. Please try again.