Skip to content

Commit

Permalink
Simplify the implementation by only calling the checks in the right p…
Browse files Browse the repository at this point in the history 
…laces
  • Loading branch information
@rafaelfranca
rafaelfranca committed Jun 17, 2021
1 parent d9f2453 commit 115d4a3
Show file tree
Hide file tree
Showing 5 changed files with 61 additions and 73 deletions.
1 change: 1 addition & 0 deletions Gemfile.lock
View file
Original file line number Diff line number Diff line change
Expand Up @@ -524,6 +524,7 @@ GEM
zeitwerk (2.4.2)

PLATFORMS
ruby
x86_64-darwin-19
x86_64-darwin-20
x86_64-linux
Expand Down
4 changes: 4 additions & 0 deletions activerecord/CHANGELOG.md
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
* Do not try to rollback transactions that failed due to a `ActiveRecord::TransactionRollbackError`.

*Jamie McCarthy*

* Active Record Encryption will now encode values as UTF-8 when using deterministic
encryption. The encoding is part of the encrypted payload, so different encodings for
different values result in different ciphertexts. This can break unique constraints and
Expand Down
36 changes: 10 additions & 26 deletions activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,6 @@ def invalidated?
@state == :invalidated
end

def uncommittable?
invalidated? || rolledback?
end

def fully_completed?
completed?
end
Expand All @@ -59,6 +55,11 @@ def full_rollback!
@state = :fully_rolledback
end

def invalidate!
@children&.each { |c| c.invalidate! }
@state = :invalidated
end

def commit!
@state = :committed
end
Expand All @@ -67,11 +68,6 @@ def full_commit!
@state = :fully_committed
end

def invalidate!
@children.each { |c| c.invalidate! }
@state = :invalidated
end

def nullify!
@state = nil
end
Expand Down Expand Up @@ -166,10 +162,6 @@ def commit_records
ite&.each { |i| i.committed!(should_run_callbacks: false) }
end

def invalidate
@state.invalidate!
end

def full_rollback?; true; end
def joinable?; @joinable; end
def closed?; false; end
Expand All @@ -195,14 +187,11 @@ def materialize!
end

def rollback
connection.rollback_to_savepoint(savepoint_name) if materialized? && !state.invalidated?
connection.rollback_to_savepoint(savepoint_name) if materialized?
@state.rollback!
end

def commit
if state.invalidated?
raise ActiveRecord::StatementInvalid, "cannot commit after transaction invalidated"
end
connection.release_savepoint(savepoint_name) if materialized?
@state.commit!
end
Expand All @@ -222,14 +211,11 @@ def materialize!
end

def rollback
connection.rollback_db_transaction if materialized? && !state.invalidated?
connection.rollback_db_transaction if materialized?
@state.full_rollback!
end

def commit
if state.invalidated?
raise ActiveRecord::StatementInvalid, "cannot commit after transaction invalidated"
end
connection.commit_db_transaction if materialized?
@state.full_commit!
end
Expand Down Expand Up @@ -307,9 +293,6 @@ def materialize_transactions
def commit_transaction
@connection.lock.synchronize do
transaction = @stack.last
if transaction.state.invalidated?
raise ActiveRecord::StatementInvalid, "cannot commit after transaction invalidated"
end

begin
transaction.before_commit_records
Expand Down Expand Up @@ -342,12 +325,13 @@ def within_new_transaction(isolation: nil, joinable: true)
rollback_transaction
after_failure_actions(transaction, error)
end

raise
ensure
if transaction
if error
# @connection still holds an open transaction, so we must not
# put it back in the pool for reuse
# @connection still holds an open or invalid transaction, so we must not
# put it back in the pool for reuse.
@connection.throw_away! unless transaction.state.rolledback?
else
if Thread.current.status == "aborting"
Expand Down
14 changes: 7 additions & 7 deletions activerecord/test/cases/adapters/mysql2/nested_deadlock_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,12 @@ class Sample < ActiveRecord::Base

setup do
@abort, Thread.abort_on_exception = Thread.abort_on_exception, false
Thread.report_on_exception, @original_report_on_exception = false, Thread.report_on_exception

@connection = ActiveRecord::Base.connection
@connection.clear_cache!

@connection.drop_table "samples", if_exists: true
@connection.create_table("samples") do |t|
@connection.create_table("samples", force: true) do |t|
t.integer "value"
end

Expand All @@ -30,6 +30,7 @@ class Sample < ActiveRecord::Base
@connection.drop_table "samples", if_exists: true

Thread.abort_on_exception = @abort
Thread.report_on_exception = @original_report_on_exception
end

test "deadlock correctly raises Deadlocked inside nested SavepointTransaction" do
Expand All @@ -45,7 +46,7 @@ class Sample < ActiveRecord::Base
Sample.transaction(requires_new: true) do
s1.lock!
barrier.wait
s2.update_attributes value: 1
s2.update value: 1
end
end
end
Expand All @@ -55,16 +56,15 @@ class Sample < ActiveRecord::Base
Sample.transaction(requires_new: true) do
s2.lock!
barrier.wait
s1.update_attributes value: 2
s1.update value: 2
end
end
ensure
thread.join
end

rescue ActiveRecord::StatementInvalid => e
if /SAVEPOINT active_record_. does not exist: ROLLBACK TO SAVEPOINT/ =~ e.to_s
assert nil, "ROLLBACK TO SAVEPOINT query issued for savepoint that no longer exists due to deadlock: #{e}"
if /SAVEPOINT active_record_. does not exist/ =~ e.to_s
flunk "ROLLBACK TO SAVEPOINT query issued for savepoint that no longer exists due to deadlock: #{e}"
else
raise e
end
Expand Down
79 changes: 39 additions & 40 deletions activerecord/test/cases/adapters/postgresql/transaction_nested_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,15 @@ class Sample < ActiveRecord::Base

setup do
@abort, Thread.abort_on_exception = Thread.abort_on_exception, false
Thread.report_on_exception, @original_report_on_exception = false, Thread.report_on_exception

@connection = ActiveRecord::Base.connection

@connection.drop_table "samples", if_exists: true
@connection.create_table("samples") do |t|
t.integer "value"
@connection.transaction do
@connection.drop_table "samples", if_exists: true
@connection.create_table("samples") do |t|
t.integer "value"
end
end

Sample.reset_column_information
Expand All @@ -29,39 +32,38 @@ class Sample < ActiveRecord::Base
@connection.drop_table "samples", if_exists: true

Thread.abort_on_exception = @abort
Thread.report_on_exception = @original_report_on_exception
end

test "unserializable transaction raises SerializationFailure inside nested SavepointTransaction" do
assert_raises(ActiveRecord::SerializationFailure) do
before = Concurrent::CyclicBarrier.new(2)
after = Concurrent::CyclicBarrier.new(2)

begin
thread = Thread.new do
with_warning_suppression do
Sample.transaction(isolation: :serializable, requires_new: false) do
Sample.transaction(requires_new: true) do
before.wait
Sample.create value: Sample.sum(:value)
after.wait
end
thread = Thread.new do
with_warning_suppression do
Sample.transaction(isolation: :serializable, requires_new: false) do
Sample.transaction(requires_new: true) do
before.wait
Sample.create value: Sample.sum(:value)
after.wait
end
end
end
end

begin
with_warning_suppression do
Sample.transaction(isolation: :serializable, requires_new: false) do
Sample.transaction(requires_new: true) do
before.wait
Sample.create value: Sample.sum(:value)
after.wait
end
begin
with_warning_suppression do
Sample.transaction(isolation: :serializable, requires_new: false) do
Sample.transaction(requires_new: true) do
before.wait
Sample.create value: Sample.sum(:value)
after.wait
end
end
ensure
thread.join
end
ensure
thread.join
end
end
end
Expand All @@ -74,35 +76,32 @@ class Sample < ActiveRecord::Base
s1 = Sample.create value: 1
s2 = Sample.create value: 2

begin
thread = Thread.new do
Sample.transaction(requires_new: false) do
Sample.transaction(requires_new: true) do
s1.lock!
barrier.wait
s2.update_attributes value: 1
end
thread = Thread.new do
Sample.transaction(requires_new: false) do
Sample.transaction(requires_new: true) do
s1.lock!
barrier.wait
s2.update value: 1
end
end
end

begin
Sample.transaction(requires_new: false) do
Sample.transaction(requires_new: true) do
s2.lock!
barrier.wait
s1.update_attributes value: 2
end
begin
Sample.transaction(requires_new: false) do
Sample.transaction(requires_new: true) do
s2.lock!
barrier.wait
s1.update value: 2
end
ensure
thread.join
end
ensure
thread.join
end
end
end
end

private

def with_warning_suppression
log_level = ActiveRecord::Base.connection.client_min_messages
ActiveRecord::Base.connection.client_min_messages = "error"
Expand Down

0 comments on commit 115d4a3

Please sign in to comment.