Skip to content

Commit

Permalink
Mitigate problems with EntityManager::flush() reentrance since 2.16…
Browse files Browse the repository at this point in the history
….0 (Take 2)

The changes from doctrine#10547, which landed in 2.16.0, cause problems for users calling `EntityManager::flush()` from within `postPersist` event listeners.

* When `UnitOfWork::commit()` is re-entered, the "inner" (reentrant) call will start working through all changesets. Eventually, it finishes with all insertions being performed and `UoW::$entityInsertions` being empty. After return, the entity insertion order, an array computed at the beginning of `UnitOfWork::executeInserts()`, still contains entities that now have been processed already. This leads to a strange-looking SQL error where the number of parameters used does not match the number of parameters bound. This has been reported as doctrine#10869.

* The fixes made to the commit order computation may lead to a different entity insertion order than previously. `postPersist` listener code may be affected by this when accessing generated IDs for other entities than the one the event has been dispatched for. This ID may not yet be available when the insertion order is different from the one that was used before 2.16. This has been mentioned in doctrine#10906 (comment).

This PR suggests to address both issues by dispatching the `postPersist` event only after _all_ new entities have their rows inserted into the database. Likewise, dispatch `postRemove` only after _all_ deletions have been executed.

This solves the first issue because the sequence of insertions or deletions has been processed completely _before_ we start calling event listeners. This way, potential changes made by listeners will no longer be relevant.

Regarding the second issue, I think deferring `postPersist` a bit until _all_ entities have been inserted does not violate any promises given, hence is not a BC break. In 2.15, this event was raised after all insertions _for a particular class_ had been processed - so, it was never an "immediate" event for every single entity. doctrine#10547 moved the event handling to directly after every single insertion. Now, this PR moves it back a bit to after _all_ insertions.
  • Loading branch information
mpdude committed Aug 21, 2023
1 parent 5577d51 commit 5517ba0
Show file tree
Hide file tree
Showing 4 changed files with 212 additions and 11 deletions.
33 changes: 27 additions & 6 deletions docs/en/reference/events.rst
Original file line number Diff line number Diff line change
Expand Up @@ -705,13 +705,21 @@ not directly mapped by Doctrine.
- The ``postUpdate`` event occurs after the database
update operations to entity data. It is not called for a DQL
``UPDATE`` statement.
- The ``postPersist`` event occurs for an entity after
the entity has been made persistent. It will be invoked after the
database insert operation for that entity. A generated primary key value for
the entity will be available in the postPersist event.
- The ``postPersist`` event occurs for an entity after the entity has
been made persistent. It will be invoked after all database insert
operations for new entities have been performed. Generated primary
key values will be available for all entities at the time this
event is triggered.
- The ``postRemove`` event occurs for an entity after the
entity has been deleted. It will be invoked after the database
delete operations. It is not called for a DQL ``DELETE`` statement.
entity has been deleted. It will be invoked after all database
delete operations for entity rows have been executed. This event is
not called for a DQL ``DELETE`` statement.

.. note::

At the time ``postPersist`` is called, there may still be collection and/or
"extra" updates pending. The database may not yet be completely in
sync with the entity states in memory, not even for the new entities.

.. warning::

Expand All @@ -720,6 +728,19 @@ not directly mapped by Doctrine.
cascade remove relations. In this case, you should load yourself the proxy in
the associated ``pre*`` event.

.. warning::

Making changes to entities and calling ``EntityManager::flush()`` from within
``post*`` event handlers is strongly discouraged, and might be deprecated and
eventually prevented in the future.

The reason is that it causes re-entrance into ``UnitOfWork::commit()`` while a commit
is currently being processed. The ``UnitOfWork`` was never designed to support this,
and its behavior in this situation is not covered by any tests.

This may lead to entity or collection updates being missed, applied only in parts and
changes being lost at the end of the commit phase.

.. _reference-events-post-load:

postLoad
Expand Down
24 changes: 19 additions & 5 deletions lib/Doctrine/ORM/UnitOfWork.php
Original file line number Diff line number Diff line change
Expand Up @@ -1164,13 +1164,13 @@ public function recomputeSingleEntityChangeSet(ClassMetadata $class, $entity)
*/
private function executeInserts(): void
{
$entities = $this->computeInsertExecutionOrder();
$entities = $this->computeInsertExecutionOrder();
$eventsToDispatch = [];

foreach ($entities as $entity) {
$oid = spl_object_id($entity);
$class = $this->em->getClassMetadata(get_class($entity));
$persister = $this->getEntityPersister($class->name);
$invoke = $this->listenersInvoker->getSubscribedSystems($class, Events::postPersist);

$persister->addInsert($entity);

Expand All @@ -1197,10 +1197,18 @@ private function executeInserts(): void
$this->addToEntityIdentifiersAndEntityMap($class, $oid, $entity);
}

$invoke = $this->listenersInvoker->getSubscribedSystems($class, Events::postPersist);

if ($invoke !== ListenersInvoker::INVOKE_NONE) {
$this->listenersInvoker->invoke($class, Events::postPersist, $entity, new PostPersistEventArgs($entity, $this->em), $invoke);
$eventsToDispatch[] = ['class' => $class, 'entity' => $entity, 'invoke' => $invoke];
}
}

// Defer dispatching `postPersist` events to until all entities have been inserted and post-insert
// IDs have been assigned.
foreach ($eventsToDispatch as $event) {
$this->listenersInvoker->invoke($event['class'], Events::postPersist, $event['entity'], new PostPersistEventArgs($event['entity'], $this->em), $event['invoke']);
}
}

/**
Expand Down Expand Up @@ -1270,7 +1278,8 @@ private function executeUpdates(): void
*/
private function executeDeletions(): void
{
$entities = $this->computeDeleteExecutionOrder();
$entities = $this->computeDeleteExecutionOrder();
$eventsToDispatch = [];

foreach ($entities as $entity) {
$oid = spl_object_id($entity);
Expand All @@ -1295,9 +1304,14 @@ private function executeDeletions(): void
}

if ($invoke !== ListenersInvoker::INVOKE_NONE) {
$this->listenersInvoker->invoke($class, Events::postRemove, $entity, new PostRemoveEventArgs($entity, $this->em), $invoke);
$eventsToDispatch[] = ['class' => $class, 'entity' => $entity, 'invoke' => $invoke];
}
}

// Defer dispatching `postRemove` events to until all entities have been removed.
foreach ($eventsToDispatch as $event) {
$this->listenersInvoker->invoke($event['class'], Events::postRemove, $event['entity'], new PostRemoveEventArgs($event['entity'], $this->em), $event['invoke']);
}
}

/** @return list<object> */
Expand Down
91 changes: 91 additions & 0 deletions tests/Doctrine/Tests/ORM/Functional/EntityListenersTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,18 @@

namespace Doctrine\Tests\ORM\Functional;

use Doctrine\ORM\Event\PostPersistEventArgs;
use Doctrine\ORM\Event\PostRemoveEventArgs;
use Doctrine\ORM\Event\PreFlushEventArgs;
use Doctrine\ORM\Event\PreUpdateEventArgs;
use Doctrine\ORM\Events;
use Doctrine\ORM\UnitOfWork;
use Doctrine\Persistence\Event\LifecycleEventArgs;
use Doctrine\Tests\Models\Company\CompanyContractListener;
use Doctrine\Tests\Models\Company\CompanyFixContract;
use Doctrine\Tests\Models\Company\CompanyPerson;
use Doctrine\Tests\OrmFunctionalTestCase;
use PHPUnit\Framework\Assert;

/** @group DDC-1955 */
class EntityListenersTest extends OrmFunctionalTestCase
Expand Down Expand Up @@ -96,6 +102,45 @@ public function testPostPersistListeners(): void
self::assertInstanceOf(LifecycleEventArgs::class, $this->listener->postPersistCalls[0][1]);
}

public function testPostPersistCalledAfterAllInsertsHaveBeenPerformedAndIdsHaveBeenAssigned(): void
{
$object1 = new CompanyFixContract();
$object1->setFixPrice(2000);

$object2 = new CompanyPerson();
$object2->setName('J. Doe');

$this->_em->persist($object1);
$this->_em->persist($object2);

$listener = new class ([$object1, $object2]) {
/** @var array<object> */
private $trackedObjects;

/** @var int */
public $invocationCount = 0;

public function __construct(array $trackedObjects)
{
$this->trackedObjects = $trackedObjects;
}

public function postPersist(PostPersistEventArgs $args): void
{
foreach ($this->trackedObjects as $object) {
Assert::assertNotNull($object->getId());
}

++$this->invocationCount;
}
};

$this->_em->getEventManager()->addEventListener(Events::postPersist, $listener);
$this->_em->flush();

self::assertSame(2, $listener->invocationCount);
}

public function testPreUpdateListeners(): void
{
$fix = new CompanyFixContract();
Expand Down Expand Up @@ -175,4 +220,50 @@ public function testPostRemoveListeners(): void
self::assertInstanceOf(CompanyFixContract::class, $this->listener->postRemoveCalls[0][0]);
self::assertInstanceOf(LifecycleEventArgs::class, $this->listener->postRemoveCalls[0][1]);
}

public function testPostRemoveCalledAfterAllRemovalsHaveBeenPerformed(): void
{
$object1 = new CompanyFixContract();
$object1->setFixPrice(2000);

$object2 = new CompanyPerson();
$object2->setName('J. Doe');

$this->_em->persist($object1);
$this->_em->persist($object2);
$this->_em->flush();

$listener = new class ($this->_em->getUnitOfWork(), [$object1, $object2]) {
/** @var UnitOfWork */
private $uow;

/** @var array<object> */
private $trackedObjects;

/** @var int */
public $invocationCount = 0;

public function __construct(UnitOfWork $uow, array $trackedObjects)
{
$this->uow = $uow;
$this->trackedObjects = $trackedObjects;
}

public function postRemove(PostRemoveEventArgs $args): void
{
foreach ($this->trackedObjects as $object) {
Assert::assertFalse($this->uow->isInIdentityMap($object));
}

++$this->invocationCount;
}
};

$this->_em->getEventManager()->addEventListener(Events::postRemove, $listener);
$this->_em->remove($object1);
$this->_em->remove($object2);
$this->_em->flush();

self::assertSame(2, $listener->invocationCount);
}
}
75 changes: 75 additions & 0 deletions tests/Doctrine/Tests/ORM/Functional/Ticket/GH10869Test.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
<?php

declare(strict_types=1);

namespace Doctrine\Tests\ORM\Functional\Ticket;

use Doctrine\ORM\Event\PostPersistEventArgs;
use Doctrine\ORM\Events;
use Doctrine\ORM\Mapping as ORM;
use Doctrine\Tests\OrmFunctionalTestCase;

use function sprintf;

class GH10869Test extends OrmFunctionalTestCase
{
protected function setUp(): void
{
parent::setUp();

$this->setUpEntitySchema([
GH10869Entity::class,
]);
}

public function testPostPersistListenerUpdatingObjectFieldWhileOtherInsertPending(): void
{
$entity1 = new GH10869Entity();
$this->_em->persist($entity1);

$entity2 = new GH10869Entity();
$this->_em->persist($entity2);

$this->_em->getEventManager()->addEventListener(Events::postPersist, new class {
public function postPersist(PostPersistEventArgs $args): void
{
$object = $args->getObject();

$objectManager = $args->getObjectManager();
$object->field = sprintf('test %s', $object->id);
$objectManager->flush();
}
});

$this->_em->flush();
$this->_em->clear();

$entity1Reloaded = $this->_em->find(GH10869Entity::class, $entity1->id);
self::assertSame($entity1->field, $entity1Reloaded->field);

$entity2Reloaded = $this->_em->find(GH10869Entity::class, $entity2->id);
self::assertSame($entity2->field, $entity2Reloaded->field);
}
}

/**
* @ORM\Entity
*/
class GH10869Entity
{
/**
* @ORM\Id
* @ORM\GeneratedValue
* @ORM\Column(type="integer")
*
* @var ?int
*/
public $id;

/**
* @ORM\Column(type="text", nullable=true)
*
* @var ?string
*/
public $field;
}

0 comments on commit 5517ba0

Please sign in to comment.