Rails core - Apr 2007 - 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

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

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

-- 
Cheers

Koz

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

On 4/8/07, Michael Koziarski <michael@koziarski.com>
wrote:
> create an exception subclass which causes the transaction
> to be rolled back, and then return from transaction rather than
> propagating.
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.

--
Alex

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

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



-- 
Cheers

Koz

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

On 4/9/07, Michael Koziarski <michael@koziarski.com>
wrote:
> Case 3 seems completely broken because it means
> code inside a transaction block is no longer guaranteed to be atomic.
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

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

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

-- 
Cheers

Koz

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

On 4/9/07, Michael Koziarski <michael@koziarski.com>
wrote:
> 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.
Stack trace generation is somewhat expensive. throw doesn''t do that,
just unwinds the stack up to the catch.

Alex

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

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


-- 
Cheers

Koz

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

On 4/11/07, Michael Koziarski <michael@koziarski.com>
wrote:
> The exception implementation was much much simpler, so I''ve
attached
> it to the ticket.
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

--
Alex

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---