zenstruck / foundry

A model factory library for creating expressive, auto-completable, on-demand dev/test fixtures with Symfony and Doctrine.
https://symfony.com/bundles/ZenstruckFoundryBundle/current/index.html
MIT License
607 stars 62 forks source link

Upgrade from 1.37 to 1.38 introduce a memory leak #626

Closed jdecool closed 5 days ago

jdecool commented 2 weeks ago

I've just upgraded this library from 1.37.0 to 1.38.0 and it broke my CI.

After a quick investigation I've notice that some test consume a lot of memory.

For example: a test class consume ~110Mb with the 1.37 and now consume ~637Mb after the upgrade.

PS: I track the source of the memory leak, I will update this issue.

nikophil commented 2 weeks ago

Hi @jdecool :wave:

that's... surprising! :sweat_smile:

I did not notice this problem in any project where I've tested this whole migration path :thinking:

I track the source of the memory leak, I will update this issue.

OK, thanks, let's tackle this

kbond commented 2 weeks ago

Could it be related to all the deprecations being called? That's probably too large a spike for this to be the culprit.

jdecool commented 1 week ago

Could it be related to all the deprecations being called? That's probably too large a spike for this to be the culprit.

I don't think that could explain the spike especially as I run the Rector Rule to fix them.

I track the source of the memory leak, I will update this issue.

I'm sorry for the delay of the investigation but I don't have much time to do this.

So I tried to update the project to Foundry 2 to see if it fix this issue.

But I have a problem.

I've a factory like this:

final class MyEntityFactory extends PersistentProxyObjectFactory
{
    protected function defaults(): array|callable
    {
        return [
            // ...
            'telephoneMobile' => self::faker()->optional(0.8)->mobileNumber(),
            'telephoneAutre' => self::faker()->optional(0.3)->phoneNumber(),
        ];
    }

    protected function initialize(): static
    {
        // see https://symfony.com/bundles/ZenstruckFoundryBundle/current/index.html#initialization
        return $this
            ->beforeInstantiate(function (array $attributes): array {
                // ...

                // Transform string into PhoneNumber object
                $attributes['telephoneMobile'] = $this->parsePhoneNumber($attributes['telephoneMobile']);
                $attributes['telephoneAutre'] = $this->parsePhoneNumber($attributes['telephoneAutre']);

                return $attributes;
            })
        ;
    }
}

The parsePhoneNumber return an instance of libphonenumber\PhoneNumber.

In foundry 2, this object is now wrap in a proxy using the Symfony\Component\VarExporter\Symfony\Component\VarExporter. The problem that the PhoneNumber class already define a public function unserialize($serialized) that is not compatible with the Symfony trait (public function __unserialize(array $data): void).

So it will throw an exception.

I don't know how to tell Foundry to not use a proxy for this object.

jdecool commented 1 week ago

I'm sorry for the delay of the investigation but I don't have much time to do this.

Currently I've just seen that there is a lot of strings that are staying in memory.

I'm looking for which part of the code keep those strings in memory.

kbond commented 1 week ago

I don't know how to tell Foundry to not use a proxy for this object.

PhoneNumber is wrapped in a proxy? Is it an entity?

jdecool commented 1 week ago

Yes from an external package

kbond commented 1 week ago

And you created a factory class for it? In foundry 2 you can have a factory extend PersistentObjectFactory (instead of PersistentProxyObjectFactory) and created objects won't be proxied.

FYI, we are working on some proxy improvements that should solve this issue soon.

pmontoya commented 1 week ago

I use the same library and I have the same issue. It's not a factory for this class. This class is used for a property of an entity. The library declare the function __unserialize($data) and LazyProxyTrait declare the function __unserialize(array $data): void

kbond commented 1 week ago

Ok, I'm still not sure how PhoneNumber is being proxied. @jdecool, can you share what your parsePhoneNumber looks like?

jdecool commented 1 week ago

Sorry. Just realise my issue is incomplete.

Je PhoneNumber come from https://packagist.org/packages/giggsey/libphonenumber-for-php and I use https://packagist.org/packages/odolbeau/phone-number-bundle for the Symfony integration.

I don’t use a Factory to construct this object, I create an instance manually.

kbond commented 1 week ago

I don’t use a Factory to construct this object, I create an instance manually.

But it's still wrapped in a proxy? This is the part I don't understand - I don't know how this can happen.

Anyway, we're in a big aside here from the original purpose of this issue - feel free to open a second issue to discuss this proxy problem further - it is legit.

jdecool commented 1 week ago

feel free to open a second issue to discuss this proxy problem further - it is legit.

https://github.com/zenstruck/foundry/issues/639

nikophil commented 6 days ago

still eager to understand what happened here. Or at least, this would be nice to know if the memory leak does not occur in v2

jdecool commented 6 days ago

still eager to understand what happened here.

Hard to find the time to correctly investigate this issue in depth.

Or at least, this would be nice to know if the memory leak does not occur in v2

Just update the project to v2 and there's no memory leak anymore.

nikophil commented 5 days ago

ok, that's nice! No need to investigate IMO

jdecool commented 5 days ago

No need to investigate IMO

I've made other tests.

Memory leak occurred in 1.38.0 and 1.38.1 but seems to be fixed in 1.38.2

We can close this issue.

Thanks @nikophil & @kbond

Really appreciate your work on this library.