From 8b0c5ac7db97eaf832b66009d0a3ec0bc7f395b4 Mon Sep 17 00:00:00 2001 From: Matthias Pigulla Date: Wed, 9 Aug 2023 19:04:57 +0200 Subject: [PATCH] Prevent `UnitOfWork::commit()` reentrance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR prevents reentrant calls to `UnitOfWork::commit()` with a clear exception message. Reentrance happens, for example, when users call `EntityManager::flush()` from within `postPersist` or similar event listeners. Since #10547, it causes strange-looking, non-helpful error messages (#10869). Instead of treating this as a bug and trying to fix it, my suggestion is to fail with a clear exception message instead. Reasons: I assume the UoW was never designed to support reentrant `flush()` calls in the first place. So, trying to make (or keep) this working might be a rabbit hole. The assumption is based on the following: * Not a single test in the ORM test suite covers or even triggers `flush()` reentrance – otherwise, such a test would have failed with the changes suggested here. * Documentation for e. g. [`preFlush`](https://www.doctrine-project.org/projects/doctrine-orm/en/2.16/reference/events.html#preflush) or [`postFlush`](https://www.doctrine-project.org/projects/doctrine-orm/en/2.16/reference/events.html#preflush) explicitly mentions that `flush()` must not be called again. I don't know why this is not also stated for e. g. `postPersist`. But why would it be safe to call `flush()` during `postPersist`, in the middle of a transaction, when there are reasons against calling it in `postFlush`, just before final cleanups are made? * Last but not least, entity insertions, computed changesets etc. are kept in fields like `UnitOfWork::$entityChangeSets` or `UnitOfWork::$originalEntityData`. It's all to easy to mess up these states when we re-compute changesets mid-way through a `flush()` – and again, if that were anticipated, I'd assume to find any kind of test coverage. --- lib/Doctrine/ORM/UnitOfWork.php | 236 +++++++++++++++++--------------- 1 file changed, 127 insertions(+), 109 deletions(-) diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 4c343f2ec93..b1e30164fa3 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -48,6 +48,7 @@ use Doctrine\Persistence\PropertyChangedListener; use Exception; use InvalidArgumentException; +use LogicException; use RuntimeException; use Throwable; use UnexpectedValueException; @@ -328,6 +329,13 @@ class UnitOfWork implements PropertyChangedListener /** @var ReflectionPropertiesGetter */ private $reflectionPropertiesGetter; + /** + * Set to `true` while the UnitOfWork has entered the `commit()` method + * + * @var bool + */ + private $pendingCommit = false; + /** * Initializes a new UnitOfWork instance, bound to the given EntityManager. */ @@ -363,146 +371,156 @@ public function __construct(EntityManagerInterface $em) */ public function commit($entity = null) { - if ($entity !== null) { - Deprecation::triggerIfCalledFromOutside( - 'doctrine/orm', - 'https://github.com/doctrine/orm/issues/8459', - 'Calling %s() with any arguments to commit specific entities is deprecated and will not be supported in Doctrine ORM 3.0.', - __METHOD__ - ); + if ($this->pendingCommit) { + throw new LogicException('The UnitOfWork is currently in the commit phase. You must not call UnitOfWork::commit() or EntityManager::flush() at this time, since commit phases cannot safely be nested.'); } - $connection = $this->em->getConnection(); + $this->pendingCommit = true; - if ($connection instanceof PrimaryReadReplicaConnection) { - $connection->ensureConnectedToPrimary(); - } - - // Raise preFlush - if ($this->evm->hasListeners(Events::preFlush)) { - $this->evm->dispatchEvent(Events::preFlush, new PreFlushEventArgs($this->em)); - } - - // Compute changes done since last commit. - if ($entity === null) { - $this->computeChangeSets(); - } elseif (is_object($entity)) { - $this->computeSingleEntityChangeSet($entity); - } elseif (is_array($entity)) { - foreach ($entity as $object) { - $this->computeSingleEntityChangeSet($object); - } - } - - if ( - ! ($this->entityInsertions || - $this->entityDeletions || - $this->entityUpdates || - $this->collectionUpdates || - $this->collectionDeletions || - $this->orphanRemovals) - ) { - $this->dispatchOnFlushEvent(); - $this->dispatchPostFlushEvent(); + try { + if ($entity !== null) { + Deprecation::triggerIfCalledFromOutside( + 'doctrine/orm', + 'https://github.com/doctrine/orm/issues/8459', + 'Calling %s() with any arguments to commit specific entities is deprecated and will not be supported in Doctrine ORM 3.0.', + __METHOD__ + ); + } - $this->postCommitCleanup($entity); + $connection = $this->em->getConnection(); - return; // Nothing to do. - } + if ($connection instanceof PrimaryReadReplicaConnection) { + $connection->ensureConnectedToPrimary(); + } - $this->assertThatThereAreNoUnintentionallyNonPersistedAssociations(); + // Raise preFlush + if ($this->evm->hasListeners(Events::preFlush)) { + $this->evm->dispatchEvent(Events::preFlush, new PreFlushEventArgs($this->em)); + } - if ($this->orphanRemovals) { - foreach ($this->orphanRemovals as $orphan) { - $this->remove($orphan); + // Compute changes done since last commit. + if ($entity === null) { + $this->computeChangeSets(); + } elseif (is_object($entity)) { + $this->computeSingleEntityChangeSet($entity); + } elseif (is_array($entity)) { + foreach ($entity as $object) { + $this->computeSingleEntityChangeSet($object); + } } - } - $this->dispatchOnFlushEvent(); + if ( + ! ($this->entityInsertions + || $this->entityDeletions + || $this->entityUpdates + || $this->collectionUpdates + || $this->collectionDeletions + || $this->orphanRemovals) + ) { + $this->dispatchOnFlushEvent(); + $this->dispatchPostFlushEvent(); - $conn = $this->em->getConnection(); - $conn->beginTransaction(); + $this->postCommitCleanup($entity); - try { - // Collection deletions (deletions of complete collections) - foreach ($this->collectionDeletions as $collectionToDelete) { - // Deferred explicit tracked collections can be removed only when owning relation was persisted - $owner = $collectionToDelete->getOwner(); + return; // Nothing to do. + } - if ($this->em->getClassMetadata(get_class($owner))->isChangeTrackingDeferredImplicit() || $this->isScheduledForDirtyCheck($owner)) { - $this->getCollectionPersister($collectionToDelete->getMapping())->delete($collectionToDelete); + $this->assertThatThereAreNoUnintentionallyNonPersistedAssociations(); + + if ($this->orphanRemovals) { + foreach ($this->orphanRemovals as $orphan) { + $this->remove($orphan); } } - if ($this->entityInsertions) { - // Perform entity insertions first, so that all new entities have their rows in the database - // and can be referred to by foreign keys. The commit order only needs to take new entities - // into account (new entities referring to other new entities), since all other types (entities - // with updates or scheduled deletions) are currently not a problem, since they are already - // in the database. - $this->executeInserts(); - } + $this->dispatchOnFlushEvent(); - if ($this->entityUpdates) { - // Updates do not need to follow a particular order - $this->executeUpdates(); - } + $conn = $this->em->getConnection(); + $conn->beginTransaction(); - // Extra updates that were requested by persisters. - // This may include foreign keys that could not be set when an entity was inserted, - // which may happen in the case of circular foreign key relationships. - if ($this->extraUpdates) { - $this->executeExtraUpdates(); - } + try { + // Collection deletions (deletions of complete collections) + foreach ($this->collectionDeletions as $collectionToDelete) { + // Deferred explicit tracked collections can be removed only when owning relation was persisted + $owner = $collectionToDelete->getOwner(); - // Collection updates (deleteRows, updateRows, insertRows) - // No particular order is necessary, since all entities themselves are already - // in the database - foreach ($this->collectionUpdates as $collectionToUpdate) { - $this->getCollectionPersister($collectionToUpdate->getMapping())->update($collectionToUpdate); - } + if ($this->em->getClassMetadata(get_class($owner))->isChangeTrackingDeferredImplicit() || $this->isScheduledForDirtyCheck($owner)) { + $this->getCollectionPersister($collectionToDelete->getMapping())->delete($collectionToDelete); + } + } - // Entity deletions come last. Their order only needs to take care of other deletions - // (first delete entities depending upon others, before deleting depended-upon entities). - if ($this->entityDeletions) { - $this->executeDeletions(); - } + if ($this->entityInsertions) { + // Perform entity insertions first, so that all new entities have their rows in the database + // and can be referred to by foreign keys. The commit order only needs to take new entities + // into account (new entities referring to other new entities), since all other types (entities + // with updates or scheduled deletions) are currently not a problem, since they are already + // in the database. + $this->executeInserts(); + } - // Commit failed silently - if ($conn->commit() === false) { - $object = is_object($entity) ? $entity : null; + if ($this->entityUpdates) { + // Updates do not need to follow a particular order + $this->executeUpdates(); + } - throw new OptimisticLockException('Commit failed', $object); - } - } catch (Throwable $e) { - $this->em->close(); + // Extra updates that were requested by persisters. + // This may include foreign keys that could not be set when an entity was inserted, + // which may happen in the case of circular foreign key relationships. + if ($this->extraUpdates) { + $this->executeExtraUpdates(); + } - if ($conn->isTransactionActive()) { - $conn->rollBack(); - } + // Collection updates (deleteRows, updateRows, insertRows) + // No particular order is necessary, since all entities themselves are already + // in the database + foreach ($this->collectionUpdates as $collectionToUpdate) { + $this->getCollectionPersister($collectionToUpdate->getMapping())->update($collectionToUpdate); + } - $this->afterTransactionRolledBack(); + // Entity deletions come last. Their order only needs to take care of other deletions + // (first delete entities depending upon others, before deleting depended-upon entities). + if ($this->entityDeletions) { + $this->executeDeletions(); + } - throw $e; - } + // Commit failed silently + if ($conn->commit() === false) { + $object = is_object($entity) ? $entity : null; - $this->afterTransactionComplete(); + throw new OptimisticLockException('Commit failed', $object); + } + } catch (Throwable $e) { + $this->em->close(); - // Unset removed entities from collections, and take new snapshots from - // all visited collections. - foreach ($this->visitedCollections as $coid => $coll) { - if (isset($this->pendingCollectionElementRemovals[$coid])) { - foreach ($this->pendingCollectionElementRemovals[$coid] as $key => $valueIgnored) { - unset($coll[$key]); + if ($conn->isTransactionActive()) { + $conn->rollBack(); } + + $this->afterTransactionRolledBack(); + + throw $e; } - $coll->takeSnapshot(); - } + $this->afterTransactionComplete(); + + // Unset removed entities from collections, and take new snapshots from + // all visited collections. + foreach ($this->visitedCollections as $coid => $coll) { + if (isset($this->pendingCollectionElementRemovals[$coid])) { + foreach ($this->pendingCollectionElementRemovals[$coid] as $key => $valueIgnored) { + unset($coll[$key]); + } + } - $this->dispatchPostFlushEvent(); + $coll->takeSnapshot(); + } - $this->postCommitCleanup($entity); + $this->dispatchPostFlushEvent(); + + $this->postCommitCleanup($entity); + } finally { + $this->pendingCommit = false; + } } /** @param object|object[]|null $entity */