fixed outdated entity

[messyOne]

I found an issue with outdated entities. If you have following code which will be called by your request:

$em->beginTransaction();
try {
    $repository = $em->getRepository(Entity::class);
    /** @var Entity $entity */
    $entity = $repository->findOneBy(['tyef' => 'foo']);
    $em->lock($entity, LockMode::PESSIMISTIC_WRITE);
    $entity->increase(1); //increase field 'bar'

    sleep(2);

    $em->persist($entity);

    $em->flush();
    $em->commit();
} catch (\Exception $e) {
    $em->rollback();
    throw $e;
}

This will happen:

1. Before requests: Number is 30 (in fieldbar)
2. Request 1 is fired
3. Request 2 is fired
4. Request 1 finished. Number is 31
5. Request 2 finished. Number is 31

Since we have a write lock inside of a transaction I expect to have 32. The entity is not up to date and has do be refreshed. If you use the EntityRepository::find() method with the write lock it works fine. It only occurs if you add the lock afterwards.

[doctrinebot]

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-3943

We use Jira to track the state of pull requests and the versions they got
included in.

[Ocramius]

@messyOne automatically refreshing the entity is a quite unwanted side-effect, no? It should be up to the user to call the refresh() in my opinion (thinking about current codebases)

[kimhemsoe]

It looks to me like you have a possible race between these 2 lines.

$entity = $repository->findOneBy(['tyef' => 'foo']);
$em->lock($entity, LockMode::PESSIMISTIC_WRITE);

[messyOne]

@Ocramius But for what do I have the lock then? It is the same behavior as without having a lock at the moment. Fetching it with find(1, LockMode::PESSIMISTIC_WRITE) would result in the expected result so I thought doing it with $em->lock() should be the same.

@kimhemsoe What do you mean exactly?

[kimhemsoe]

What i mean is that it possible for the data in the database to have changed between you loading the data and you locking the data. You should find and lock at the same time.

[messyOne]

@kimhemsoe That is true. That is what I'm doing currently. I overwrote the findOneBy to delegate the lock mode. But I thought it should work out of the box.

[DHager]

What about having the auto-refreshing responsibility in \Doctrine\ORM\Persisters\BasicEntityPersister::lock() instead?

Currently it does a SELECT 1 ... to do the locking on the same entity's row, but changing that to also retrieve the relevant columns would let you avoid a second SQL query.