From 115d4a3994a92abc5010ad074d44e68c24debc21 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Wed, 16 Jun 2021 21:17:34 +0000 Subject: [PATCH] Simplify the implementation by only calling the checks in the right places --- Gemfile.lock | 1 + activerecord/CHANGELOG.md | 4 + .../abstract/transaction.rb | 36 +++------ .../adapters/mysql2/nested_deadlock_test.rb | 14 ++-- .../postgresql/transaction_nested_test.rb | 79 +++++++++---------- 5 files changed, 61 insertions(+), 73 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 6d356d9c587..6aae1e9963f 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -524,6 +524,7 @@ GEM zeitwerk (2.4.2) PLATFORMS + ruby x86_64-darwin-19 x86_64-darwin-20 x86_64-linux diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 120830a2a3e..9e77a5e6441 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -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 diff --git a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb index bb127615953..d26e869d08b 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb @@ -37,10 +37,6 @@ module ActiveRecord @state == :invalidated end - def uncommittable? - invalidated? || rolledback? - end - def fully_completed? completed? end @@ -59,6 +55,11 @@ module ActiveRecord @state = :fully_rolledback end + def invalidate! + @children&.each { |c| c.invalidate! } + @state = :invalidated + end + def commit! @state = :committed end @@ -67,11 +68,6 @@ module ActiveRecord @state = :fully_committed end - def invalidate! - @children.each { |c| c.invalidate! } - @state = :invalidated - end - def nullify! @state = nil end @@ -166,10 +162,6 @@ module ActiveRecord 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 @@ -195,14 +187,11 @@ module ActiveRecord 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 @@ -222,14 +211,11 @@ module ActiveRecord 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 @@ -307,9 +293,6 @@ module ActiveRecord 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 @@ -342,12 +325,13 @@ module ActiveRecord 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" diff --git a/activerecord/test/cases/adapters/mysql2/nested_deadlock_test.rb b/activerecord/test/cases/adapters/mysql2/nested_deadlock_test.rb index 60a9651e5a8..955656b89ea 100644 --- a/activerecord/test/cases/adapters/mysql2/nested_deadlock_test.rb +++ b/activerecord/test/cases/adapters/mysql2/nested_deadlock_test.rb @@ -13,12 +13,12 @@ module ActiveRecord 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 @@ -30,6 +30,7 @@ module ActiveRecord @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 @@ -45,7 +46,7 @@ module ActiveRecord Sample.transaction(requires_new: true) do s1.lock! barrier.wait - s2.update_attributes value: 1 + s2.update value: 1 end end end @@ -55,16 +56,15 @@ module ActiveRecord 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 diff --git a/activerecord/test/cases/adapters/postgresql/transaction_nested_test.rb b/activerecord/test/cases/adapters/postgresql/transaction_nested_test.rb index 3b056adf943..5009e5cc77b 100644 --- a/activerecord/test/cases/adapters/postgresql/transaction_nested_test.rb +++ b/activerecord/test/cases/adapters/postgresql/transaction_nested_test.rb @@ -14,12 +14,15 @@ module ActiveRecord 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 @@ -29,6 +32,7 @@ module ActiveRecord @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 @@ -36,32 +40,30 @@ module ActiveRecord 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 @@ -74,35 +76,32 @@ module ActiveRecord 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"