New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make AR#increment! AR#decrement! concurency safe #11410

Merged
merged 1 commit into from Oct 10, 2015

Conversation

Projects
None yet
7 participants
@bogdan
Contributor

bogdan commented Jul 12, 2013

increment! and decrement! should be concurency safe because
incorrect update can cause serious problems.

Especially when rails already have a concurency-safe counters update
mechanics in .update_counters method.

So this patch includes the following changes:

  • Reuse code between increment and decrement methods
  • Extracted a code snippet (search for ultimate 'eww' comment in diff)
    from has_many association internals to #increment! method
  • Make use of #increment! and #decrement! methods where possible
    instead of .update_counters that doesn't update an counters on
    record instance level
@sorentwo

View changes

activerecord/lib/active_record/persistence.rb Outdated
@@ -296,16 +296,18 @@ def increment(attribute, by = 1)
# Saving is not subjected to validation checks. Returns +true+ if the
# record could be saved.
def increment!(attribute, by = 1)
increment(attribute, by).update_attribute(attribute, self[attribute])
increment(attribute, by)
change = send(attribute) - (attribute_was(attribute.to_s) || 0)

This comment has been minimized.

@sorentwo

sorentwo Jul 19, 2013

Contributor

Should this be using public_send instead? The attribute being referenced shouldn't ever be private.

@sorentwo

This comment has been minimized.

Contributor

sorentwo commented Jul 19, 2013

It looks like this gains concurrency protection when the same using the same object, but I don't see where it would protect against concurrent issues on a threaded system. ActiveRecord isn't identity mapped (by default), so multiple instances could potentially change counters at the same time.

Aside from that I like that the methods were moved off of the class.

@thedarkone

This comment has been minimized.

Contributor

thedarkone commented Jul 19, 2013

@sorentwo I think this is about DB concurrency not ruby-land AR thread-safety.

@bogdan

This comment has been minimized.

Contributor

bogdan commented Jul 19, 2013

This PR is about DB concurrency of course.
Ruby level can only be solved with identity map but this is another story.

changed send to public_send as @sorentwo suggested.

@sorentwo

This comment has been minimized.

Contributor

sorentwo commented Jul 19, 2013

That makes a lot more sense.

@tamird

This comment has been minimized.

Contributor

tamird commented Sep 24, 2014

This LGTM. @bogdan, could you rebase it?

@bogdan bogdan force-pushed the bogdan:increment-concurency branch Sep 26, 2014

@bogdan

This comment has been minimized.

Contributor

bogdan commented Sep 26, 2014

@tamird done

update_counter_in_memory(difference, reflection)
end
def update_counter_in_database(difference, reflection = reflection())

This comment has been minimized.

@tamird

tamird Sep 26, 2014

Contributor

how come this method is removed while the equivalent in has_many_through_association was kept?

This comment has been minimized.

@bogdan

bogdan Jan 23, 2015

Contributor

I simply moved method to the place where it is used: only in has_many_through_association but not has_many_association.

a2 = Account.find(accounts(:signals37).id)
a1.increment!(:credit_limit)
a2.increment!(:credit_limit)
assert_equal initial_credit + 2, a1.reload.credit_limit

This comment has been minimized.

@tamird

tamird Sep 26, 2014

Contributor

can we also verify that the in-memory objects have the right value (without a reload) after updating?

This comment has been minimized.

@bogdan

bogdan Jan 23, 2015

Contributor

This patch doesn't fix that. We can not make everything perfect from one patch. I would prefer to apply it afterwards.

@tamird

This comment has been minimized.

Contributor

tamird commented Sep 26, 2014

Do you think this could be implemented using optimistic locking? In other words, we should be able to go through the optimistic locking codepath to obtain the guarantees we want here. (optimistically lock on what we think is the original value of the counter).

@tamird

This comment has been minimized.

Contributor

tamird commented Sep 26, 2014

@bogdan I think the safest thing to do is kill these methods with fire, since they are so dangerous and provide hardly any value at all. #17073

@bogdan

This comment has been minimized.

Contributor

bogdan commented Sep 27, 2014

World is dangerous place.

These methods used in rails internals anyway and I need them in my project anyway.
Killing this methods will not kill the fact that we need to update counter at instance level with updating it in database. It will just make every person to reinvent the pattern that already exists in Rails.

And the danger is what I am trying to fix here and make it safe for user in the same way it is safe in rails internally.

Of course, it is not 100% safeness. But Rails never was that safe to use. Danger exists in many methods because we didn't figure out how to make it better yet.

Check "Concurency and Integrity" paragraph here: http://apidock.com/rails/ActiveRecord/Validations/ClassMethods/validates_uniqueness_of.

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Jan 22, 2015

@jeremy WDYT?

bogdan referenced this pull request Jan 26, 2015

Improve consistency of counter caches updating in memory
When we made sure that the counter gets updated in memory, we only did
it on the has many side. The has many side only does the update if the
belongs to cannot. The belongs to side was updated to update the counter
cache (if it is able). This means that we need to check if the
belongs_to is able to update in memory on the has_many side.

We also found an inconsistency where the reflection names were used to
grab the association which should update the counter cache. Since
reflection names are now strings, this means it was using a different
instance than the one which would have the inverse instance set.

Fixes #18689

[Sean Griffin & anthonynavarre]

@bogdan bogdan force-pushed the bogdan:increment-concurency branch Jan 27, 2015

@bogdan bogdan force-pushed the bogdan:increment-concurency branch Oct 3, 2015

@bogdan

This comment has been minimized.

Contributor

bogdan commented Oct 3, 2015

Rebased against master.

a2.increment!(:credit_limit)
assert_equal initial_credit + 2, a1.reload.credit_limit
end

This comment has been minimized.

@jeremy

jeremy Oct 3, 2015

Member

"test increment concurrency" needs to be more precise so future readers of this code understand the guarantee we're making. This is testing the I in ACID, that #increment! isn't vulnerable to stale reads.

Does this single test really cover all the code changes above? It's difficult to determine but seems unlikely.

This comment has been minimized.

@bogdan

bogdan Oct 5, 2015

Contributor

@jeremy Maybe the changes fix some bugs when counter cache is only updated in DB but not on instance, but it is hard to assume where it happens and if it really happens. We have no identity map and we can not be sure we updated all instances in memory with a proper counter value.

So I would consider all these changes a refactoring that just reuses code.

Renamed test name to be more precise about use case.

@bogdan bogdan force-pushed the bogdan:increment-concurency branch to 85a1c02 Oct 5, 2015

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Oct 8, 2015

r? @jeremy

@jeremy jeremy merged commit 85a1c02 into rails:master Oct 10, 2015

jeremy added a commit that referenced this pull request Oct 10, 2015

Merge pull request #11410 from bogdan/increment-concurency
Make AR#increment! and #decrement! concurrency-safe
change = public_send(attribute) - (attribute_was(attribute.to_s) || 0)
self.class.update_counters(id, attribute => change)
clear_attribute_change(attribute) # eww
self

This comment has been minimized.

@aripollak

aripollak Oct 16, 2015

Contributor

I think this changes the behavior from the documentation, which says that these methods return true on success, but now it seems to always return self?
EDIT: the documentation also says that it passes through the attribute setter, which doesn't seem to be the case either.

This comment has been minimized.

@jeremy

akihiro17 added a commit to akihiro17/rails that referenced this pull request Oct 17, 2015

Update timestamp in AR#increment!
Now, AR#increment! is concurrent safe(rails#11410). However, this commit breaks backward compatability because `.update_counters` doesn't update timestamp.

Here is a reproduction script for this issue.
https://gist.github.com/akihiro17/aa08276f5d0f72d9869b

In this commit, `.update_counters` updates timestamp if both `touch` and `record_time_stamp` are true.

@zillou zillou referenced this pull request Sep 7, 2016

Closed

Rails 5 compatibility #382

1 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment