Deprecate committing a transaction exited with return or throw #29333

dylanahsmith · 2017-06-02T19:08:10Z

Problem

I found that we had transactions that were being committed because of the use of Timeout.timeout(duration) do wrapping database transactions. The reason why this was happening is that when Timeout.timeout isn't given a second argument (an exception class) then it uses throw and catch to exit the transaction block, after which Timeout.timeout will raise an exception. This why the documentation for Timeout.timeout says

The exception thrown to terminate the given block cannot be rescued inside the block unless klass is given explicitly.

This can be reproduced using ruby code like

Timeout.timeout(1) do
  Example.transaction do
    example.update_attributes(value: 1)
    sleep 3 # simulate something slow
  end
end

where the update will be committed even if the block gets a timeout exception.

The reason why this results in the transaction being committed is that fc83920 was added to commit a transaction when the transaction block is exited with return. Unfortunately, the ensure block can't distinguish between a block exited with return, break or throw, so we can't fix the problem with throw as used in Timeout.timeout without a backwards incompatible change.

Solution

Set a local variable completed = true after the yield so that we can detect when the ensure block didn't complete, then use a deprecation warning to try to let developers know that rails will rollback instead of commit in this case in the future.

That way we can fix this behaviour in the next release for the Timeout.timeout case. I will open a PR against master doing this.

rails-bot · 2017-06-02T19:08:15Z

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rafaelfranca (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

rails-bot · 2017-06-02T19:08:15Z

Warning

Pull requests are usually filed against the master branch for this repo, but this one is against 5-1-stable. Please double check that you specified the right target!

matthewd · 2017-06-02T23:03:15Z

We aren't going to add a deprecation to an existing release series; this needs to be directed to master.

rails will rollback instead of commit in this case in the future

That doesn't seem acceptable either. The reason for the previous change is that silently rolling back on a non-error return is also a hugely surprising behaviour.

dylanahsmith · 2017-06-03T01:22:40Z

Changed the base branch to master.

Would it be more acceptable to raise an exception along with rolling back in the ensure block in the Rails release following the deprecation so it doesn't silently support either return or throw as a way to exit the transaction block?

matthewd · 2017-06-03T07:57:45Z

Yeah, while unfortunate, I think that would at least avoid nasty surprises.

My preference would be to silently rollback on any throw (i.e., treating it like an exception), while silently committing if we're unwinding due to a return. That's basically the theory behind the current behaviour: no-one uses throw/catch, so a non-exception unwind is most likely a return. And that theory worked well for about 8 years. 🙈

Short of petitioning for a new language feature, the only option that comes to mind to distinguish them would be to monkey-patch Kernel#catch to keep track of what catches are currently in scope on the current thread -- and then "re-catch" them inside the transaction.

I wonder how terrible that would actually be...

rails-bot · 2019-12-18T05:52:55Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

dylanahsmith · 2019-12-18T19:00:33Z

I changed the description of the future behaviour in the exception message:

-                    in the next release of Rails it will instead rollback.
+                    in the next release of Rails it will raise and rollback.

and resolved merge conflicts

rails-bot · 2020-03-17T19:41:58Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

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.

eileencodes · 2020-05-26T19:41:11Z

We're working through this deprecation at GitHub and it feels like this is overreaching a bit. There are cases where you might want to return early and this deprecation reads like early returns will just start throwing exceptions in Rails 6.2.

How are these changes intended to work with the following:

with_lock do
  return if some_crtieria_met?

  # do work
end

Will this start raising in 6.2? I've read this PR a few times and we're having trouble figuring out if our use case is intended to be unsupported or this is just a deprecation that's not applicable for us. I'm a little worried this is confusing for upgrades in other apps as well.

dylanahsmith · 2020-05-26T21:31:57Z

That is an interesting use case, since in your early return example it doesn't really matter if the transaction is committed or rolled back since there hasn't been a write to the database. As a result, there is also no concern about ambiguity of whether the early return is supposed to mean the transaction should be committed or rolled back.

I'm more concerned about the case where the application writes to the database and then does an early return with the intention of committing the transaction, since we can't support that without also committing the transaction in the case of a timeout from a Timeout.timeout(duration) do block surrounding the transaction.

To support your use case, perhaps we should use write_query? on database queries in a transaction to detect the first write to it, then we could only give a deprecation warning where we need to make a breaking change.

The other question is whether we should even bother to raise an exception in Rails 6.2 in place of the deprecation warning after rolling back the transaction. If we allow early returns out of a transaction without writes, then it would be simpler remove the write detection code and just always rollback from the transaction. The simpler behaviour would avoid the need to explain the special case to the user in documentation. It would also avoid translating a timeout into a different exception. I think the only advantage of raising an exception would be to make the breaking change more visible and thus easier to notice if the warning wasn't noticed, but the rollback from an early return seems pretty likely to be noticed from test suites if the deprecation isn't noticed. The ability to rollback the transaction through break or return inside the transaction might also be quite convenient for transactions with writes.

eileencodes · 2020-05-27T14:59:32Z

I think raising in 6.2 if there is ever a return, break, or throw inside a transaction could be unexpected and not desired, especially if you're returning before a write. Your use case makes a lot of sense but I think that it can cause confusion for apps upgrading.

We're happy to ignore this deprecation as long as this won't start raising in 6.2. I don't know if using write_query will work since it's mainly intended for simulating a replica (preventing writes in most cases) and not for checking whether any and all queries are writing. We've had some issues opened regarding its use and it definitely won't catch all write queries.

I'm not sure what the right path forward. This change, while fixing a real issue, can be confusing and fixes one case while introducing issues in another case.

dylanahsmith · 2020-05-27T16:58:04Z

Yeah, I'm with you now about not raising in rails 6.2

If the deprecation warning is ignored because it isn't appropriate in some cases, then it won't be effective in letting developers know where to update their code to avoid being effected by the breaking change.

I realize the write_query? might not be perfect, but I think it will be good enough to help make the deprecation warning accurate enough to be useful. It is implemented using a whitelist of read queries, so it might result in false positives warnings, but shouldn't result in false negatives. Without using that, we basically have a lot more false positives without an easy way of fixing the false positives by improving the read query whitelist.

I'll open a PR with my proposed path forward.

sam0x17 · 2022-05-03T19:20:06Z

So what now is the canonical way to return from the current context (let's assume you're nested inside several blocks so next is insufficient) and you still want the enclosing transaction to commit?

majkelcc · 2022-05-04T07:17:23Z

@dylanahsmith

I changed the description of the future behaviour in the exception message:

-                    in the next release of Rails it will instead rollback.
+                    in the next release of Rails it will raise and rollback.

Is this a bug that the following code __silently__ rolls back any updates from the transaction without any exception? This is on Rails 7.0.2.4, also checked the current master:

def update(params)
  ActiveRecord::Base.transaction do
    if user.update(params)
      return :success
    end
  end
  :error
end

eileencodes · 2022-05-04T12:23:35Z

@sam0x17 @majkelcc commenting on 2 year old PRs will likely not get the attention you need.

If you have found a bug, please open a new issue with a reproduction script. Some newer changes have been made to transactions that were not caused by this PR. A bug in 7.x was recently fixed in #44955 which may be related to your current issues.

majkelcc · 2022-05-04T13:28:10Z

@eileencodes thanks, I'll create a new bug report. It looks like my issue is caused by this PR - deprecation in Rails 6 turned into a rollback in Rails 7, but the fact that it does it silently is completely unexpected.

dylanahsmith · 2022-05-04T14:20:42Z

This PR was just a deprecation. The intention was to have it raise in rails 7 so that it didn't rollback silently.

So what now is the canonical way to return from the current context (let's assume you're nested inside several blocks so next is insufficient) and you still want the enclosing transaction to commit?

Extracting the contents of the transaction into its own method would make it easier to return out of those nested blocks while still committing. In that case, the return value returned from that extracted method might be needed to conditionally skip over any code that follows the transaction in that original method.

majkelcc · 2022-05-04T14:40:56Z

The intention was to have it raise in rails 7 so that it didn't rollback silently.

That was my impression after reading this topic - created a bug report here #45017.

dylanahsmith · 2022-05-04T14:51:19Z

Oh right, this PR does has some follow-up discussion already which led to #39453 (we can continue the conversation in #45017 though) as well as discussion about ways of trying to refactor the code to account for this upgrade.

TheRusskiy · 2023-01-29T14:48:15Z

being a bit of a necromant by reviving this thread, just wanted to share the syntax I use for this:
I prefer turning the old

    object.transaction do
      post = Post.find_by_id(123)
      return unless post

      do_stuff(post)
    end

into

    return unless object.transaction do
      post = Post.find_by_id(123)
      next unless post

      do_stuff(post)

      true
    end

this way you can avoid extra nested if statements and return_after_this_transaction variables

throw/catch is used for non-local control flow, not for exceptional situations. For exceptional situations, raise should be used instead. A timeout is an exceptional situation, so it should use raise, not throw/catch. Timeout's implementation that uses throw/catch internally causes serious problems. Consider the following code: ```ruby def handle_exceptions yield rescue Exception => exc handle_error # e.g. ROLLBACK for databases raise ensure handle_exit unless exc # e.g. COMMIT for databases end Timeout.timeout(1) do handle_exceptions do do_something end end ``` This kind of design ensures that all exceptions are handled as errors, and ensures that all exits (normal exit, early return, throw/catch) are not handled as errors. With Timeout's throw/catch implementation, this type of code does not work, since a timeout triggers the normal exit path. See rails/rails#29333 for an example of the damage Timeout's design has caused the Rails ecosystem. This switches Timeout.timeout to use raise/rescue internally. It adds a Timeout::ExitException subclass of exception for the internal raise/rescue, which Timeout.timeout will convert to Timeout::Error for backwards compatibility. Timeout::Error remains a subclass of RuntimeError. This is how timeout used to work in Ruby 2.0. It was changed in Ruby 2.1, after discussion in [Bug #8730] (commit 238c003 in the timeout repository). I think the change from using raise/rescue to using throw/catch has caused significant harm to the Ruby ecosystem at large, and reverting it is the most sensible choice. From the translation of [Bug #8730], it appears the issue was that someone could rescue Exception and not reraise the exception, causing timeout errors to be swallowed. However, such code is broken anyway. Using throw/catch causes far worse problems, because then it becomes impossible to differentiate between normal control flow and exceptional control flow. Also related to this is [Bug #11344], which changed how Thread.handle_interrupt interacted with Timeout.

throw/catch is used for non-local control flow, not for exceptional situations. For exceptional situations, raise should be used instead. A timeout is an exceptional situation, so it should use raise, not throw/catch. Timeout's implementation that uses throw/catch internally causes serious problems. Consider the following code: ```ruby def handle_exceptions yield rescue Exception => exc handle_error # e.g. ROLLBACK for databases raise ensure handle_exit unless exc # e.g. COMMIT for databases end Timeout.timeout(1) do handle_exceptions do do_something end end ``` This kind of design ensures that all exceptions are handled as errors, and ensures that all exits (normal exit, early return, throw/catch) are not handled as errors. With Timeout's throw/catch implementation, this type of code does not work, since a timeout triggers the normal exit path. See rails/rails#29333 for an example of the damage Timeout's design has caused the Rails ecosystem. This switches Timeout.timeout to use raise/rescue internally. It adds a Timeout::ExitException subclass of exception for the internal raise/rescue, which Timeout.timeout will convert to Timeout::Error for backwards compatibility. Timeout::Error remains a subclass of RuntimeError. This is how timeout used to work in Ruby 2.0. It was changed in Ruby 2.1, after discussion in [Bug #8730] (commit 238c003 in the timeout repository). I think the change from using raise/rescue to using throw/catch has caused significant harm to the Ruby ecosystem at large, and reverting it is the most sensible choice. From the translation of [Bug #8730], it appears the issue was that someone could rescue Exception and not reraise the exception, causing timeout errors to be swallowed. However, such code is broken anyway. Using throw/catch causes far worse problems, because then it becomes impossible to differentiate between normal control flow and exceptional control flow. Also related to this is [Bug #11344], which changed how Thread.handle_interrupt interacted with Timeout. Co-authored-by: Nobuyoshi Nakada <nobu@ruby-lang.org>

(ruby/timeout#30) throw/catch is used for non-local control flow, not for exceptional situations. For exceptional situations, raise should be used instead. A timeout is an exceptional situation, so it should use raise, not throw/catch. Timeout's implementation that uses throw/catch internally causes serious problems. Consider the following code: ```ruby def handle_exceptions yield rescue Exception => exc handle_error # e.g. ROLLBACK for databases raise ensure handle_exit unless exc # e.g. COMMIT for databases end Timeout.timeout(1) do handle_exceptions do do_something end end ``` This kind of design ensures that all exceptions are handled as errors, and ensures that all exits (normal exit, early return, throw/catch) are not handled as errors. With Timeout's throw/catch implementation, this type of code does not work, since a timeout triggers the normal exit path. See rails/rails#29333 for an example of the damage Timeout's design has caused the Rails ecosystem. This switches Timeout.timeout to use raise/rescue internally. It adds a Timeout::ExitException subclass of exception for the internal raise/rescue, which Timeout.timeout will convert to Timeout::Error for backwards compatibility. Timeout::Error remains a subclass of RuntimeError. This is how timeout used to work in Ruby 2.0. It was changed in Ruby 2.1, after discussion in [Bug #8730] (commit ruby/timeout@238c003c921e in the timeout repository). I think the change from using raise/rescue to using throw/catch has caused significant harm to the Ruby ecosystem at large, and reverting it is the most sensible choice. From the translation of [Bug #8730], it appears the issue was that someone could rescue Exception and not reraise the exception, causing timeout errors to be swallowed. However, such code is broken anyway. Using throw/catch causes far worse problems, because then it becomes impossible to differentiate between normal control flow and exceptional control flow. Also related to this is [Bug #11344], which changed how Thread.handle_interrupt interacted with Timeout. ruby/timeout@f16545abe6 Co-authored-by: Nobuyoshi Nakada <nobu@ruby-lang.org>

Fix: rails#45017 Ref: rails#29333 Ref: ruby/timeout#30 Historically only raised errors would trigger a rollback, but in Ruby `2.3`, the `timeout` library started using `throw` to interupt 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.

rails-bot assigned rafaelfranca Jun 2, 2017

dylanahsmith mentioned this pull request Jun 2, 2017

Avoid committing a transaction on a timeout from Timeout.timeout #29334

Closed

dylanahsmith changed the base branch from 5-1-stable to master June 3, 2017 01:08

dylanahsmith force-pushed the deprecate-transaction-return branch from d60726b to 0f67779 Compare June 3, 2017 01:08

gmcgibbon added the activerecord label Sep 30, 2018

rails-bot bot added the stale label Dec 18, 2019

dylanahsmith force-pushed the deprecate-transaction-return branch from 5315007 to 7620dea Compare December 18, 2019 18:59

rails-bot bot removed the stale label Dec 18, 2019

rails-bot bot added the stale label Mar 17, 2020

dylanahsmith force-pushed the deprecate-transaction-return branch from 7620dea to c8fac62 Compare March 18, 2020 17:53

rails-bot bot removed the stale label Mar 18, 2020

jeremy added this to the 6.1.0 milestone Mar 18, 2020

Merge branch 'master' into deprecate-transaction-return

1a4427a

jeremy merged commit 31be40d into rails:master Mar 18, 2020

dylanahsmith mentioned this pull request May 27, 2020

activerecord: No warning for return out of transaction block without writes #39453

Merged

dylanahsmith deleted the deprecate-transaction-return branch May 27, 2020 17:44

teckwan mentioned this pull request Mar 2, 2022

[Fix #642] Add new Rails/TransactionExitStatement cop rubocop/rubocop-rails#648

Merged

matthewd mentioned this pull request Mar 3, 2022

Re-deprecate return/throw exits from transactions #44607

Merged

dorkrawk mentioned this pull request Apr 26, 2022

Introduce new cop, TransactionInTimeout rubocop/rubocop-rails#699

Closed

10 tasks

majkelcc mentioned this pull request May 4, 2022

ActiveRecord silently triggers a rollback when using return in the transaction block in Rails 7 #45017

Closed

javierjulio mentioned this pull request Jun 16, 2022

Rails/TransactionExitStatement - Inspect #with_lock method too rubocop/rubocop-rails#710

Merged

9 tasks

jeremyevans mentioned this pull request May 24, 2023

Raise exception instead of throw/catch for timeouts ruby/timeout#30

Merged

casperisfine mentioned this pull request Jun 28, 2023

Active Record commit transaction on return, break and throw #48600

Merged

DanielHeath mentioned this pull request Sep 22, 2023

[🐛 Bug]: When taking a screenshot fails, the resulting NameError is misinterpreted as a lack of driver support for screenshots SeleniumHQ/selenium#12799

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate committing a transaction exited with return or throw #29333

Deprecate committing a transaction exited with return or throw #29333

dylanahsmith commented Jun 2, 2017

rails-bot commented Jun 2, 2017

rails-bot commented Jun 2, 2017

matthewd commented Jun 2, 2017

dylanahsmith commented Jun 3, 2017

matthewd commented Jun 3, 2017

rails-bot bot commented Dec 18, 2019

dylanahsmith commented Dec 18, 2019

rails-bot bot commented Mar 17, 2020

eileencodes commented May 26, 2020

dylanahsmith commented May 26, 2020

eileencodes commented May 27, 2020

dylanahsmith commented May 27, 2020

sam0x17 commented May 3, 2022

majkelcc commented May 4, 2022

eileencodes commented May 4, 2022

majkelcc commented May 4, 2022

dylanahsmith commented May 4, 2022

majkelcc commented May 4, 2022 •

edited

dylanahsmith commented May 4, 2022

TheRusskiy commented Jan 29, 2023

Deprecate committing a transaction exited with return or throw #29333

Deprecate committing a transaction exited with return or throw #29333

Conversation

dylanahsmith commented Jun 2, 2017

Problem

Solution

rails-bot commented Jun 2, 2017

rails-bot commented Jun 2, 2017

matthewd commented Jun 2, 2017

dylanahsmith commented Jun 3, 2017

matthewd commented Jun 3, 2017

rails-bot bot commented Dec 18, 2019

dylanahsmith commented Dec 18, 2019

rails-bot bot commented Mar 17, 2020

eileencodes commented May 26, 2020

dylanahsmith commented May 26, 2020

eileencodes commented May 27, 2020

dylanahsmith commented May 27, 2020

sam0x17 commented May 3, 2022

majkelcc commented May 4, 2022

eileencodes commented May 4, 2022

majkelcc commented May 4, 2022

dylanahsmith commented May 4, 2022

majkelcc commented May 4, 2022 • edited

dylanahsmith commented May 4, 2022

TheRusskiy commented Jan 29, 2023

majkelcc commented May 4, 2022 •

edited