Doctrinebundle: Weird behaviour due to EM resets after update to 2.0.6

[acasademont]

Hi,

PHP-PM used to manually reset or clear the entity manager after every request (https://github.com/php-pm/php-pm-httpkernel/blob/master/Bootstraps/Symfony.php#L168). Now it seems like Symfony is able to automatically do it thanks to the kernel.reset tag but we're seeing a very strange behaviour.

On the first request all is fine,
On the second request, the EntityManager is reset at the beginning of the request, but the first ORM query is still going to the old EntityManager. The second ORM query sees that the manager has been reset, and instantiates another EM/UnitOfWork, effectively clearing the entityStates cache, which is bad, because in my code I have an entity created by the first query that the newly created UnitOfWork doesn't know anything about. If it is persisted, it will try an INSERT, instead of an UPDATE :(

To be honest, I don't really understand why the first ORM query after the reset is still using the old EM/UnitOfWork.

In any case, the EM shouldn't be reset after every request, a simple clear should be enough unless the EM hasn't been closed due to an Exception. In that case, yes, the manager should be reset. We've been working with that code in PHP-PM for a very long time and it works pretty well.

            foreach ($doctrineRegistry->getManagers() as $curManagerName => $curManager) {
                if (!$curManager->isOpen()) {
                    $doctrineRegistry->resetManager($curManagerName);
                } else {
                    $curManager->clear();
                }
            }

We've reverted to 2.0.2 in the meantime, but I'm not entirely sure how to solve this issue going forward.

[acasademont]

@dnna that's correct, we shouldn't clear in php-pm if things are working good on doctrine. But besides the weird behaviour, a full reset of the manager is not needed after every request, a simple clear should be enough unless the manager has been closed.

[dnna]

My guess is the issue was introduced with https://github.com/doctrine/DoctrineBundle/pull/1108, which was merged with 2.0.5. @acasademont out of curiosity could you try with 2.0.4 to make sure the issue is not occurring there?

[acasademont]

AFAIK this was first introduced in 2.0.3 https://github.com/doctrine/DoctrineBundle/compare/2.0.2...2.0.3 and was later fixed for the LazyLoading case afterwards, but the reset behaviour is the same, that's why we reverted to 2.0.2

[dnna]

AFAIK the kernel.reset behavior in Symfony works a little differently than what PHP-PM does:

• In Symfony, the service reset doesn't occur at the end of the current request, but at the start of the next one (https://github.com/symfony/http-kernel/blob/b99359da1c2c1421cf4b056b9a4df7f2478b66b2/Kernel.php#L103)
• In PHP-PM the reset (postHandle function) occurs at the end of the current request (https://github.com/php-pm/php-pm-httpkernel/blob/537a2c626da134aa80c4a4d1695794dee75616bb/Bridges/HttpKernel.php#L101)

Is it possible that your first ORM query runs somehow before the reset function in the kernel executes, so it uses the old manager?

[acasademont]

Not really, the reset is done on kernel boot, nothing has happened before that. I believe is more of an issue of how the SF container manages dependencies and services. Something similar seems to be happening in #1112

[bendavies]

should be resolved by using ServiceEntityRepository+autoconfigure or the doctrine.repository_service tag as provided in https://github.com/doctrine/DoctrineBundle/pull/727

[karser]

Just updated doctrine/doctrine-bundle (2.0.2 => 2.0.6) and encountered this bug, 16 tests are down :)
In most cases I'm getting this error while doing

$entity = $this->em->find(Entity::class, $id);
$this->apiClient->callApi('PUT', '/entities/'.$id, [...]);
$this->em->refresh($entity); <-------------------- Entity is not managed....

Reverted back to 2.0.2

Edited: btw, I'm using ServiceEntityRepository+autoconfigure

[ostrolucky]

I don't see how your suggestion with clear() instead of reset would help here. EM would loose access to identity map after clear() too, wouldn't it?

We could also use a reproducer, as description of issue is quite strange too. I don't see how how can reset happen while first query did not finish yet, that looks like some missed locking opportunity in php-pmhttpkernel.

[acasademont]

@ostrolucky they are 2 independent things I believe.

On the one hand, the weird behaviour which I don't really yet fully underestand why is it happening.
Hopefully I can take a deeper look this weekend.

On the other, that besides the issue, a full reset is not needed for what we're trying to achieve, a clear() should be enough. We've been using it for a very long time without any problems in php-pm.

[ostrolucky]

I would argue opposite. Class can have other side effects due to internal state not properly reset. Especially when using custom EMs. clear() is not enough and one exception of already mentioned isOpen() handling is proof. But such discussion is perhaps better to be had in symfony/symfony repository, since their doctrine bridge defines semantics of entity manager service reset. Surely there is no point of such hard reset from your POV, so you may suggest there to replace existing behaviour with your code and see the response.

Meanwhile, waiting for more information to troubleshoot the issue as I don't see how we can help otherwise.

[acasademont]

@ostrolucky the logic for deciding reset vs clear is on the Registry, not the bridge, https://github.com/doctrine/DoctrineBundle/blob/master/Registry.php#L59

@stof already raised the question (unanswered) of why we couldn't clear instead of reset the whole thing on the PR https://github.com/doctrine/DoctrineBundle/pull/1099#issuecomment-567098093

But anyways, this is not what this issue is about.

[acasademont]

Some more info. Seems like the first doctrine query from the Security fetch is done with the old entity manager, then it gets actually reset (due to the lazy proxy it's not reset immediately).

start of request
reset doctrine.orm.default_entity_manager
[2020-01-04 18:18:11] request.INFO: Matched route "gesdinet_jwt_refresh_token". {"route":"gesdinet_jwt_refresh_token","route_parameters":{"_route":"gesdinet_jwt_refresh_token","_controller":"gesdinet.jwtrefreshtoken::refresh"},"request_uri":"http://localhost:8080/api/token/refresh","method":"POST"} []
[2020-01-04 18:18:11] security.INFO: Populated the TokenStorage with an anonymous Token. [] []
[2020-01-04 18:18:11] doctrine.DEBUG: SELECT t0.refresh_token AS refresh_token_1, t0.username AS username_2, t0.valid AS valid_3, t0.id AS id_4 FROM refresh_tokens t0 WHERE t0.refresh_token = ? LIMIT 1 ["47fec6ec9b54dd3aa6b5b65229 [...]"] []
new entity manager Doctrine\ORM\EntityManager

The question is: why that first Doctrine query is not invoking the Proxy method that would trigger the reset?

[acasademont]

ok @ostrolucky I think I got it. See this code

class A {
    private $repository;

    public function __construct($em)
    {
        $this->repository = $em->getRepository('Foo');
    }

    public function getA($a)
    {
        return $this->repository->find($a);
    }
}

class B {
    private $em;

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

    public function getA($a)
    {
        return $this->em->find('Foo', $a);
    }
}

Imagine we have these 2 classes, with different ways of accessing the same entity Foo.

On class A, the EM is injected but then the Repository is left as a property of A and all queries go through that repo. On class B, the EM is left as a property and all queryies go directly through the EM.

On the first request, all is fine!

On the second request, at the beginning, the EM is reset. As the EM is a LazyProxy, the way of resetting it is that it will create a new instance of EM once an EM method is accessed.

However, on the second request, class A will still be using the old EM, it will never be reset, as no method of the proxy is ever called that forces to "reinstantiate". On class B, every request will reinstantiate the EM. Therefore, on the second request, we end up using 2 different EMs, with different UoW and different entityMaps. And that causes the weird behaviour I reported.

It shouldn't be hard to replicate this in a test, hope the issue is a bit more clear now.

[dnna]

But what is the solution in this case (aside from rewriting the app to inject the Registry instead of Manager and setting that as the property)?

It seems the PHP-PM method has worked because it basically never replaces the EM under normal circumstances (it performs clear unless an error occurred that caused the EM to close). If it always performed a reset instead of clear we would have observed the same issue as we do here.

In my opinion we should use clear in this bundle too (similar code to what PHP-PM uses) or revert this change and reserve for a major release, as the current reset method IMHO is causing a BC break.

[acasademont]

@dnna indeed, I just replicated the same behaviour with Doctrine 2.0.2 by forcing an EM closing (a DB exception while flushing changes) on the previous request. This is a deeper issue I'm afraid. The EM reset mechanism is not really working as expected. It's just that now this is more evident due to the forced EM resets after every request.

As you pointed, there are 2 solutions:

• clear the EM, not reset, knowing that full resets are only sctrictly needed when the EM has been closed due to a DB exception, and hope for the best (tbh, I haven't seen this issue in 2+ years we've been using php-pm in production, DB exceptions are rare, nobody has reported it either). Still, the underlying issue persists, we just "hide it".

• Make sure the EM injected inside repositories is the exact same LazyProxy EM instance, to avoid having more than 1 EM of the same kind in memory. I believe this is not possible due to the way the ProxyFactory works as a wrapper around the real EM instance

• Fix the reset mechanism, it can't really rely on reinstantiating the LazyProxy. There should probably be a proper reset() function inside the EM that allows reopening closed EMs?

[alcaeus]

The duplicate service issue is fixed by #1118 which will be released as 1.12.7 and 2.0.7 soon. We're still evaluating how to proceed with the other breaks caused by clearing the manager.