1240145 - Bugzilla::Error::_in_eval() is always true in the cases it is used. why does it exist?

Status: NEW

(Bugzilla :: WebService, defect)

Description

[Dylan Hardison [:dylan] (he/him)]

Aside from the fact that _in_eval() is extremely inefficient (looking at every stack frame twice, since the PSGI support was added) it is also wrong.
It does not take into account that JSONRPC methods are executed inside an eval block. 

We have this code:     $dbh->bz_rollback_transaction() if ($dbh->bz_in_transaction() && !_in_eval());

which means transactions would not be rolled back in web service methods. The only reason this is not manifested as a bug is that we also call it from _cleanup().

The only other use case is to call it for JSONRPC here:

# Most JSON-RPC Throw*Error calls happen within an eval inside
# of JSON::RPC. So, in that circumstance, instead of exiting,
# we die with no message. JSON::RPC checks raise_error before
# it checks $@, so it returns the proper error.
die if _in_eval();

So we know that _in_eval() is true. I am not aware of any code paths in JSONRPC that would lead to it ever being false. I think we can remove _in_eval() completely.

I have some longer-term plans for Throw*Error functions, but this is a step in the right direction.

Comment 1

[Frédéric Buclin]

Before removing _in_eval(), you should look at bug 391073 to see why it has been implemented. Simply removing it is very likely going to regress something.

Comment 2

[Dylan Hardison [:dylan] (he/him)]

Ignoring for a moment that this code is being run inside an _in_eval() == true situation for every web service method...

The code at the time was concerned with removing locks or rolling back the transaction
(because it passed ABORT to $dbh->bz_unlock_tables(1)).  Since Bug 121069 this was replaced with rolling back the transaction
(something which already happens in Bugzilla::_cleanup() -- which is called about 7 times under mod_perl but I digress)

Rewinding a bit -- if transactions are rolled back at the end of every request (by _cleanup, if they're open)
why does _throw_error() need to rollback transactions at all?

Transactions were added in Bug 374004, but the unlock tables during errors dates back about 14 years, to CGI.pl
and ThrowUserError, to Bug 54566. Things were so radically different then it's not even worth talking about.

I think it is entirely safe to remove the transaction rollback from _throw_error(),
and _in_eval().

Comment 3

[Dylan Hardison [:dylan] (he/him)]

Woops, I meant to say "and ThrowUserError, to UserError, to PuntAndTry in Bug 54566" but I think the point was still made.