From 770425c75abbb875f58f5bfdc3903dfee05afcc9 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Wed, 1 Dec 2021 17:25:24 +0100 Subject: [PATCH] Prevent infinite nesting of lazy `ObjectManager` instances when `ObjectManager` is reset This patch ensures that the `ObjectManager` that the `Symfony\Bridge\Doctrine\ManagerRegistry` replaces during `Symfony\Bridge\Doctrine\ManagerRegistry#resetService()` operations is a fresh non-lazy service. Before this change, `Symfony\Bridge\Doctrine\ManagerRegistry#resetService()` would replace the initialization of the lazy proxy with a new service each time, but that service being lazy, this led to an additional proxy nesting level at each service reset call. That leads to general issues around memory reliability, stack trace nesting (and therefore bigger logged traces) and potentially even stack overflow problems, when running with XDebug. The problem seems to only apply when the `symfony/dependency-injection` `Container` is compiled as a set of small factory files: that's because each generated factory has a boolean flag that indicates whether a lazy or non-lazy version of a service is requested, as introduced in the original implementation at https://github.com/symfony/symfony/pull/7890 --- .../Bridge/Doctrine/ManagerRegistry.php | 2 +- .../Doctrine/Tests/ManagerRegistryTest.php | 93 +++++++++++++++++++ 2 files changed, 94 insertions(+), 1 deletion(-) diff --git a/src/Symfony/Bridge/Doctrine/ManagerRegistry.php b/src/Symfony/Bridge/Doctrine/ManagerRegistry.php index 2139562dedbaf..0ed055fec64b7 100644 --- a/src/Symfony/Bridge/Doctrine/ManagerRegistry.php +++ b/src/Symfony/Bridge/Doctrine/ManagerRegistry.php @@ -62,7 +62,7 @@ function (&$wrappedInstance, LazyLoadingInterface $manager) use ($name) { $name = $this->aliases[$name]; } if (isset($this->fileMap[$name])) { - $wrappedInstance = $this->load($this->fileMap[$name]); + $wrappedInstance = $this->load($this->fileMap[$name], false); } else { $method = $this->methodMap[$name] ?? 'get'.strtr($name, $this->underscoreMap).'Service'; // BC with DI v3.4 $wrappedInstance = $this->{$method}(false); diff --git a/src/Symfony/Bridge/Doctrine/Tests/ManagerRegistryTest.php b/src/Symfony/Bridge/Doctrine/Tests/ManagerRegistryTest.php index a004935a6afdc..dcdc4ce6360fc 100644 --- a/src/Symfony/Bridge/Doctrine/Tests/ManagerRegistryTest.php +++ b/src/Symfony/Bridge/Doctrine/Tests/ManagerRegistryTest.php @@ -12,8 +12,15 @@ namespace Symfony\Bridge\Doctrine\Tests; use PHPUnit\Framework\TestCase; +use ProxyManager\Proxy\LazyLoadingInterface; +use ProxyManager\Proxy\ValueHolderInterface; use Symfony\Bridge\Doctrine\ManagerRegistry; +use Symfony\Bridge\ProxyManager\LazyProxy\PhpDumper\ProxyDumper; use Symfony\Bridge\ProxyManager\Tests\LazyProxy\Dumper\PhpDumperTest; +use Symfony\Component\DependencyInjection\ContainerBuilder; +use Symfony\Component\DependencyInjection\ContainerInterface; +use Symfony\Component\DependencyInjection\Dumper\PhpDumper; +use Symfony\Component\Filesystem\Filesystem; class ManagerRegistryTest extends TestCase { @@ -39,6 +46,92 @@ public function testResetService() $this->assertSame($foo, $container->get('foo')); $this->assertObjectNotHasAttribute('bar', $foo); } + + /** + * When performing an entity manager lazy service reset, the reset operations may re-use the container + * to create a "fresh" service: when doing so, it can happen that the "fresh" service is itself a proxy. + * + * Because of that, the proxy will be populated with a wrapped value that is itself a proxy: repeating + * the reset operation keeps increasing this nesting until the application eventually runs into stack + * overflow or memory overflow operations, which can happen for long-running processes that rely on + * services that are reset very often. + */ + public function testResetServiceWillNotNestFurtherLazyServicesWithinEachOther() + { + // This test scenario only applies to containers composed as a set of generated sources + $this->dumpLazyServiceProjectAsFilesServiceContainer(); + + /** @var ContainerInterface $container */ + $container = new \LazyServiceProjectAsFilesServiceContainer(); + + $registry = new TestManagerRegistry( + 'irrelevant', + [], + ['defaultManager' => 'foo'], + 'irrelevant', + 'defaultManager', + 'irrelevant' + ); + $registry->setTestContainer($container); + + $service = $container->get('foo'); + + self::assertInstanceOf(\stdClass::class, $service); + self::assertInstanceOf(LazyLoadingInterface::class, $service); + self::assertInstanceOf(ValueHolderInterface::class, $service); + self::assertFalse($service->isProxyInitialized()); + + $service->initializeProxy(); + + self::assertTrue($container->initialized('foo')); + self::assertTrue($service->isProxyInitialized()); + + $registry->resetManager(); + $service->initializeProxy(); + + $wrappedValue = $service->getWrappedValueHolderValue(); + self::assertInstanceOf(\stdClass::class, $wrappedValue); + self::assertNotInstanceOf(LazyLoadingInterface::class, $wrappedValue); + self::assertNotInstanceOf(ValueHolderInterface::class, $wrappedValue); + } + + /** @return void */ + private function dumpLazyServiceProjectAsFilesServiceContainer() + { + if (class_exists(\LazyServiceProjectAsFilesServiceContainer::class, false)) { + return; + } + + $container = new ContainerBuilder(); + + $container->register('foo', \stdClass::class) + ->setPublic(true) + ->setLazy(true); + $container->compile(); + + $fileSystem = new Filesystem(); + + $temporaryPath = $fileSystem->tempnam(sys_get_temp_dir(), 'symfonyManagerRegistryTest'); + $fileSystem->remove($temporaryPath); + $fileSystem->mkdir($temporaryPath); + + $dumper = new PhpDumper($container); + + $dumper->setProxyDumper(new ProxyDumper()); + $containerFiles = $dumper->dump([ + 'class' => 'LazyServiceProjectAsFilesServiceContainer', + 'as_files' => true, + ]); + + array_walk( + $containerFiles, + static function (string $containerSources, string $fileName) use ($temporaryPath): void { + (new Filesystem())->dumpFile($temporaryPath.'/'.$fileName, $containerSources); + } + ); + + require $temporaryPath.'/LazyServiceProjectAsFilesServiceContainer.php'; + } } class TestManagerRegistry extends ManagerRegistry