Remove deprecated behavior that would rollback a transaction block when exited using return, break or throw.

Author: rafaelfranca

activerecord/CHANGELOG.md

@@ -1,3 +1,11 @@
+*   Remove deprecated behavior that would rollback a transaction block when exited using `return`, `break` or `throw`.
+
+    *Rafael Mendonça França*
+
+*   Deprecate `Rails.application.config.active_record.commit_transaction_on_non_local_return`.
+
+    *Rafael Mendonça França*
+
 *   Remove deprecated support to pass `rewhere` to `ActiveRecord::Relation#merge`.
 
     *Rafael Mendonça França*

activerecord/lib/active_record.rb

@@ -333,8 +333,19 @@ def self.global_executor_concurrency # :nodoc:
   singleton_class.attr_accessor :run_after_transaction_callbacks_in_order_defined
   self.run_after_transaction_callbacks_in_order_defined = false
 
-  singleton_class.attr_accessor :commit_transaction_on_non_local_return
-  self.commit_transaction_on_non_local_return = false
+  def self.commit_transaction_on_non_local_return
+    ActiveRecord.deprecator.warn <<-WARNING.squish
+      `Rails.application.config.active_record.commit_transaction_on_non_local_return`
+      is deprecated and will be removed in Rails 7.3.
+    WARNING
+  end
+
+  def self.commit_transaction_on_non_local_return=(value)
+    ActiveRecord.deprecator.warn <<-WARNING.squish
+      `Rails.application.config.active_record.commit_transaction_on_non_local_return`
+      is deprecated and will be removed in Rails 7.3.
+    WARNING
+  end
 
   ##
   # :singleton-method: warn_on_records_fetched_greater_than

activerecord/lib/active_record/connection_adapters/abstract/transaction.rb

@@ -534,7 +534,6 @@ def within_new_transaction(isolation: nil, joinable: true)
           transaction = begin_transaction(isolation: isolation, joinable: joinable)
           begin
             ret = yield
-            completed = true
             ret
           rescue Exception => error
             rollback_transaction
@@ -543,24 +542,8 @@ def within_new_transaction(isolation: nil, joinable: true)
             raise
           ensure
             unless error
-              # In 7.1 we enforce timeout >= 0.4.0 which no longer use throw, so we can
-              # go back to the original behavior of committing on non-local return.
-              # If users are using throw, we assume it's not an error case.
-              completed = true if ActiveRecord.commit_transaction_on_non_local_return
-
               if Thread.current.status == "aborting"
                 rollback_transaction
-              elsif !completed && transaction.written
-                ActiveRecord.deprecator.warn(<<~EOW)
-                  A transaction is being rolled back because the transaction block was
-                  exited using `return`, `break` or `throw`.
-                  In Rails 7.2 this transaction will be committed instead.
-                  To opt-in to the new behavior now and suppress this warning
-                  you can set:
-
-                    Rails.application.config.active_record.commit_transaction_on_non_local_return = true
-                EOW
-                rollback_transaction
               else
                 begin
                   commit_transaction

activerecord/test/cases/relation/merging_test.rb

@@ -150,12 +150,6 @@ def test_merge_doesnt_duplicate_same_clauses
       assert_queries_match(/WHERE \(#{Regexp.escape(author_id)} IN \('1'\)\)\z/) do
         assert_equal [david], only_david.merge(only_david)
       end
-
-      assert_queries_match(/WHERE \(#{Regexp.escape(author_id)} IN \('1'\)\)\z/) do
-        assert_deprecated(ActiveRecord.deprecator) do
-          assert_equal [david], only_david.merge(only_david, rewhere: true)
-        end
-      end
     else
       assert_queries_match(/WHERE \(#{Regexp.escape(author_id)} IN \(1\)\)\z/) do
         assert_equal [david], only_david.merge(only_david)

activerecord/test/cases/transactions_test.rb

@@ -17,11 +17,6 @@ class TransactionTest < ActiveRecord::TestCase
 
   def setup
     @first, @second = Topic.find(1, 2).sort_by(&:id)
-    @commit_transaction_on_non_local_return_was = ActiveRecord.commit_transaction_on_non_local_return
-  end
-
-  def teardown
-    ActiveRecord.commit_transaction_on_non_local_return = @commit_transaction_on_non_local_return_was
   end
 
   def test_rollback_dirty_changes
@@ -343,49 +338,7 @@ def test_deprecation_on_ruby_timeout_outside_inner_transaction
     assert_predicate Topic.find(1), :approved?, "First should have been approved"
   end
 
-  def test_rollback_with_return
-    committed = false
-
-    Topic.connection.class_eval do
-      alias :real_commit_db_transaction :commit_db_transaction
-      define_method(:commit_db_transaction) do
-        committed = true
-        real_commit_db_transaction
-      end
-    end
-
-    assert_deprecated(ActiveRecord.deprecator) do
-      transaction_with_return
-    end
-    assert_not committed
-
-    assert_not_predicate Topic.find(1), :approved?
-    assert_predicate Topic.find(2), :approved?
-  ensure
-    Topic.connection.class_eval do
-      remove_method :commit_db_transaction
-      alias :commit_db_transaction :real_commit_db_transaction rescue nil
-    end
-  end
-
-  def test_rollback_on_ruby_timeout
-    assert_deprecated(ActiveRecord.deprecator) do
-      catch do |timeout|
-        Topic.transaction do
-          @first.approved = true
-          @first.save!
-
-          throw timeout
-        end
-      end
-    end
-
-    assert_not_predicate Topic.find(1), :approved?
-  end
-
-  def test_break_from_transaction_7_1_behavior
-    ActiveRecord.commit_transaction_on_non_local_return = true
-
+  def test_break_from_transaction_commits
     @first.transaction do
       assert_not_predicate @first, :approved?
       @first.update!(approved: true)
@@ -401,9 +354,7 @@ def test_break_from_transaction_7_1_behavior
     assert_predicate Topic.find(2), :approved?, "Second should have been approved"
   end
 
-  def test_thow_from_transaction_7_1_behavior
-    ActiveRecord.commit_transaction_on_non_local_return = true
-
+  def test_throw_from_transaction_commits
     catch(:not_an_error) do
       @first.transaction do
         assert_not_predicate @first, :approved?
@@ -433,9 +384,7 @@ def _test_return_from_transaction_7_1_behavior
     end
   end
 
-  def test_return_from_transaction_7_1_behavior
-    ActiveRecord.commit_transaction_on_non_local_return = true
-
+  def test_return_from_transaction_commits
     _test_return_from_transaction_7_1_behavior
     assert_predicate Topic.find(1), :approved?, "First should have been approved"
     assert_predicate Topic.find(2), :approved?, "Second should have been approved"

guides/source/7_2_release_notes.md

@@ -164,10 +164,14 @@ Please refer to the [Changelog][active-record] for detailed changes.
 
 *   Remove deprecated support to pass `rewhere` to `ActiveRecord::Relation#merge`.
 
+*   Remove deprecated behavior that would rollback a transaction block when exited using `return`, `break` or `throw`.
+
 ### Deprecations
 
 *   Deprecate `Rails.application.config.active_record.allow_deprecated_singular_associations_name`
 
+*   Deprecate `Rails.application.config.active_record.commit_transaction_on_non_local_return`
+
 ### Notable changes
 
 Active Storage

guides/source/configuring.md

@@ -71,7 +71,6 @@ Below are the default values associated with each target version. In cases of co
 - [`config.action_view.sanitizer_vendor`](#config-action-view-sanitizer-vendor): `Rails::HTML::Sanitizer.best_supported_vendor`
 - [`config.active_record.before_committed_on_all_records`](#config-active-record-before-committed-on-all-records): `true`
 - [`config.active_record.belongs_to_required_validates_foreign_key`](#config-active-record-belongs-to-required-validates-foreign-key): `false`
-- [`config.active_record.commit_transaction_on_non_local_return`](#config-active-record-commit-transaction-on-non-local-return): `true`
 - [`config.active_record.default_column_serializer`](#config-active-record-default-column-serializer): `nil`
 - [`config.active_record.encryption.hash_digest_class`](#config-active-record-encryption-hash-digest-class): `OpenSSL::Digest::SHA256`
 - [`config.active_record.encryption.support_sha1_for_non_deterministic_encryption`](#config-active-record-encryption-support-sha1-for-non-deterministic-encryption): `false`
@@ -1322,39 +1321,6 @@ The default value depends on the `config.load_defaults` target version:
 | (original)            | `false`              |
 | 7.0                   | `true`               |
 
-#### `config.active_record.commit_transaction_on_non_local_return`
-
-Defines whether `return`, `break` and `throw` inside a `transaction` block cause the transaction to be
-committed or rolled back. e.g.:
-
-```ruby
-Model.transaction do
-  model.save
-  return
-  other_model.save # not executed
-end
-```
-
-If set to `false`, it will be rolled back.
-
-If set to `true`, the above transaction will be committed.
-
-| Starting with version | The default value is |
-| --------------------- | -------------------- |
-| (original)            | `false`              |
-| 7.1                   | `true`               |
-
-Historically only raised errors would trigger a rollback, but in Ruby `2.3`, the `timeout` library
-started using `throw` to interrupt execution which had the adverse effect of committing open transactions.
-
-To solve this, in Active Record 6.1 the behavior was changed to instead rollback the transaction as it was safer
-than to potentially commit an incomplete transaction.
-
-Using `return`, `break` or `throw` inside a `transaction` block was essentially deprecated from Rails 6.1 onwards.
-
-However with the release of `timeout 0.4.0`, `Timeout.timeout` now raises an error again, and Active Record is able
-to return to its original, less surprising, behavior.
-
 #### `config.active_record.raise_on_assign_to_attr_readonly`
 
 Enable raising on assignment to attr_readonly attributes. The previous

railties/lib/rails/application/configuration.rb

@@ -279,7 +279,6 @@ def load_defaults(target_version)
 
           if respond_to?(:active_record)
             active_record.run_commit_callbacks_on_first_saved_instances_in_transaction = false
-            active_record.commit_transaction_on_non_local_return = true
             active_record.sqlite3_adapter_strict_strings_by_default = true
             active_record.query_log_tags_format = :sqlcommenter
             active_record.raise_on_assign_to_attr_readonly = true