Deprecate committing a transaction exited with return or throw (#29333)

If a transaction is wrapped in a Timeout.timeout(duration) block, then the
transaction will be committed when the transaction block is exited from the
timeout, since it uses `throw`. Ruby code doesn't have a way to distinguish
between a block being exited from a `return`, `break` or `throw`, so
fixing this problem for the case of `throw` would require a backwards
incompatible change for block exited with `return` or `break`. As such,
the current behaviour so it can be changed in the future.

activerecord/CHANGELOG.md

+*   Deprecate using `return`, `break` or `throw` to exit a transaction block
+
+    *Dylan Thacker-Smith*
+
 *   Raise error when non-existent enum used in query.
 
     This change will raise an error when a non-existent enum is passed to a query.

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

@@ -292,7 +292,9 @@ def rollback_transaction(transaction = nil)
       def within_new_transaction(isolation: nil, joinable: true)
         @connection.lock.synchronize do
           transaction = begin_transaction(isolation: isolation, joinable: joinable)
-          yield
+          ret = yield
+          completed = true
+          ret
         rescue Exception => error
           if transaction
             rollback_transaction
@@ -304,6 +306,16 @@ def within_new_transaction(isolation: nil, joinable: true)
             if Thread.current.status == "aborting"
               rollback_transaction
             else
+              unless completed
+                ActiveSupport::Deprecation.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 raise and rollback.
+                EOW
+              end
               begin
                 commit_transaction
               rescue Exception

activerecord/test/cases/transactions_test.rb

@@ -139,7 +139,9 @@ def test_successful_with_return
     end
 
     assert_not_called(@first, :committed!) do
-      transaction_with_return
+      assert_deprecated do
+        transaction_with_return
+      end
     end
     assert committed
 
@@ -152,6 +154,21 @@ def test_successful_with_return
     end
   end
 
+  def test_deprecation_on_ruby_timeout
+    assert_deprecated do
+      catch do |timeout|
+        Topic.transaction do
+          @first.approved = true
+          @first.save!
+
+          throw timeout
+        end
+      end
+    end
+
+    assert Topic.find(1).approved?, "First should have been approved"
+  end
+
   def test_number_of_transactions_in_commit
     num = nil