mirror of
https://github.com/doctrine/orm.git
synced 2026-03-24 06:52:09 +01:00
ORM and DBAL: Erroneous PDOException "There is no active transaction" thrown during commit #5169
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @nagmat84 on GitHub (Jun 29, 2016).
Overview
Assume that we rely on the implicit transaction handling of the Doctrine ORM EntityManager. Assume the following simple code
and further assume that entity 42 cannot be removed due to violated foreign key constraints.
Expected result
The expected result is that
$em->flush()throws some sort of exception that relates to the foreign key constraint, e.g.\Doctrine\DBAL\Exception\ForeignKeyConstraintViolationException.Actual result
The actual result is a native PHP
PDOExceptionwith error message "There is no active transaction". The latter is an astonishing exception, because the aforementioned code does not deal with transactions directly.Investigations and Proposed Solutions
Below I will explain where the bug stems from. The story is a little bit longer, because it involves 1 bug inside of the ORM component and 3 bugs inside of the DBAL component.
Reason 1: ORM component
Snippet from class
\Doctrine\ORM\UnitOfWorkAt line 410 while calling
$conn->commit()the following exception is thrown:This is correct, because the foreign key constraint is evaluated during the commitment. However, the type of the exception should probably be a
\Doctrine\DBAL\Exception\ForeignKeyException. We come to this point later, because this must be fixed elsewhere.The problem here is that at line 413 inside of the catch-block
$conn->rollback()tries to rollback a transaction that has already been terminated by$conn->commit(). It seems that this code does not take into account that$conn->commit()can fail with an exception after the transaction has already been terminated. In this case the transaction is automatically rolled back at server side and must not be rolled back a second time.At the moment at line line 413
$conn->rollback()throws an additional exception and the original exception is lost, because line 417 will never be executed:Proposed solution (part 1 of 4): Check inside of the exeption handler if transaction is still active and only roll back in this case. Replace line 413 by:
if( $conn->isTransactionActive() ) $conn->rollback()However, even at the moment the exception thrown by rollback should be
\Doctrine\DBAL\ConnectionException::noActiveTransaction()and not a plain PDOException. This dicrepancy is due to another bug. We come to this point later, because this must be fixed elsewhere.Reason 2: DBAL component
The both remaining bugs that have already been mentioned are located at class
\Doctrine\DBAL\ConnectionAt line 1216 inside the commit-method the method
$this->_conn->commit()of the underlying native PHP PDO object is called. Here the originalPDOExceptiondue to the violated foreign key constraint is thrown. However, the code does not take into account that$this->_conn->commit()might fail with an exception and still terminates the transaction nonetheless. The_transactionNestingLevelshould be decremented to 0, because the transaction has been terminated.Proposed solution (part 2 of 4): Nest
$this->_conn->commit()into atry-block, catch thePDOException, inside thecatch-block check if the transaction has been terminated and decrement_transactionNestingLevelif necessary.Proposed solution (part 3 of 4): Moreover, re-throw the
PDOExceptionbut wrap or convert it into an appropriate child object of\Doctrine\DBAL\DBALException(in this particular case:\Doctrine\DBAL\Exception\ForeignKeyConstraintViolationException).At the moment the condition at line 1265 inside the rollback-method does not catch the aforementioned case. If the proposed solution is applied the
_transactionNestingLevelwould be zero andConnectionException::noActiveTransaction()would be thrown. At the moment_transactionNestingLevelerroneously equals 1 and$this->_conn->rollback()throws aPDOExceptionbecause there is no active transaction. This problem would be fixed with proposed solution part 2.However, there are still one more bug in this code. (I am sorry.) In method
beginTransaction()at line 1168 the_transactionNestingLevelis increased and the transaction is started at line 1176. The code does not take into account that$this->_conn->beginTransaction()might fail with an exception without starting a transaction. In this case_transactionNestingLevelerroneously equals 1 but should be 0.Proposed solution (part 4 of 4): Nest
$this->_conn->beginTransaction()into atry-block and decrement_transactionNestingLevelagain, if the transaction could not be started.@deeky666 commented on GitHub (Jun 30, 2016):
@nagmat84 first of all thanks for the very detailed issue description and investigation. There seem to be some valid points here, although I want to point out two things:
commit()andbeginTransaction()calls is that the related DBAL connection interface does not define exceptions to be thrown but returntrueorfalseinstead. This might have been a design mistake or by intention I don't know. The question here is then whether we can redefine the interface description to throw exceptions (which it currently partially does anyways) or if driver implementations have to be adjusted. I personally would prefer the first as error information might get lost otherwise.That pointed out, I might take a look at the DBAL side issues as soon as I have time.
Thanks for reporting.
@nagmat84 commented on GitHub (Jun 30, 2016):
@deeky666 thank you for your reply and your offer to look at it. Regarding your points
I see your point. I only found
Doctrine/DBAL/Driver/PDOConnection.php, so this was my mistake. I have the feeling that PHP PDO behaved differently in the past, too. PHP PDO methodscommit(),beginTransaction()androllback()can either throw exceptions or returntrueorfalse. That depends on what the attributePDO::ATTR_ERRMODEis set to. I have the strange feeling that I did not encounter this problem in the past and maybe the PHP default settings have changed. But this is just a guess.You are right in this point. If a failing
commit()does not throw an exception but just returnsfalsethe current code would at least not throw an exception and behave more or less correctly. However, even if you stick to return booleans the following code\Doctrine\ORM\UnitOfWorkis still not perfect:A failing commitment without an exception would at least not trigger a superfluous rollback and it would correctly decrement the
_transactionNestingLevelbut the error would never be reported to the user code. The return code ofcommit()is not checked. If one sticks to return booleans they should be checked at least.I would like to raise another point. Personally, from looking at the code I have the feeling that the whole code is written without taking into account that SQL constraints can be deferred within a transaction until the end of the transaction. I have the feeling that the whole code expects each query to throw an exception immediately, if the SQL statement cannot be executed. Hence, if the DBMS, the SQL table or the SQL constraint is defined to be "immediately", we would have the following result:
The above works just fine with DBAL. But if the DBMS, the SQL table or constraint is "deferred", we would have the following result:
Probably, I wrong you guys, but I have the impression that this aspect was overseen.