doctrine / DoctrineModule

Doctrine Module for Laminas
http://www.doctrine-project.org/projects/doctrine-module.html
MIT License
398 stars 269 forks source link

bind Closure to null to allow the garbage collector do its work #664

Closed rieschl closed 5 years ago

rieschl commented 5 years ago

The closure did bind to $this or self and therefore couldn't be garbage collected.

rieschl commented 5 years ago

Credit goes to @Xerkus for discovering it 😄

rieschl commented 5 years ago

To write a test for it we would need Weakref, which is probably not possible.

Ocramius commented 5 years ago

To write a test for it we would need Weakref, which is probably not possible.

A test case would use memory_get_usage(), IMO

rieschl commented 5 years ago

A test case would use memory_get_usage(), IMO

I changed the fix (and squashed the commit) to use static but I'm not sure if I understand PHP internals good enough to write a meaningful test using memory_get_usage().

rieschl commented 5 years ago

Any chance someone could write a test for that so it can be merged?

michalbundyra commented 5 years ago

@rieschl I was looking on it and really I don't know how it should work... I came up with:

    public function testInit()
    {
        $moduleManager = $this->prophesize(ModuleManager::class);

        echo PHP_EOL;
        echo memory_get_usage(), PHP_EOL;
        $module = new Module();
        for ($i = 0; $i < 10000; ++$i) {
            $module->init($moduleManager->reveal());
        }
        echo memory_get_usage(), PHP_EOL;
        AnnotationRegistry::reset();
        echo memory_get_usage(), PHP_EOL;
    }

but with/without your changes results are as I would expect:

@Ocramius would you be able to help, please?

TomHAnderson commented 5 years ago

I'm ready to merge this unless someone wants to raise issue as-is.