1103005 - Stop refusing to roll back transactions when in an eval { }

Status: NEW

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Gervase Markham [:gerv]]

A while back, I found a long-standing bug in a Bugzilla extension I maintain.

Regularly, this extension tries to create a series of bugs. Sometimes
the data for one or more of those bugs was bad (e.g. invalid version)
and this failed. This caused the entire job to die. So I wrapped the
Bugzilla::Bug->create() call in eval {} and handled the error, then
proceeded. I thought this was the right thing to do.

However, it turns out that if you are using eval(), then
Bugzilla::Bug->create() will start a transaction, but the Bugzilla error
code will not roll it back or close it:

Error.pm, lines 57-60, function _throw_error:

    # Make sure any transaction is rolled back (if supported).
    # If we are within an eval(), do not roll back transactions as we
    # are eval'uating some test on purpose.
    $dbh->bz_rollback_transaction() if ($dbh->bz_in_transaction() &&
!_in_eval());

The result was that I was still in a transaction after handling the
error. The rest of my code created more bugs, opening and closing
transactions each time, but when it all got to the end, there was still
an open transaction - and we don't call commit() until all transactions
are closed. So none of my bugs actually got created in the DB (although
the auto_increment showed that their bug numbers had been allocated)!

The fix was to manually close the transaction when I had a failure, but
the need to do this was not at all obvious.

This error behaviour seems wrong to me (in that it exposes weird
requirements on the user of an API which are not obvious and easy to
miss or get wrong). This bug is tracking possible alternatives.

Gerv

Comment 1

[Frédéric Buclin]

_throw_error() correctly refuses to rollback when in an eval. That's really the correct behavior. Else an eval() which fails would rollback everything which happened before or after the eval(), defeating the goal of using eval(). What you want is to only revert SQL commands called during the eval(), not everything. I don't know if/how this can be done.

Comment 2

[Gervase Markham [:gerv]]

In the middle of the night, I had the brainwave that nested transactions might solve this, because you could start a new one and just roll back the most recent... but it seems like MySQL does not support them:
http://stackoverflow.com/questions/1306869/are-nested-transactions-allowed-in-mysql

If you rollback(), you rollback everything since the first bz_start_transaction() was called. But if you don't rollback after getting an error, how can you know the state of the $dbh is OK to continue? That doesn't seem great either.

We want to test stuff, and if it works, do it, but if it doesn't work, recover and continue, right? (When do we need to do that? Surely if someone tries to create a bug with an invalid version, we don't create it, do we? Or do we use the default?)

Could we use a different Bugzilla->dbh, swapping it out for the duration of the eval, trying the things we need to try, then throwing away that dbh and returning to the original one, and repeating the work which succeeded? Would that work?

The fundamental problem here is that if anyone wants to call Bugzilla::Bug->create(), they have two options:

A) Don't put it in an eval { }, and have your code die horribly on errors; or
B) Put it in an eval { }, and remember to roll back the transaction manually every time an error is hit

I just don't think B) is a reasonable API contract, even if we document it.

Gerv

Comment 3

[Frédéric Buclin]

(In reply to Gervase Markham [:gerv] from comment #2)
> In the middle of the night, I had the brainwave that nested transactions
> might solve this, because you could start a new one and just roll back the
> most recent... but it seems like MySQL does not support them:

Did you look at savepoints? http://dev.mysql.com/doc/refman/5.5/en/savepoint.html

If I read it correctly, one may create a savepoint when entering eval(), and call rollback to savepoint if the eval fails.

Comment 4

[Gervase Markham [:gerv]]

OK, those might do the trick... can we do this in a cross-DB way? Or would we implement it just for MySQL and make it no-op for other DBs?

Gerv

Comment 5

[:glob ✱]

(In reply to Gervase Markham [:gerv] from comment #2)
> OK, those might do the trick... can we do this in a cross-DB way? Or would
> we implement it just for MySQL

a quick search tells me that pg, sqlite, and oracle have some form of savepoints.

> .. and make it no-op for other DBs?

if this is implemented, the behaviour needs to be the same regardless of the db used.  otherwise you'll be expecting an extension which needs this fix to either limit their extension to mysql, or take different behaviour depending on the database used.

Comment 6

[Gervase Markham [:gerv]]

So how would this work? Perhaps a new function, eval_sql, which took a coderef to execute. It would:

* set a savepoint
* call the coderef inside an eval
* release the savepoint if there was no error

If an error was thrown, Throw*Error would roll back the most recent savepoint instead of rolling back everything, and then call "die", as now. Then, when the eval finished, it wouldn't try and release the savepoint because an error was thrown.

eval_sql should check for self-nesting, because we shouldn't do that otherwise (I think) it'll all go wrong.

Then, we'd wrap all the places where we were "testing" something with eval_sql instead of eval { }.

Is that about right?

Gerv

Comment 7

[Frédéric Buclin]

(In reply to Gervase Markham [:gerv] from comment #6)
> So how would this work? Perhaps a new function, eval_sql, which took a
> coderef to execute.

No way! bz_start_transaction() would simply check if we are in an eval, and if true, create a savepoint. _throw_error() would then call "rollback to savepoint" if a savepoint exists, else call "rollback" itself. Nothing more.

Comment 8

[Gervase Markham [:gerv]]

LpSolit: What would bz_start_transaction do if we were in two evals? Still create just one savepoint? And what would it do if it were called a second time, also in an eval (it can be called multiple times)? Would it create one savepoint per call?

Gerv

Comment 9

[Frédéric Buclin]

(In reply to Gervase Markham [:gerv] from comment #8)
> LpSolit: What would bz_start_transaction do if we were in two evals? Still
> create just one savepoint? And what would it do if it were called a second
> time, also in an eval (it can be called multiple times)? Would it create one
> savepoint per call?

caller($i) lets you know how many eval() are running. bz_start_transaction() would create one savepoint per level. When _throw_error() is called, it will revert to the corresponding savepoint.

Comment 10

[Gervase Markham [:gerv]]

LpSolit: how do you define "corresponding"?

Let's say we have the following call stack:

function
function
eval {
function
bz_start_transaction [savepoint 1]
function
eval {
function
bz_start_transaction [savepoint 2]
function
Throw*Error
_throw_error

Oops! _throw_error wants to roll back. It knows of two savepoints. Does it roll back to savepoint 2 or 1?

(The above stack I think matches the situation which led to this, but it could also match other situations not involving extension code.)

Gerv

Comment 11

[Frédéric Buclin]

(In reply to Gervase Markham [:gerv] from comment #10)
> Oops! _throw_error wants to roll back. It knows of two savepoints. Does it
> roll back to savepoint 2 or 1?

It looks at caller($i) to know in which eval() it is, and calls the corresponding savepoint. In your case, caller($i) will report that 2 eval() are in progress, so _throw_error() calls savepoint 2. If the inner eval() was complete already, caller($i) would report that there is only one eval() in progress and _throw_error() would call savepoint 1.