Skip to content

Commit

Permalink
Mitigate problems with EntityManager::flush() reentrance since 2.16.0
Browse files Browse the repository at this point in the history
This PR addresses the issue brought up in doctrine#10869. It happens when users use event listeners – `postPersist` in particular – to make changes to entities and/or persist new entities and finally call `EntityManager::flush()`, while the `UnitOfWork` is currently within the commit phase.

There is a discussion in doctrine#10900 whether this kind of reentrance should be deprecated in 2.x and prevented in 3.x. But, in order to prevent complete breakage with the 2.16.0 update, this PR tries to apply a band-aid 🩹.

A few things changed inside the UoW commit implementation in 2.16, and for sure this PR does not restore all details of the previous behavior. Take it with a grain of salt.

Here's the details.

The issue appears when `UoW::commit()` is performing entity insertions, and `postPersist` listener causes `commit()` reentrance when there are pending insertions left.

In that situation, the "inner" (reentrant) call will start working through all changesets. Eventually, it finishes with all insertions being performed and `UoW::$entityInsertions` being empty.

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 the reported failure down the road. The mitigation is to check for this condition and skip such entities.

Before doctrine#10547 (pre-2.16), things worked a bit differently:

The UoW did not build a list of entities (objects) as the commit order, but a sequence of _classes_ to process.

For every entity _class_, it would find all entity instances in `UoW::$entityInsertions` and pass them to the persister in a batch. The persister, in turn, would execute the `INSERT` statements for _all_ those entities _before_ it dispatched the `postInsert` events.

That means when a `postInsert` listener was notified pre-2.16, the UoW was in a state where 1) all insertions for a particular class had been written to the database already and 2) the UoW had not yet gathered the next round of entities to process.
  • Loading branch information
mpdude committed Aug 17, 2023
1 parent 597a63a commit 1895cf8
Show file tree
Hide file tree
Showing 2 changed files with 95 additions and 1 deletion.
21 changes: 20 additions & 1 deletion lib/Doctrine/ORM/UnitOfWork.php
Original file line number Diff line number Diff line change
Expand Up @@ -1167,7 +1167,26 @@ private function executeInserts(): void
$entities = $this->computeInsertExecutionOrder();

foreach ($entities as $entity) {
$oid = spl_object_id($entity);
$oid = spl_object_id($entity);

// Mitigation for GH-10869:
// Users may use postPersist and similar listeners to make entity updates and call
// EM::flush() -> UoW::commit() again, while a transaction is currently running.
// This "somehow" worked pre 2.16, although it was never officially endorsed and/or
// is disputed (and there is no guarantee that the UoW will be able to deal with this,
// does not lose updates etc.).
// https://github.com/doctrine/orm/pull/10900 is a discussion about deprecating this
// kind of reentrance and disallowing it in 3.0.
//
// However, to ease the pain somewhat for users in 2.16, this condition covers that
// a reentrant call into UoW::commit() may have processed pending insertions that we
// had in our computed insertion order. So, after the second (inner) commit() returned
// and the outer one continues, deal with the situation that entities are no longer in
// the set of pending insertions.
if (! isset($this->entityInsertions[$oid])) {
continue;
}

$class = $this->em->getClassMetadata(get_class($entity));
$persister = $this->getEntityPersister($class->name);
$invoke = $this->listenersInvoker->getSubscribedSystems($class, Events::postPersist);
Expand Down
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 1895cf8

Please sign in to comment.