Not throwing proper exception when in transaction

simPod commented 5 years ago

Bug Report

Q	A
Version	2.6.3

Summary

EntityManager tries to rollback the transaction when exception is thrown during commit operation. The rollback is performed through DBAL Connection. The problem is that the transaction is no longer active and when DBAL Connection tries to rollback through PDOConnection, There is no active transaction exception is thrown instead and the original exception is lost. So user has no easy way to know actual exception and gets There is no active transaction instead.

Current behavior

PDOException A SQLSTATE[23505]: Unique violation: 7 ERROR: duplicate key value violates unique constraint ... occurs on commit in https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/EntityManager.php#L238
Transaction is attempting to rollback https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/EntityManager.php#L243 before exception A is thrown on https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/EntityManager.php#L245
DBAL Connection::rollback() is called where $this->_conn->rollBack(); is called. _conn is instance of PDOConnection that throws There is no active transaction exception instead and the original exception is lost.

Expected behavior

The original exception is properly thrown.

Ocramius commented 5 years ago

The original exception is properly thrown.

I'd rather say that the failed rollback should have priority here, as something much worse is happening (no active transaction in first place).

simPod commented 5 years ago

I forgot to mention that the whole operation is called within $entityManager::transactional() but that can be seen in linked source.

I see that it starts with $this->conn->beginTransaction();
Followed by $this->conn->commit() after flush and $this->_conn->commit() within it.

Just checked that before calling PDOConnection::commit(), the transaction is active PDO::inTransaction() -> true

After first PDOException with Unique violation is thrown, the transaction is no longer active.

Most possibly because $this->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); is set on DBAL PDOConnection in the constructor so commit() throws exception and cancels transaction.

simPod commented 5 years ago

I decided to go with a test. Found out, that it works just fine. The problem is that in some cases, I defer unique constrains (eg. when doing full sync, running all CUD operations)

When constraint is deferred, the violation is thrown during commit, not on flush and therefore the issue pops up.

https://github.com/simPod/doctrine2/commit/cd031c84b7346fc264819b211e51f5d00408cc94

Tests pass when line :48 is commented out (constraint defer is disabled).

Ocramius commented 5 years ago

@simPod I'd suggest tackling this within 3.x, by adding a better exception type that captures both rollback and previous failures.

simPod commented 5 years ago

@grongor took over 👋

ro0NL commented 4 months ago

the original exception should not/never be swallowed, and it's a real prod debug issue

we'll get an exception either way, i prefer the original one

so i suggest tackling this within 2.x

greg0ire commented 4 months ago

@ro0NL this was reported 6 years ago. Can you please provide a full stack trace of the exception you are getting now? I would like to know if it is still a PDOException or if it is wrapped in another exception type. If we could with Marco's solution, I think changing the exception type could be considered a breaking change, even though I don't see why anyone would want to catch something this serious. If that's the case, I would suggest cloning the exception, and tweaking the message of the cloned exception so that it contains a string representation of the previous exception, or adding a property to the exception so that it's possible for an error handler to display it however they see fit. I don't think we will be able to use $previous here.

ro0NL commented 4 months ago

Doctrine\DBAL\Exception\DriverException

An exception occurred while executing a query: SQLSTATE[42000]: Syntax error or access violation: 1305 SAVEPOINT DOCTRINE_2 does not exist

/app/vendor/doctrine/dbal/src/Connection.php:1943
/app/vendor/doctrine/dbal/src/Connection.php:1885
/app/vendor/doctrine/dbal/src/Connection.php:1213
/app/vendor/doctrine/dbal/src/Connections/PrimaryReadReplicaConnection.php:292
/app/vendor/doctrine/dbal/src/Connection.php:1638
/app/vendor/doctrine/dbal/src/Connections/PrimaryReadReplicaConnection.php:365
/app/vendor/doctrine/dbal/src/Connection.php:1535
/app/vendor/doctrine/dbal/src/Connections/PrimaryReadReplicaConnection.php:322
/app/vendor/doctrine/orm/src/UnitOfWork.php:485
/app/vendor/doctrine/orm/src/EntityManager.php:403

previous: Doctrine\DBAL\Driver\PDO\Exception

SQLSTATE[42000]: Syntax error or access violation: 1305 SAVEPOINT DOCTRINE_2 does not exist

/app/vendor/doctrine/dbal/src/Driver/PDO/Connection.php:39
/app/vendor/doctrine/dbal/src/Driver/Middleware/AbstractConnectionMiddleware.php:46
/app/src/Bundles/CoreFunctionsBundle/Dbal/Connection.php:90
/app/vendor/doctrine/dbal/src/Connection.php:1211
/app/vendor/doctrine/dbal/src/Connections/PrimaryReadReplicaConnection.php:292
/app/vendor/doctrine/dbal/src/Connection.php:1638
/app/vendor/doctrine/dbal/src/Connections/PrimaryReadReplicaConnection.php:365
/app/vendor/doctrine/dbal/src/Connection.php:1535
/app/vendor/doctrine/dbal/src/Connections/PrimaryReadReplicaConnection.php:322
/app/vendor/doctrine/orm/src/UnitOfWork.php:485
/app/vendor/doctrine/orm/src/EntityManager.php:403

previous previous: PDOException

SQLSTATE[42000]: Syntax error or access violation: 1305 SAVEPOINT DOCTRINE_2 does not exist

/app/vendor/doctrine/dbal/src/Driver/PDO/Connection.php:33
/app/vendor/doctrine/dbal/src/Driver/Middleware/AbstractConnectionMiddleware.php:46
/app/src/Bundles/CoreFunctionsBundle/Dbal/Connection.php:90
/app/vendor/doctrine/dbal/src/Connection.php:1211
/app/vendor/doctrine/dbal/src/Connections/PrimaryReadReplicaConnection.php:292
/app/vendor/doctrine/dbal/src/Connection.php:1638
/app/vendor/doctrine/dbal/src/Connections/PrimaryReadReplicaConnection.php:365
/app/vendor/doctrine/dbal/src/Connection.php:1535
/app/vendor/doctrine/dbal/src/Connections/PrimaryReadReplicaConnection.php:322
/app/vendor/doctrine/orm/src/UnitOfWork.php:485
/app/vendor/doctrine/orm/src/EntityManager.php:403

greg0ire commented 4 months ago

Then I think a possible design could be defining DriverException::withPrevious(\Throwable $throwable) or rather DriverException::withSomeOtherName() to avoid confusion with $previous, add a getter for that thing, then modify Connection::rollback() so that it accepts a throwable, and calls the wither above when that throwable is defined and an exception occurs here: https://github.com/doctrine/dbal/blob/4.0.x/src/Connection.php#L1061-L1063

Then, that extra exception could be displayed by the error handler… that could be an issue since it would mean the error handler would have to know about the wither. Another solution to that could be to clone the exception and modify its message.

Sadly, this means this cannot be tackled in 2.x

ro0NL commented 4 months ago

perhaps simply rethrow with original exception concatted in message?

https://3v4l.org/FmcJa

greg0ire commented 4 months ago

Yes, that's what I mean by "modify its message" (except I would use deep cloning). In the meantime, I think there is something you could try on your application to debug your urgent production issue with ORM 2 and DBAL 3: writing a DBAL middleware that catches, logs and rethrows any exception it sees go through.

Note that since AbstractConnectionMiddleware has a rollback method, you could go as far as memoizing the exception, and then logging when and only when rollback() is called and an exception occurs when calling $this->wrapped->rollback().

adlpz commented 2 months ago

We've been seeing this issue. I'll add some context on what we've found:

We're in ORM 2.
The DB is MySQL and we're using savepoints.
In our case, the underlying exception is a Doctrine\DBAL\Exception\DeadlockException.
Apparently with MySQL an implicit rollback happens when a deadlock occurs.
The exception is triggered by a $em->flush() which calls $uow->commit(). In the main try/catch block of the method $conn->isTransactionActive() is true and $conn->rollBack() is called.
If we don't use savepoints, this results in a ROLLBACK issued, which with no transactions active (they've been implicitly rolled back) is a no-op.
However with savepoints this results in a ROLLBACK TO SAVEPOINT DOCTRINE_X. As this savepoint doesn't exist anymore, a Doctrine\DBAL\Exception\DriverException is thrown.
The same catch block handles the new exception which overwrites the original from then on. The original issue never reaches our logging infrastructure.

We've seen two possible workarounds:

Forking/patching UnitOfWork to add a try-catch around the rollback.
Using a connection middleware to patch $connection->exec() and $connection->rollBack() and catch/log any exceptions from these statements.

However in any case a proper solution for this issue would be, as suggested, to include the information of the previous exception somehow. Maybe with a specific exception class that handles exceptions happened during the handling of an exception like it's the case here.

doctrine / orm