Active Record commit transaction on return, break and throw

Gemfile.lock

@@ -74,6 +74,7 @@ PATH
     activerecord (7.1.0.alpha)
       activemodel (= 7.1.0.alpha)
       activesupport (= 7.1.0.alpha)
+      timeout (>= 0.4.0)
     activestorage (7.1.0.alpha)
       actionpack (= 7.1.0.alpha)
       activejob (= 7.1.0.alpha)
@@ -334,7 +335,7 @@ GEM
     mysql2 (0.5.4)
     net-http-persistent (4.0.1)
       connection_pool (~> 2.2)
-    net-imap (0.3.4)
+    net-imap (0.3.6)
       date
       net-protocol
     net-pop (0.1.2)
@@ -511,7 +512,7 @@ GEM
     terser (1.1.13)
       execjs (>= 0.3.0, < 3)
     thor (1.2.2)
-    timeout (0.3.2)
+    timeout (0.4.0)
     tomlrb (2.0.3)
     trailblazer-option (0.1.2)
     turbo-rails (1.3.2)

activerecord/CHANGELOG.md

@@ -1,3 +1,34 @@
+*   Bring back the historical behavior of committing transaction on non-local return.
+
+    ```ruby
+    Model.transaction do
+      model.save
+      return
+      other_model.save # not executed
+    end
+    ```
+
+    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.
+
+    This historical behavior can now be opt-ed in via:
+
+    ```
+    Rails.application.config.active_record.commit_transaction_on_non_local_return = true
+    ```
+
+    And is the default for new applications created in Rails 7.1.
+
+    *Jean Boussier*
+
 *   Deprecate `name` argument on `#remove_connection`.
 
     The `name` argument is deprecated on `#remove_connection` without replacement. `#remove_connection` should be called directly on the class that established the connection.

activerecord/activerecord.gemspec

@@ -37,4 +37,5 @@ Gem::Specification.new do |s|
 
   s.add_dependency "activesupport", version
   s.add_dependency "activemodel",   version
+  s.add_dependency "timeout", ">= 0.4.0"
 end

activerecord/lib/active_record.rb

@@ -320,6 +320,9 @@ 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
+
   ##
   # :singleton-method:
   # Specify a threshold for the size of query result sets. If the number of

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

@@ -90,7 +90,7 @@ def invalidate!; end
 
     class Transaction # :nodoc:
       attr_reader :connection, :state, :savepoint_name, :isolation_level
-      attr_accessor :written, :written_indirectly
+      attr_accessor :written
 
       delegate :invalidate!, :invalidated?, to: :@state
 
@@ -463,10 +463,6 @@ def commit_transaction
 
           dirty_current_transaction if transaction.dirty?
 
-          if current_transaction.open?
-            current_transaction.written_indirectly ||= transaction.written || transaction.written_indirectly
-          end
-
           transaction.commit
           transaction.commit_records
         end
@@ -498,30 +494,25 @@ 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
-                # This was deprecated in 6.1, and has now changed to a rollback
-                rollback_transaction
-              elsif !completed && !transaction.written_indirectly
-                # This was a silent commit in 6.1, but now becomes a rollback; we skipped
-                # the warning because (having not been written) the change generally won't
-                # have any effect
+                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
-                if !completed && transaction.written_indirectly
-                  # This is the case that was missed in the 6.1 deprecation, so we have to
-                  # do it now
-                  ActiveRecord.deprecator.warn(<<~EOW)
-                    Using `return`, `break` or `throw` to exit a transaction block is
-                    deprecated without replacement. If the `throw` came from
-                    `Timeout.timeout(duration)`, pass an exception class as a second
-                    argument so it doesn't use `throw` to abort its block. This results
-                    in the transaction being committed, but in the next release of Rails
-                    it will rollback.
-                  EOW
-                end
-
                 begin
                   commit_transaction
                 rescue ActiveRecord::ConnectionFailed

activerecord/test/cases/transactions_test.rb

@@ -17,6 +17,11 @@ 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
@@ -270,7 +275,7 @@ def test_successful_with_return_outside_inner_transaction
       end
     end
 
-    assert_deprecated(ActiveRecord.deprecator) do
+    assert_not_deprecated(ActiveRecord.deprecator) do
       transaction_with_shallow_return
     end
     assert committed
@@ -285,7 +290,7 @@ def test_successful_with_return_outside_inner_transaction
   end
 
   def test_deprecation_on_ruby_timeout_outside_inner_transaction
-    assert_deprecated(ActiveRecord.deprecator) do
+    assert_not_deprecated(ActiveRecord.deprecator) do
       catch do |timeout|
         Topic.transaction do
           Topic.transaction(requires_new: true) do
@@ -312,7 +317,9 @@ def test_rollback_with_return
       end
     end
 
-    transaction_with_return
+    assert_deprecated(ActiveRecord.deprecator) do
+      transaction_with_return
+    end
     assert_not committed
 
     assert_not_predicate Topic.find(1), :approved?
@@ -325,24 +332,76 @@ def test_rollback_with_return
   end
 
   def test_rollback_on_ruby_timeout
-    catch do |timeout|
-      Topic.transaction do
-        @first.approved = true
-        @first.save!
+    assert_deprecated(ActiveRecord.deprecator) do
+      catch do |timeout|
+        Topic.transaction do
+          @first.approved = true
+          @first.save!
 
-        throw timeout
+          throw timeout
+        end
       end
     end
 
     assert_not_predicate Topic.find(1), :approved?
   end
 
-  def test_early_return_from_transaction
-    assert_not_deprecated(ActiveRecord.deprecator) do
-      @first.with_lock do
-        break
+  def test_break_from_transaction_7_1_behavior
+    ActiveRecord.commit_transaction_on_non_local_return = true
+
+    @first.transaction do
+      assert_not_predicate @first, :approved?
+      @first.update!(approved: true)
+
+      break if true
+
+      # dead code
+      assert_predicate @first, :approved?
+      @first.update!(approved: false)
+    end
+
+    assert_predicate Topic.find(1), :approved?, "First should have been approved"
+    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
+
+    catch(:not_an_error) do
+      @first.transaction do
+        assert_not_predicate @first, :approved?
+        @first.update!(approved: true)
+
+        throw :not_an_error
+
+        # dead code
+        assert_predicate @first, :approved?
+        @first.update!(approved: false)
       end
     end
+    assert_predicate Topic.find(1), :approved?, "First should have been approved"
+    assert_predicate Topic.find(2), :approved?, "Second should have been approved"
+  end
+
+  def _test_return_from_transaction_7_1_behavior
+    @first.transaction do
+      assert_not_predicate @first, :approved?
+      @first.update!(approved: true)
+
+      return if true
+
+      # dead code
+      assert_predicate @first, :approved?
+      @first.update!(approved: false)
+    end
+  end
+
+  def test_return_from_transaction_7_1_behavior
+    ActiveRecord.commit_transaction_on_non_local_return = true
+
+    _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"
   end
 
   def test_number_of_transactions_in_commit

guides/source/configuring.md

@@ -69,6 +69,7 @@ Below are the default values associated with each target version. In cases of co
 - [`config.active_record.allow_deprecated_singular_associations_name`](#config-active-record-allow-deprecated-singular-associations-name): `false`
 - [`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`
@@ -1274,6 +1275,39 @@ 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

@@ -276,6 +276,7 @@ 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.allow_deprecated_singular_associations_name = false
             active_record.sqlite3_adapter_strict_strings_by_default = true
             active_record.query_log_tags_format = :sqlcommenter

railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_1.rb.tt

@@ -170,6 +170,10 @@
 # In previous versions of Rails, they ran in the inverse order.
 # Rails.application.config.active_record.run_after_transaction_callbacks_in_order_defined = true
 
+# Whether a `transaction` block is committed or rolled back when exited via `return`, `break` or `throw`.
+#
+# Rails.application.config.active_record.commit_transaction_on_non_local_return = true
+
 # ** Please read carefully, this must be configured in config/application.rb **
 # Change the format of the cache entry.
 # Changing this default means that all new cache entries added to the cache