aboutsummaryrefslogtreecommitdiffstats
path: root/activerecord/lib
diff options
context:
space:
mode:
authorRyuta Kamizono <kamipo@gmail.com>2019-05-03 13:36:44 +0900
committerRyuta Kamizono <kamipo@gmail.com>2019-05-07 02:19:57 +0900
commit718a32ca745672a977a0d4ae401f61f439767405 (patch)
tree048ba67fc584728771525b5d273b5695f13f2ea4 /activerecord/lib
parent020856c328085ffe4728afdab99e9008cc86d9e6 (diff)
downloadrails-718a32ca745672a977a0d4ae401f61f439767405.tar.gz
rails-718a32ca745672a977a0d4ae401f61f439767405.tar.bz2
rails-718a32ca745672a977a0d4ae401f61f439767405.zip
Should attempt `committed!`/`rolledback!` to all enrolled records in the transaction
Currently, `committed!`/`rolledback!` will only be attempted for the first enrolled record in the transaction, that will cause some problematic behaviors. The first one problem, `clear_transaction_record_state` won't be called even if the transaction is finalized except the first enrolled record. This means that de-duplicated records in the transaction won't refer latest state (e.g. won't happen rolling back record state). The second one problem, the enrolled order is not always the same as the order in which the actions actually happened, the first enrolled record may succeed no actions (e.g. `destroy` has already succeeded on another record during `before_destroy`), it will lose to fire any transactional callbacks. To avoid both problems, we should attempt `committed!`/`rolledback!` to all enrolled records in the transaction.
Diffstat (limited to 'activerecord/lib')
-rw-r--r--activerecord/lib/active_record/connection_adapters/abstract/transaction.rb16
-rw-r--r--activerecord/lib/active_record/touch_later.rb1
-rw-r--r--activerecord/lib/active_record/transactions.rb16
3 files changed, 20 insertions, 13 deletions
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
index c9e84e48cc..7b6321d83b 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
@@ -98,9 +98,13 @@ module ActiveRecord
end
def rollback_records
- ite = records.uniq
+ ite = records.uniq(&:object_id)
+ already_run_callbacks = {}
while record = ite.shift
- record.rolledback!(force_restore_state: full_rollback?)
+ trigger_callbacks = record.trigger_transactional_callbacks?
+ should_run_callbacks = !already_run_callbacks[record] && trigger_callbacks
+ already_run_callbacks[record] ||= trigger_callbacks
+ record.rolledback!(force_restore_state: full_rollback?, should_run_callbacks: should_run_callbacks)
end
ensure
ite.each do |i|
@@ -113,10 +117,14 @@ module ActiveRecord
end
def commit_records
- ite = records.uniq
+ ite = records.uniq(&:object_id)
+ already_run_callbacks = {}
while record = ite.shift
if @run_commit_callbacks
- record.committed!
+ trigger_callbacks = record.trigger_transactional_callbacks?
+ should_run_callbacks = !already_run_callbacks[record] && trigger_callbacks
+ already_run_callbacks[record] ||= trigger_callbacks
+ record.committed!(should_run_callbacks: should_run_callbacks)
else
# if not running callbacks, only adds the record to the parent transaction
connection.add_transaction_record(record)
diff --git a/activerecord/lib/active_record/touch_later.rb b/activerecord/lib/active_record/touch_later.rb
index b60cc96165..6872b7844e 100644
--- a/activerecord/lib/active_record/touch_later.rb
+++ b/activerecord/lib/active_record/touch_later.rb
@@ -18,6 +18,7 @@ module ActiveRecord
surreptitiously_touch @_defer_touch_attrs
add_to_transaction
+ @_new_record_before_last_commit ||= false
# touch the parents as we are not calling the after_save callbacks
self.class.reflect_on_all_associations(:belongs_to).each do |r|
diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb
index 9e03deede7..148bc0550c 100644
--- a/activerecord/lib/active_record/transactions.rb
+++ b/activerecord/lib/active_record/transactions.rb
@@ -333,7 +333,7 @@ module ActiveRecord
# Ensure that it is not called if the object was never persisted (failed create),
# but call it after the commit of a destroyed object.
def committed!(should_run_callbacks: true) #:nodoc:
- if should_run_callbacks && trigger_transactional_callbacks?
+ if should_run_callbacks
@_committed_already_called = true
_run_commit_without_transaction_enrollment_callbacks
_run_commit_callbacks
@@ -346,7 +346,7 @@ module ActiveRecord
# Call the #after_rollback callbacks. The +force_restore_state+ argument indicates if the record
# state should be rolled back to the beginning or just to the last savepoint.
def rolledback!(force_restore_state: false, should_run_callbacks: true) #:nodoc:
- if should_run_callbacks && trigger_transactional_callbacks?
+ if should_run_callbacks
_run_rollback_callbacks
_run_rollback_without_transaction_enrollment_callbacks
end
@@ -378,6 +378,11 @@ module ActiveRecord
status
end
+ def trigger_transactional_callbacks? # :nodoc:
+ (@_new_record_before_last_commit || _trigger_update_callback) && persisted? ||
+ _trigger_destroy_callback && destroyed?
+ end
+
private
attr_reader :_committed_already_called, :_trigger_update_callback, :_trigger_destroy_callback
@@ -392,10 +397,7 @@ module ActiveRecord
level: 0
}
@_start_transaction_state[:level] += 1
- remember_new_record_before_last_commit
- end
- def remember_new_record_before_last_commit
if _committed_already_called
@_new_record_before_last_commit = false
else
@@ -457,10 +459,6 @@ module ActiveRecord
self.class.connection.add_transaction_record(self)
end
- def trigger_transactional_callbacks?
- @_new_record_before_last_commit && !new_record? || _trigger_update_callback || _trigger_destroy_callback
- end
-
def has_transactional_callbacks?
!_rollback_callbacks.empty? || !_commit_callbacks.empty? || !_before_commit_callbacks.empty?
end