Skip to content

Commit

Permalink
Prevent UnitOfWork::commit() reentrance
Browse files Browse the repository at this point in the history 
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 doctrine#10547, it causes strange-looking, non-helpful error messages (doctrine#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.
  • Loading branch information
@mpdude
mpdude committed Aug 9, 2023
1 parent 597a63a commit 8b0c5ac
Showing 1 changed file with 127 additions and 109 deletions.
236 changes: 127 additions & 109 deletions lib/Doctrine/ORM/UnitOfWork.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
use Doctrine\Persistence\PropertyChangedListener;
use Exception;
use InvalidArgumentException;
use LogicException;
use RuntimeException;
use Throwable;
use UnexpectedValueException;
Expand Down Expand Up @@ -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.
*/
Expand Down Expand Up @@ -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 */
Expand Down

1 comment on commit 8b0c5ac

@mpdude
Copy link
Owner Author

@mpdude mpdude commented on 8b0c5ac Aug 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmperruchini This commit is not the right place for a discussion. Please use doctrine#10900.

Apart from that, you're showing personally identifiable information in your comment above. You may want to remove that information.

Please sign in to comment.