Merge pull request #11410 from bogdan/increment-concurency

Make AR#increment! and #decrement! concurrency-safe

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