bug #39058 [DependencyInjection] Fix circular detection with multiple paths (jderusse)
This PR was merged into the 4.4 branch.
Discussion
----------
[DependencyInjection] Fix circular detection with multiple paths
| Q | A
| ------------- | ---
| Branch? | 4.4
| Bug fix? | yes
| New feature? | no
| Deprecations? | no
| Tickets | Fix #39056
| License | MIT
| Doc PR | -
There are currently 2 kind of issues related to the Dependency Injection:
1. performance issue when project contains many loops (#37850)
Which has been fixed by #38882
2. Infinity loop in some case (#38970)
Which has been fixed by #38980 and #39021
The new issue #39056 has been introduced by #38882 (The performance issue refactor) because in order to optimize loop detection, I take a short cut and choose to not collect ALL the circular loop but only the one that matters
I was wrong. All loops matters.
This PR fix my previous refacto to collect ALL the paths, with a low CPU footprint
Commits
-------
1c3721e8ad
Fix circular detection with multiple paths
This commit is contained in:
commit
faead9574a
@ -413,14 +413,13 @@ EOF;
|
|||||||
$this->singleUsePrivateIds = array_diff_key($this->singleUsePrivateIds, $this->circularReferences);
|
$this->singleUsePrivateIds = array_diff_key($this->singleUsePrivateIds, $this->circularReferences);
|
||||||
}
|
}
|
||||||
|
|
||||||
private function collectCircularReferences(string $sourceId, array $edges, array &$checkedNodes, array $path = [], bool $byConstructor = true): void
|
private function collectCircularReferences(string $sourceId, array $edges, array &$checkedNodes, array &$loops = [], array $path = [], bool $byConstructor = true): void
|
||||||
{
|
{
|
||||||
$path[$sourceId] = $byConstructor;
|
$path[$sourceId] = $byConstructor;
|
||||||
$checkedNodes[$sourceId] = true;
|
$checkedNodes[$sourceId] = true;
|
||||||
foreach ($edges as $edge) {
|
foreach ($edges as $edge) {
|
||||||
$node = $edge->getDestNode();
|
$node = $edge->getDestNode();
|
||||||
$id = $node->getId();
|
$id = $node->getId();
|
||||||
|
|
||||||
if (!($definition = $node->getValue()) instanceof Definition || $sourceId === $id || ($edge->isLazy() && ($this->proxyDumper ?? $this->getProxyDumper())->isProxyCandidate($definition)) || $edge->isWeak()) {
|
if (!($definition = $node->getValue()) instanceof Definition || $sourceId === $id || ($edge->isLazy() && ($this->proxyDumper ?? $this->getProxyDumper())->isProxyCandidate($definition)) || $edge->isWeak()) {
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
@ -428,9 +427,12 @@ EOF;
|
|||||||
if (isset($path[$id])) {
|
if (isset($path[$id])) {
|
||||||
$loop = null;
|
$loop = null;
|
||||||
$loopByConstructor = $edge->isReferencedByConstructor();
|
$loopByConstructor = $edge->isReferencedByConstructor();
|
||||||
|
$pathInLoop = [$id, []];
|
||||||
foreach ($path as $k => $pathByConstructor) {
|
foreach ($path as $k => $pathByConstructor) {
|
||||||
if (null !== $loop) {
|
if (null !== $loop) {
|
||||||
$loop[] = $k;
|
$loop[] = $k;
|
||||||
|
$pathInLoop[1][$k] = $pathByConstructor;
|
||||||
|
$loops[$k][] = &$pathInLoop;
|
||||||
$loopByConstructor = $loopByConstructor && $pathByConstructor;
|
$loopByConstructor = $loopByConstructor && $pathByConstructor;
|
||||||
} elseif ($k === $id) {
|
} elseif ($k === $id) {
|
||||||
$loop = [];
|
$loop = [];
|
||||||
@ -438,7 +440,39 @@ EOF;
|
|||||||
}
|
}
|
||||||
$this->addCircularReferences($id, $loop, $loopByConstructor);
|
$this->addCircularReferences($id, $loop, $loopByConstructor);
|
||||||
} elseif (!isset($checkedNodes[$id])) {
|
} elseif (!isset($checkedNodes[$id])) {
|
||||||
$this->collectCircularReferences($id, $node->getOutEdges(), $checkedNodes, $path, $edge->isReferencedByConstructor());
|
$this->collectCircularReferences($id, $node->getOutEdges(), $checkedNodes, $loops, $path, $edge->isReferencedByConstructor());
|
||||||
|
} elseif (isset($loops[$id])) {
|
||||||
|
// we already had detected loops for this edge
|
||||||
|
// let's check if we have a common ancestor in one of the detected loops
|
||||||
|
foreach ($loops[$id] as [$first, $loopPath]) {
|
||||||
|
if (!isset($path[$first])) {
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
// We have a common ancestor, let's fill the current path
|
||||||
|
$fillPath = null;
|
||||||
|
foreach ($loopPath as $k => $pathByConstructor) {
|
||||||
|
if (null !== $fillPath) {
|
||||||
|
$fillPath[$k] = $pathByConstructor;
|
||||||
|
} elseif ($k === $id) {
|
||||||
|
$fillPath = $path;
|
||||||
|
$fillPath[$k] = $pathByConstructor;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// we can now build the loop
|
||||||
|
$loop = null;
|
||||||
|
$loopByConstructor = $edge->isReferencedByConstructor();
|
||||||
|
foreach ($fillPath as $k => $pathByConstructor) {
|
||||||
|
if (null !== $loop) {
|
||||||
|
$loop[] = $k;
|
||||||
|
$loopByConstructor = $loopByConstructor && $pathByConstructor;
|
||||||
|
} elseif ($k === $first) {
|
||||||
|
$loop = [];
|
||||||
|
}
|
||||||
|
}
|
||||||
|
$this->addCircularReferences($first, $loop, true);
|
||||||
|
break;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
unset($path[$sourceId]);
|
unset($path[$sourceId]);
|
||||||
|
@ -1374,6 +1374,9 @@ class ContainerBuilderTest extends TestCase
|
|||||||
$container = include __DIR__.'/Fixtures/containers/container_almost_circular.php';
|
$container = include __DIR__.'/Fixtures/containers/container_almost_circular.php';
|
||||||
$container->compile();
|
$container->compile();
|
||||||
|
|
||||||
|
$pA = $container->get('pA');
|
||||||
|
$this->assertEquals(new \stdClass(), $pA);
|
||||||
|
|
||||||
$logger = $container->get('monolog.logger');
|
$logger = $container->get('monolog.logger');
|
||||||
$this->assertEquals(new \stdClass(), $logger->handler);
|
$this->assertEquals(new \stdClass(), $logger->handler);
|
||||||
|
|
||||||
|
@ -1054,6 +1054,9 @@ class PhpDumperTest extends TestCase
|
|||||||
|
|
||||||
$container = new $container();
|
$container = new $container();
|
||||||
|
|
||||||
|
$pA = $container->get('pA');
|
||||||
|
$this->assertEquals(new \stdClass(), $pA);
|
||||||
|
|
||||||
$logger = $container->get('monolog.logger');
|
$logger = $container->get('monolog.logger');
|
||||||
$this->assertEquals(new \stdClass(), $logger->handler);
|
$this->assertEquals(new \stdClass(), $logger->handler);
|
||||||
|
|
||||||
|
@ -9,6 +9,21 @@ use Symfony\Component\DependencyInjection\Tests\Fixtures\FooForCircularWithAddCa
|
|||||||
$public = 'public' === $visibility;
|
$public = 'public' === $visibility;
|
||||||
$container = new ContainerBuilder();
|
$container = new ContainerBuilder();
|
||||||
|
|
||||||
|
// multiple path detection
|
||||||
|
|
||||||
|
$container->register('pA', 'stdClass')->setPublic(true)
|
||||||
|
->addArgument(new Reference('pB'))
|
||||||
|
->addArgument(new Reference('pC'));
|
||||||
|
|
||||||
|
$container->register('pB', 'stdClass')->setPublic($public)
|
||||||
|
->setProperty('d', new Reference('pD'));
|
||||||
|
$container->register('pC', 'stdClass')->setPublic($public)
|
||||||
|
->setLazy(true)
|
||||||
|
->setProperty('d', new Reference('pD'));
|
||||||
|
|
||||||
|
$container->register('pD', 'stdClass')->setPublic($public)
|
||||||
|
->addArgument(new Reference('pA'));
|
||||||
|
|
||||||
// monolog-like + handler that require monolog
|
// monolog-like + handler that require monolog
|
||||||
|
|
||||||
$container->register('monolog.logger', 'stdClass')->setPublic(true)
|
$container->register('monolog.logger', 'stdClass')->setPublic(true)
|
||||||
|
@ -40,6 +40,7 @@ class Symfony_DI_PhpDumper_Test_Almost_Circular_Private extends Container
|
|||||||
'manager2' => 'getManager2Service',
|
'manager2' => 'getManager2Service',
|
||||||
'manager3' => 'getManager3Service',
|
'manager3' => 'getManager3Service',
|
||||||
'monolog.logger' => 'getMonolog_LoggerService',
|
'monolog.logger' => 'getMonolog_LoggerService',
|
||||||
|
'pA' => 'getPAService',
|
||||||
'root' => 'getRootService',
|
'root' => 'getRootService',
|
||||||
'subscriber' => 'getSubscriberService',
|
'subscriber' => 'getSubscriberService',
|
||||||
];
|
];
|
||||||
@ -87,6 +88,9 @@ class Symfony_DI_PhpDumper_Test_Almost_Circular_Private extends Container
|
|||||||
'manager4' => true,
|
'manager4' => true,
|
||||||
'monolog.logger_2' => true,
|
'monolog.logger_2' => true,
|
||||||
'multiuse1' => true,
|
'multiuse1' => true,
|
||||||
|
'pB' => true,
|
||||||
|
'pC' => true,
|
||||||
|
'pD' => true,
|
||||||
'subscriber2' => true,
|
'subscriber2' => true,
|
||||||
];
|
];
|
||||||
}
|
}
|
||||||
@ -374,6 +378,28 @@ class Symfony_DI_PhpDumper_Test_Almost_Circular_Private extends Container
|
|||||||
return $instance;
|
return $instance;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Gets the public 'pA' shared service.
|
||||||
|
*
|
||||||
|
* @return \stdClass
|
||||||
|
*/
|
||||||
|
protected function getPAService()
|
||||||
|
{
|
||||||
|
$a = new \stdClass();
|
||||||
|
|
||||||
|
$b = ($this->privates['pC'] ?? $this->getPCService());
|
||||||
|
|
||||||
|
if (isset($this->services['pA'])) {
|
||||||
|
return $this->services['pA'];
|
||||||
|
}
|
||||||
|
|
||||||
|
$this->services['pA'] = $instance = new \stdClass($a, $b);
|
||||||
|
|
||||||
|
$a->d = ($this->privates['pD'] ?? $this->getPDService());
|
||||||
|
|
||||||
|
return $instance;
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Gets the public 'root' shared service.
|
* Gets the public 'root' shared service.
|
||||||
*
|
*
|
||||||
@ -481,4 +507,34 @@ class Symfony_DI_PhpDumper_Test_Almost_Circular_Private extends Container
|
|||||||
|
|
||||||
return $instance;
|
return $instance;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Gets the private 'pC' shared service.
|
||||||
|
*
|
||||||
|
* @return \stdClass
|
||||||
|
*/
|
||||||
|
protected function getPCService($lazyLoad = true)
|
||||||
|
{
|
||||||
|
$this->privates['pC'] = $instance = new \stdClass();
|
||||||
|
|
||||||
|
$instance->d = ($this->privates['pD'] ?? $this->getPDService());
|
||||||
|
|
||||||
|
return $instance;
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Gets the private 'pD' shared service.
|
||||||
|
*
|
||||||
|
* @return \stdClass
|
||||||
|
*/
|
||||||
|
protected function getPDService()
|
||||||
|
{
|
||||||
|
$a = ($this->services['pA'] ?? $this->getPAService());
|
||||||
|
|
||||||
|
if (isset($this->privates['pD'])) {
|
||||||
|
return $this->privates['pD'];
|
||||||
|
}
|
||||||
|
|
||||||
|
return $this->privates['pD'] = new \stdClass($a);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
@ -53,6 +53,10 @@ class Symfony_DI_PhpDumper_Test_Almost_Circular_Public extends Container
|
|||||||
'manager3' => 'getManager3Service',
|
'manager3' => 'getManager3Service',
|
||||||
'monolog.logger' => 'getMonolog_LoggerService',
|
'monolog.logger' => 'getMonolog_LoggerService',
|
||||||
'monolog.logger_2' => 'getMonolog_Logger2Service',
|
'monolog.logger_2' => 'getMonolog_Logger2Service',
|
||||||
|
'pA' => 'getPAService',
|
||||||
|
'pB' => 'getPBService',
|
||||||
|
'pC' => 'getPCService',
|
||||||
|
'pD' => 'getPDService',
|
||||||
'root' => 'getRootService',
|
'root' => 'getRootService',
|
||||||
'subscriber' => 'getSubscriberService',
|
'subscriber' => 'getSubscriberService',
|
||||||
];
|
];
|
||||||
@ -558,6 +562,71 @@ class Symfony_DI_PhpDumper_Test_Almost_Circular_Public extends Container
|
|||||||
return $instance;
|
return $instance;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Gets the public 'pA' shared service.
|
||||||
|
*
|
||||||
|
* @return \stdClass
|
||||||
|
*/
|
||||||
|
protected function getPAService()
|
||||||
|
{
|
||||||
|
$a = ($this->services['pB'] ?? $this->getPBService());
|
||||||
|
|
||||||
|
if (isset($this->services['pA'])) {
|
||||||
|
return $this->services['pA'];
|
||||||
|
}
|
||||||
|
$b = ($this->services['pC'] ?? $this->getPCService());
|
||||||
|
|
||||||
|
if (isset($this->services['pA'])) {
|
||||||
|
return $this->services['pA'];
|
||||||
|
}
|
||||||
|
|
||||||
|
return $this->services['pA'] = new \stdClass($a, $b);
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Gets the public 'pB' shared service.
|
||||||
|
*
|
||||||
|
* @return \stdClass
|
||||||
|
*/
|
||||||
|
protected function getPBService()
|
||||||
|
{
|
||||||
|
$this->services['pB'] = $instance = new \stdClass();
|
||||||
|
|
||||||
|
$instance->d = ($this->services['pD'] ?? $this->getPDService());
|
||||||
|
|
||||||
|
return $instance;
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Gets the public 'pC' shared service.
|
||||||
|
*
|
||||||
|
* @return \stdClass
|
||||||
|
*/
|
||||||
|
protected function getPCService($lazyLoad = true)
|
||||||
|
{
|
||||||
|
$this->services['pC'] = $instance = new \stdClass();
|
||||||
|
|
||||||
|
$instance->d = ($this->services['pD'] ?? $this->getPDService());
|
||||||
|
|
||||||
|
return $instance;
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Gets the public 'pD' shared service.
|
||||||
|
*
|
||||||
|
* @return \stdClass
|
||||||
|
*/
|
||||||
|
protected function getPDService()
|
||||||
|
{
|
||||||
|
$a = ($this->services['pA'] ?? $this->getPAService());
|
||||||
|
|
||||||
|
if (isset($this->services['pD'])) {
|
||||||
|
return $this->services['pD'];
|
||||||
|
}
|
||||||
|
|
||||||
|
return $this->services['pD'] = new \stdClass($a);
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Gets the public 'root' shared service.
|
* Gets the public 'root' shared service.
|
||||||
*
|
*
|
||||||
|
Reference in New Issue
Block a user