aboutsummaryrefslogtreecommitdiffstats
path: root/activerecord
diff options
context:
space:
mode:
authorJeremy Daer <jeremydaer@gmail.com>2015-10-10 13:24:54 -0700
committerJeremy Daer <jeremydaer@gmail.com>2015-10-10 13:24:54 -0700
commitf50d953ff646b84a1c791a8034cfb4f3789fc8bf (patch)
tree218d7f2053b315f5274e36f356d5ebdc0a9addea /activerecord
parentc61826eebcecd62e021cda4086dbcc1497820ac1 (diff)
parent85a1c025081faf8af8a065e39ee764caebf4054b (diff)
downloadrails-f50d953ff646b84a1c791a8034cfb4f3789fc8bf.tar.gz
rails-f50d953ff646b84a1c791a8034cfb4f3789fc8bf.tar.bz2
rails-f50d953ff646b84a1c791a8034cfb4f3789fc8bf.zip
Merge pull request #11410 from bogdan/increment-concurency
Make AR#increment! and #decrement! concurrency-safe
Diffstat (limited to 'activerecord')
-rw-r--r--activerecord/CHANGELOG.md5
-rw-r--r--activerecord/lib/active_record/associations/belongs_to_association.rb46
-rw-r--r--activerecord/lib/active_record/associations/has_many_association.rb10
-rw-r--r--activerecord/lib/active_record/persistence.rb12
-rw-r--r--activerecord/test/cases/persistence_test.rb9
5 files changed, 42 insertions, 40 deletions
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 1ae41facd2..ad65efc7bc 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,8 @@
+* Concurrent `AR::Base#increment!` and `#decrement!` on the same record
+ are all reflected in the database rather than overwriting each other.
+
+ *Bogdan Gusiev*
+
* Avoid leaking the first relation we call `first` on, per model.
Fixes #21921.
diff --git a/activerecord/lib/active_record/associations/belongs_to_association.rb b/activerecord/lib/active_record/associations/belongs_to_association.rb
index 260a0c6a2d..41698c5360 100644
--- a/activerecord/lib/active_record/associations/belongs_to_association.rb
+++ b/activerecord/lib/active_record/associations/belongs_to_association.rb
@@ -10,7 +10,7 @@ module ActiveRecord
def replace(record)
if record
raise_on_type_mismatch!(record)
- update_counters(record)
+ update_counters_on_replace(record)
replace_keys(record)
set_inverse_instance(record)
@updated = true
@@ -32,45 +32,37 @@ module ActiveRecord
end
def decrement_counters # :nodoc:
- with_cache_name { |name| decrement_counter name }
+ update_counters(-1)
end
def increment_counters # :nodoc:
- with_cache_name { |name| increment_counter name }
+ update_counters(1)
end
private
- def find_target?
- !loaded? && foreign_key_present? && klass
- end
-
- def with_cache_name
- counter_cache_name = reflection.counter_cache_column
- return unless counter_cache_name && owner.persisted?
- yield counter_cache_name
+ def update_counters(by)
+ if require_counter_update? && foreign_key_present?
+ if target && !stale_target?
+ target.increment!(reflection.counter_cache_column, by)
+ else
+ klass.update_counters(target_id, reflection.counter_cache_column => by)
+ end
+ end
end
- def update_counters(record)
- with_cache_name do |name|
- return unless different_target? record
- record.class.increment_counter(name, record.id)
- decrement_counter name
- end
+ def find_target?
+ !loaded? && foreign_key_present? && klass
end
- def decrement_counter(counter_cache_name)
- if foreign_key_present?
- klass.decrement_counter(counter_cache_name, target_id)
- end
+ def require_counter_update?
+ reflection.counter_cache_column && owner.persisted?
end
- def increment_counter(counter_cache_name)
- if foreign_key_present?
- klass.increment_counter(counter_cache_name, target_id)
- if target && !stale_target?
- target.increment(counter_cache_name)
- end
+ def update_counters_on_replace(record)
+ if require_counter_update? && different_target?(record)
+ record.increment!(reflection.counter_cache_column)
+ decrement_counters
end
end
diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
index 7da20d8eea..a9f6aaafef 100644
--- a/activerecord/lib/active_record/associations/has_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_association.rb
@@ -88,21 +88,15 @@ module ActiveRecord
end
def update_counter(difference, reflection = reflection())
- update_counter_in_database(difference, reflection)
- update_counter_in_memory(difference, reflection)
- end
-
- def update_counter_in_database(difference, reflection = reflection())
if reflection.has_cached_counter?
- owner.class.update_counters(owner.id, reflection.counter_cache_column => difference)
+ owner.increment!(reflection.counter_cache_column, difference)
end
end
def update_counter_in_memory(difference, reflection = reflection())
if reflection.counter_must_be_updated_by_has_many?
counter = reflection.counter_cache_column
- owner[counter] ||= 0
- owner[counter] += difference
+ owner.increment(counter, difference)
owner.send(:clear_attribute_change, counter) # eww
end
end
diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb
index 3f02f73a5a..f659d0b10f 100644
--- a/activerecord/lib/active_record/persistence.rb
+++ b/activerecord/lib/active_record/persistence.rb
@@ -332,16 +332,18 @@ module ActiveRecord
# 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 = public_send(attribute) - (attribute_was(attribute.to_s) || 0)
+ self.class.update_counters(id, attribute => change)
+ clear_attribute_change(attribute) # eww
+ self
end
# Initializes +attribute+ to zero if +nil+ and subtracts the value passed as +by+ (default is 1).
# The decrement is performed directly on the underlying attribute, no setter is invoked.
# Only makes sense for number-based attributes. Returns +self+.
def decrement(attribute, by = 1)
- self[attribute] ||= 0
- self[attribute] -= by
- self
+ increment(attribute, -by)
end
# Wrapper around +decrement+ that saves the record. This method differs from
@@ -349,7 +351,7 @@ module ActiveRecord
# Saving is not subjected to validation checks. Returns +true+ if the
# record could be saved.
def decrement!(attribute, by = 1)
- decrement(attribute, by).update_attribute(attribute, self[attribute])
+ increment!(attribute, -by)
end
# Assigns to +attribute+ the boolean opposite of <tt>attribute?</tt>. So
diff --git a/activerecord/test/cases/persistence_test.rb b/activerecord/test/cases/persistence_test.rb
index 7f14082a9a..31686bde3f 100644
--- a/activerecord/test/cases/persistence_test.rb
+++ b/activerecord/test/cases/persistence_test.rb
@@ -120,6 +120,15 @@ class PersistenceTest < ActiveRecord::TestCase
assert_equal 59, accounts(:signals37, :reload).credit_limit
end
+ def test_increment_updates_counter_in_db_using_offset
+ a1 = accounts(:signals37)
+ initial_credit = a1.credit_limit
+ a2 = Account.find(accounts(:signals37).id)
+ a1.increment!(:credit_limit)
+ a2.increment!(:credit_limit)
+ assert_equal initial_credit + 2, a1.reload.credit_limit
+ end
+
def test_destroy_all
conditions = "author_name = 'Mary'"
topics_by_mary = Topic.all.merge!(:where => conditions, :order => 'id').to_a