joomla-framework
Transaction handling is fundamentally broken due to automatic reconnect behaviour in execute() method
Steps to reproduce the issue
It's not that simple to reproduce this error since you have to simulate a database disconnect.
The following code illustrates the problem, it targets MySQL (but all database drivers are affected since every database rolls back transactions automatically when the connection gets lost).
$db = JFactory::getDbo(); // set up connection (Joomla CMS)
$db->setQuery('CREATE TABLE IF NOT EXISTS transactiontest (pk_value INT, somecol INT)');
$db->execute();
try {
$db->transactionStart();
$db->setQuery('INSERT INTO transactiontest VALUES (1,1)');
$db->execute();
// to simulate a database disconnect set a brakepoint in the debugger on the next line
// and when it get's hit restart your database and then continue debugging when the database is running again
$db->setQuery('INSERT INTO transactiontest VALUES (2,2)');
// the next line should fail but it does not since the automatic reconnect behaviour inside the execute method
// will kick in and reconnect sucessfully and just execute the query without triggering an error
$db->execute();
$db->transactionCommit();
}
catch(RuntimeException $e)
{
// something failed - rollback
$db->transactionRollback();
}
$db->setQuery('SELECT * FROM transactiontest');
$rows = $db->loadObjectList();
// excpected result is zero rows, but it returns 1 row with the values 2,2
var_dump($rows);
Expected result
No rows inserted into the database
Actual result
1 row inserted into the database
System information (as much as possible)
Doesn't matter - since the issue is independent of OS, database or driver that's used. All platforms are affected.
Additional comments
There are two solutions to the issue.
-
remove the automatic reconnect code in the execute() method, this is probably problematic since it isn't in general a bad idea to do so, intermittend database outages might occur and if one is not inside a transaction the reconnect behaviour is a good thing
-
only try to reconnect if not inside a transaction - this is actually my proposed solution since to implement that just one line needs to be changed in the execute method (of all database classes)
// If an error occurred handle it.
if (!$this->cursor)
{
// Get the error number and message before we execute any more queries.
$this->errorNum = $this->getErrorNumber();
$this->errorMsg = $this->getErrorMessage();
// Check if the server was disconnected, but only if not inside a transaction
if ($this->transactionDepth == 0 && !$this->connected())
{
try
{
// Attempt to reconnect.
$this->connection = null;
$this->connect();
}
...
Could you do a pull request with your change? Transactions aren't a strong point in my SQL knowledge but the fix makes sense to me.
