SQLite3 build is broken because calling rollback!() doesn't prevent COMMIT

I know why SQLite3 is broken. This code:

    Topic.transaction do |transaction|       transaction.rollback!     end

results in the following database commands:   BEGIN;   ROLLBACK;   COMMIT;

In other words, there is a COMMIT when no transaction is in flight. MySQL and Postgres are coll with it, but SQLite3 blows up with indignation.

Recorded as a ticket: http://dev.rubyonrails.org/ticket/8030

Alex

I know why SQLite3 is broken. This code:

    Topic.transaction do |transaction|       transaction.rollback!     end

results in the following database commands:   BEGIN;   ROLLBACK;   COMMIT;

In other words, there is a COMMIT when no transaction is in flight. MySQL and Postgres are coll with it, but SQLite3 blows up with indignation.

Recorded as a ticket: http://dev.rubyonrails.org/ticket/8030

We've discussed this a while back, perhaps on campfire... However I think the real fix is to no longer rely on yielding the connection, and instead create an exception subclass which causes the transaction to be rolled back, and then return from transaction rather than propagating. This will let us ensure we don't try to commit after rollback, and also let people roll back from deeper scopes without passing around the txn.

Thoughts?

Fundamental problem here is that we have nested transaction() method calls, but they all deal with the same transaction, isn't it?

I see three possible ways to handle rollback!():

1. What you say: rollback throws an exception that unwinds the stack all the way to the top-most transaction() call. By the way, it may be a good idea to use throw/catch instead of raise/rescue here, because this would make it less likely that rollback exception is intercepted by a rescue clause in the block given to transaction() method.

2. Some sort of connection state that knows when a transaction was rolled back but not "completed" yet, and raises an error if any database operation is attempted.

3. Begin a new transaction immediately after rollback!()

It seems obvious to me that (2) is worse than (1) (if you are going to blow up, do so at the earliest opportunity).

Intuitively, (1) seems obviously better than (3) because it probably covers 9 cases out of 10.

But... I'm in doubt for the following reasons:

(1) is already covered (by just raising an error). What about the following scenario though:

1. do database operations X, Y 2. figure out that operation Z is impossible 3. rollback X and Y, and do something else.

As long as I have this code in an application, everything is fine and dandy. What if I need to do it in a library method, which is not guaranteed to be the top-most transaction()?

So, (1) covers 90% of cases, but those are already covered by other means. (3), on the other hand, covers the remaining 10%.

Another benefit of (3) is that it will not break any existing code that relies on the ability to do database operations after rollback!()

As for the principle of least surprise, (1) and (3) will both surprise someone. If your brain (like mine) is wired so that rollback is ALWAYS an exception, (3) is a surprise. But I'd avoid using rollback!() in the first place.

If you are from a C or PHP 3 background (error handling through return codes, no such thing as an exception), then you will use rollback!() and (1) will surprise you a lot, because this piece of code:

transaction do |trx|   if there_is_a_problem     trx.rollback!()     log "hmm, there is a problem"   end end

contains no indication that log() won't be called.

(1) is already covered (by just raising an error). What about the following scenario though:

1. do database operations X, Y 2. figure out that operation Z is impossible 3. rollback X and Y, and do something else.

As long as I have this code in an application, everything is fine and dandy. What if I need to do it in a library method, which is not guaranteed to be the top-most transaction()?

So, (1) covers 90% of cases, but those are already covered by other means. (3), on the other hand, covers the remaining 10%.

You can cover the remaining case by manually calling rollback! on the model's connection. Case 3 seems completely broken because it means code inside a transaction block is no longer guaranteed to be atomic.

transaction do   @model.operation_one   txn.rollback!   @model.operation_two end

instead of one and two being all ACID like, you get operation two committed to the db with operation one nowhere to be seen. That seems like a much more dangerous situation than having op two never called.

Agree. Although it is exactly what happens now (except that there is no new transaction).

I've never used explicit rollback() inside business logic in any language that has exceptions, for that very reason (atomicity creates the "rollback is exception" paradigm).

If we can disregard breaking any existing code that goes like:

  transaction do |trx|     ...     if there_is_a_problem       trx.rollback!       log 'Oops!'       # other interesting side effects     end   end

then I'll happily live with (1).

Alex

If we can disregard breaking any existing code that goes like:

  transaction do |trx|     ...     if there_is_a_problem       trx.rollback!       log 'Oops!'       # other interesting side effects     end   end

then I'll happily live with (1).

Unless I'm mistaken this mode of using transactions was added during the current trunk phase, and I'm pretty sure that only david uses it. I'll be sure to send a headsup before this list before I commit anything.

As for throw/raise. I'm going to wear my newb hat and say I don't really 'get' the difference. But I certainly like the idea of not breaking existing rescue clauses, so throw seems perfect.

Does anyone have any objections to using catch(:rollback) throw :rollback?

Stack trace generation is somewhat expensive. throw doesn't do that, just unwinds the stack up to the catch.

Alex

Stack trace generation is somewhat expensive. throw doesn't do that, just unwinds the stack up to the catch.

The exception implementation was much much simpler, so I've attached it to the ticket.

http://dev.rubyonrails.org/attachment/ticket/8030/sqlite-safe-rollback.diff

Heads up to anyone using transaction.rollback!, once I commit this (in the next 48 hours, barring objections) your code may do funny things if you were expecting statements to execute after rollback! was called.

This fixes a failure in the SQLite3 build. There are two more tests failing, for an entirely different reason. I'll write it up separately.

Donning my tester's hat, I wrote a couple unit tests, the first of which fails:

  def test_manually_rolling_back_a_nested_transaction     Topic.transaction do |transaction|       @first.approved = true       @second.approved = false       @first.save       Topic.transaction do |nested_transaction|         @second.save         nested_transaction.rollback!       end     end

    assert @first.approved?, "First should still be changed in the objects"     assert !@second.approved?, "Second should still be changed in the objects"

    assert !Topic.find(1).approved?, "First shouldn't have been approved"     assert Topic.find(2).approved?, "Second should still be approved"   end

  def test_code_after_rollback_should_not_be_executed     Topic.transaction do |transaction|       @first.approved = true       @first.save       transaction.rollback!       fail "code after rollback should not be executed"     end     assert @first.approved?, "First should still be changed in the objects"     assert !Topic.find(1).approved?, "First shouldn't have been approved"   end