Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix desynchronized entity managers after kernel reset #1118

Merged

Conversation

ostrolucky
Copy link
Member

@ostrolucky ostrolucky commented Jan 5, 2020

Fixes #1114

We can't reset because of this symfony/symfony#35216 + performance issues, so let's just do clear() so it works for long running processes (same thing php-pm does for years).

@ostrolucky ostrolucky added the Bug label Jan 5, 2020
@ostrolucky ostrolucky added this to the 1.12.7 milestone Jan 5, 2020
@ostrolucky ostrolucky self-assigned this Jan 5, 2020
@ostrolucky ostrolucky force-pushed the fix-inconsistent-maps-after-kernel-reset branch 3 times, most recently from 7d8f315 to 561af7f Compare January 5, 2020 03:48
@bendavies
Copy link
Contributor

This is what I was going to propose.
just clearing seems enough. resetting in case of an error if the EM has become closed in still necessary.

This also fixes a large performance regression seen in tests and probably php-pm, whereby resetting the entity managers every request unloads all the mapping metadata, which means it is loaded every request.

@ostrolucky
Copy link
Member Author

I'm not entirely happy with this because reset semantics are in bridge and we are now working around that here. If these semantics are wrong, it should be changed there. All we should ideally do is just call resetService, without conditionals on our side. That said, it's our fault for causing breaks and symfony team is ignoring the issue, so we must do it on our side, at least until they fix it.

In some scenarios involving long running processes and proxies,
resetting the entity manager results in a creation of a new entity
manager, while some services still hold a reference to the old one,
which keeps its previous state.

This abandons resetting the entity manager in favor of a call to clear()
on the existing entity manager if it is open.

Fixes doctrine#1112
@greg0ire greg0ire force-pushed the fix-inconsistent-maps-after-kernel-reset branch from 561af7f to 7c6f6ed Compare January 8, 2020 18:46
@greg0ire greg0ire requested a review from a team January 8, 2020 18:49
@alcaeus alcaeus merged commit 18fb7d2 into doctrine:1.12.x Jan 10, 2020
@alcaeus
Copy link
Member

alcaeus commented Jan 10, 2020

Merging this as it definitely fixes the duplicated service issue we've been experiencing. We're still discussing how to proceed with other breaks introduced by resetting the manager.

Thanks @ostrolucky!

@alcaeus alcaeus removed the request for review from a team January 10, 2020 12:27
@acasademont
Copy link

Thanks @ostrolucky, this should be enough :)

We also merged a check in php-pm to not clear the EM if the Registry already implements the ResetInterface

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants