1499262 - Bugzilla::DB should gracefully handle disconnection events that happen during transactions

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Phabricator Integration, defect, P1)

Description

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

Every friday, the Request Naggers script runs a query that (at least recently) crashes the RDS/mysql server it connects to. This is an annoyance, but has been made into a dire situation due to interaction with the feed daemon that syncs phabricator and bugzilla.

If the feed daemon is connected to the same server, this causes the feed daemon to be disconnected.

In order to fix problems with stale connections (which also caused the feed daemon to break, but in a different way) Bugzilla::DB switched from being a subclass of DBI to having a 'has a' relationship with DBIx::Connector, which is a connection pool factory for DBI.

Bugzilla does not currently use the transaction features offered by DBIx::Connector, and instead opts to use a pairing of Bugzilla::DB->bz_start_transaction() and Bugzilla::DB->bz_commit_transaction() to handle transactions. 

In addition, bugzilla tracks if it is in a transaction using an attribute 'bz_in_transaction'.

For every DBI method called, the Bugzilla::DB object delegates to the DBIx::Connector instance, which returns a (possibly reused) DBI object.

Imagine the following scenario in pseudo-code:

$db->bz_start_transaction();
$db->do("INSERT ...");
$db->do("UPDATE ...");
$db->bz_commit_transaction();

In between the INSERT and UPDATE, a reconnection event may occur.
Should this happen, there is a 'on_connection' callback that will be called.
This callback will note that $db->bz_in_transaction is true,
and as a result will throw an error.

This is what you'd want, as it is not possible to continue.

However, the problem happens in Bugzilla's ->cleanup routine.

The cleanup routine is called at the end of every request, and
one of the things it does is if any transaction is left open, it will roll it back by calling bz_rollback_transaction().

The problem is that bz_rollback_transaction() will call the delegated 'rollback' method, which will cause a new DBI object to be created.
This means the on_connect() callback is called, and this is a fatal error.

The rest of the cleanup process will be skipped, and bz_in_transaction() will never be reset.

The solution to this becomes quite clear after figuring all this out...

Comment 1

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

Attachment: GitHub Pull Request

Comment 2

[Kohei Yoshino]

Merged to master.