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.

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!

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.

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?

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...

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.

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

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.

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.

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.

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.

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.

Honestly I think this PR is causing a whole lot more trouble than it's worth. There's lots of code built up over the years that return early in a transaction even after writing, and to have to refactor all of it into hard-to-follow deeply nested if statements, and then having to do something like set a return_after_this_transaction = true variable and then checking it after the transaction commits, is just a really cumbersome thing to make developers have to do, and results in less readable code. Especially considering that Timeout.timeout really shouldn't be being used in the first place: it's inherently unsafe, and in the opinion of many should be deprecated itself the way the equivalent was in Java and other languages: see http://blog.headius.com/2008/02/ruby-threadraise-threadkill-timeoutrb.html, https://www.mikeperham.com/2015/05/08/timeout-rubys-most-dangerous-api/, and https://jvns.ca/blog/2015/11/27/why-rubys-timeout-is-dangerous-and-thread-dot-raise-is-terrifying/

I would definitely support Timeout.timeout being deprecated, but you will have to take those arguments upstream. Based on the age of the articles you linked to, this problem has been known for a long time, so I'm not sure how likely it is to be deprecated.

To avoid this problem with Timeout.timeout, I don't think we even have to completely remove Timeout.timeout, we just need to deprecate the default nil exception klass argument in order to avoid the throw & catch behaviour. That's because this is a deeper issue than just Timeout.timeout, I just used it as an example to highlight the problem given how widespread it has (unfortunately) been used, how clear it is that the desired result is to rollback rather than commit the transaction and how easy it would be for this mistake to accidentally make its way into production. A deprecation warning for Timeout.timeout usage would certainly help though.

I think you might be overestimating how much effort it would take to refactor code to avoid the early return, especially if you compare it with refactoring code wrapped in a Timeout.timeout to not use Timeout.timeout. In some hard to refactor cases, I have ended up swapping Timeout.timeout with a wrapper method that sends a QUIT signal to instruct the server to restart after responding to the request (or to raise Minitest::Assertion for tests).

and to have to refactor all of it into hard-to-follow deeply nested if statements

Similar to what you would do with any method with deeply nested conditions, you can extract method a method to avoid a deeply nested or long method. In this case, you could extract the transaction block into a method and use return as you were doing before, along with a result value that can be used outside the transaction to return early there when appropriate.

What you completely skipped over is the underlying problem that can also be encountered with direct usage of break and throw/catch. The intention isn't as clear in these cases, but it seems more likely that the intention is to not commit the transaction.

Note that an early return on the success path can easily lead to problems when wrapping other block yielding methods, since it skips the code after the yield which is normally where code would be executed to handle the success case. So I think the change would help in setting the right expectation for other code.

@dylanahsmith instead of pulling out the inside of a transaction into it's own function just to be able to use return and avoid deeply nested if statements, which means a good amount of work/testing to make sure you aren't touching any local variables, and at least to me feels more cumbersome than the status quo, I think @zachahn's suggestion in #40052 to just use next to end the block early AND still commit might be a good compromise that will let people like me and him do a relatively painless refactoring and that, at least to my subjective taste, doesn't feel too cumbersome. It currently works, but when the final change is made in 6.2, if you or whoever reading this could make sure that next isn't included in the list of things that will cause a rollback that would be awesome... even better if it was documented functionality we could count on...

Just PS though, the example you gave of

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

still isn't safe to use - what if the Timeout raises in the middle of transaction's ensure block before the open transaction is committed or rolled back or the connection is thrown away? Although I suppose in a test suite it could be useful shortcut, which seems to be the way you are using it.

Anyway, @dylanahsmith thank you for hearing me out, and please consider the suggestion about leaving us the next keyword to be able to exit the transaction early and still be able to count on a commit!

You can absolutely use next, which is basically a block return, so would be the same to rails as if the end of the block was reached. It just won't work if you were returning out of a further nested block, which is why I suggested method extraction as something that would even work in those situations.

If you don't get a deprecation warning with your code, then it shouldn't break from the corresponding breaking change. We know very well how important this is for upgrading old code to prepare for a new release.

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?

@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

@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.

@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.

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.

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.

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.

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