-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Form] Memory leak in EntityType when using phppm #23975
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
Comments
One more; I made this quick&dirty solution that is clearing memory via Reflection: public function onKernelTerminate()
{
$choiceLoadersReflection = $this->getReflection();
$choiceLoadersReflection->setValue($this->entityType, []);
}
private function getReflection()
{
if (!$this->reflection) {
$reflection = new \ReflectionObject($this->entityType);
$doctrineType = $reflection->getParentClass();
$loaders = $doctrineType->getProperty('choiceLoaders');
$loaders->setAccessible(true);
$this->reflection = $loaders;
}
return $this->reflection;
} I am aware it is bad but it works. |
If I'm understanding this issue correctly: You can probably solve this by not binding your entity to a form directly, but using a DTO: https://stovepipe.systems/post/avoiding-entities-in-forms |
@iltar You are right, I was thinking about using DTO but I find that solution as too complex (no offence). For example, if I have a form with collection and that collection has its own collection, extra code needed would go beserk. Example of such form is editing album with tagged users. Check this: S3 form system would automatically call adders and removers. So at least for now, I would prefer using entities directly. With
EntityType::class keeps all those objects in My solution with ReflectionObject is bad. mostly because it prevents decorating it via |
@iltar On topic of DTO, I did try it to avoid this issue. To avoid extra code, I did something like this: class MovieDTO extends Movie
{
// no code here
} Extra service copied values from Movie to MovieDTO by reading form fields. It sortof worked, can't remember what the problem was but I am thinking of extending FormType with key English is not my first language so I hope the above makes sense, 😄 |
Well, the way phppm works means that any kind of memoization turns into long-term memory leaks, because it got rid of the fact that the PHP process lives only until the end of the request processing. Your proposal would require that any library doing memoization would implement the same interface (and so depend on the package containing it as it is not part of PHP) to be compatible with phppm (to allow to build such a listener calling their clean method). I don't think making this interface part of Symfony itself will work (external libraries doing memoization will not want to depend on Symfony for that), and I'm not even sure they would be willing to make such change to accommodate a need specific to phppm. |
I was thinking of something like I made for this problem (bundle here): class MemoryCleaner implements EventSubscriberInterface
{
/**
* @var MemoryCleanerInterface[]
*/
private $cleaners;
/**
* $cleaners set in @see MemoryCleanerPass
*
* @param MemoryCleanerInterface[] $cleaners
*/
public function __construct(array $cleaners = [])
{
$this->cleaners = $cleaners;
}
public static function getSubscribedEvents()
{
return [
'kernel.terminate' => 'onKernelTerminate',
];
}
public function onKernelTerminate()
{
foreach ($this->cleaners as $cleaner) {
$cleaner->clean();
}
}
} and this pass: class MemoryCleanerPass implements CompilerPassInterface
{
public function process(ContainerBuilder $builder)
{
if (!$builder->hasDefinition(MemoryCleaner::class)) {
return;
}
$definition = $builder->getDefinition(MemoryCleaner::class);
$taggedCleaners = $builder->findTaggedServiceIds(MemoryCleanerInterface::class);
$cleaners = [];
foreach ($taggedCleaners as $id => $tags) {
$cleaners[] = new Reference($id);
}
$definition->setArgument(0, $cleaners);
}
} and this line in Extension::load() $builder->registerForAutoconfiguration(MemoryCleanerInterface::class)->addTag(MemoryCleanerInterface::class); So it is totally optional, if someone wants to clean memory, he only needs to implement the interface. If it goes into standard Symfony distro, I think people would quickly adopt it because phppm is just too good. |
Closed in favor of #23984 |
I am playing around with phppm and found memory leak in EntityType::class. This ticket on phppm repository explains the problem in details.
In short, EntityType caches results in local property so same query will not be executed more than once. Given that I do $em->clear() in kernel.terminate event (otherwise, identity map would quickly fill up), I get exception on next request when submitting the same form.
I am reporting this as bug and not a feature request because I saw other tickets about memory leaks being reported the same.
This might sound funny but what if a service is tagged with for example
memory_clean
, it would need to implementMemoryCleanInterface
with onlyclean()
method.And on
kernel.terminate
event, this method would be called on all these services. It could be done with a simple listener but this would bring consistency for future bundles when local caching is needed.What do you think?
The text was updated successfully, but these errors were encountered: