Skip to content
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

[DI] Fix circular reference when using setters #25180

Merged
merged 1 commit into from Nov 29, 2017

Conversation

@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Nov 27, 2017

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

I did not manage to make a reproducing test case, yet @deguif provided me an app that I could play with to debug and fix.

@sroze
Copy link
Member

@sroze sroze commented Nov 28, 2017

@nicolas-grekas you can imagine that we definitely need tests for that 😄 Where is the application you played with? Which branch is failing without this patch?

Loading

@deguif
Copy link
Contributor

@deguif deguif commented Nov 28, 2017

Here's a reproducing test case.
Thanks @nicolas-grekas for your time yesterday on this. I was wrong, the service we were playing with were marked as shared = false by a compiler pass. So the bug you encounter by declaring an unshared service is the same.

Service declarations

<service id="foo" class="Foo" shared="false">
    <call method="setFooBar">
        <argument type="service" id="foo_bar" />
    </call>
</service>

<service id="foo_bar" class="FooBar">
    <argument type="collection">
        <argument type="service" id="foo" />
    </argument>
</service>

This will lead to this container service dump (which causes an infinite loop).

<?php

use Symfony\Component\DependencyInjection\Argument\RewindableGenerator;

// This file has been auto-generated by the Symfony Dependency Injection Component for internal use.
// Returns the private 'foo_bar' shared service.

$a = new \Foo();
$a->setFooBar(${($_ = isset($this->services['foo_bar']) ? $this->services['foo_bar'] : $this->load(__DIR__.'/getFooBarService.php')) && false ?: '_'});

return $this->services['foo_bar'] = new \FooBar(array(0 => $a));

Loading

@stof
Copy link
Member

@stof stof commented Nov 28, 2017

Circular setter injection for shared services works fine in some cases, as the instance is registered in $this->services before calling the setter on it. But this is not always the case. Here are a few case which break things:

  • injecting the service in the constructor of the main service (as done here for foo_bar), as this requires building the dependency before injecting it in the setter
  • using only non-shared services in the cycle (we need a shared service in the cycle to break the instantiation loop)
  • using a non-shared service as main service would create weird things (as this instance would not be registered, and so the loop would instantiate it again in the cycle). Note that the first shared service reached in the cycle inherits the restrictions of the "main" service.

What I called "main" services are all the services which can trigger the beginning of the cycle:
Here is what identifies the "main" services of a cycle, for which we have the restriction about not involving its constructor in the cycle:

  • all public services (as they can be retrieved through get)
  • all shared services referenced from outside this cycle (as the cycle can start when resolving the reference)

Loading

@stof
Copy link
Member

@stof stof commented Nov 28, 2017

However, we need to be sure we don't break BC for cases working fine.

Loading

@stof
Copy link
Member

@stof stof commented Nov 28, 2017

and this means we should write test covering the working case to, to avoid breaking it

Loading

@deguif
Copy link
Contributor

@deguif deguif commented Nov 28, 2017

Thanks @stof for your explanation
I got this issue when upgrading from Symfony 3.3 to 3.4.
So currently there is a BC break as I'm not able to upgrade to Symfony 3.4 without adapting the service definitions.

Loading

@nicolas-grekas
Copy link
Member Author

@nicolas-grekas nicolas-grekas commented Nov 28, 2017

The issue is deeper than anticipated: there are much more cases where we can generate code that loop infinitely.
I just pushed a patch that should fix that, but is missing many tests for now (so not all situations are proven as OK yet.)
I'll finish tomorrow. If you want to have a look meanwhile, that's possible.

Loading

@sroze
Copy link
Member

@sroze sroze commented Nov 28, 2017

Loading

@nicolas-grekas nicolas-grekas force-pushed the di-fix-circular branch 6 times, most recently from 1a467d5 to e01466d Nov 29, 2017
Copy link
Member Author

@nicolas-grekas nicolas-grekas left a comment

PR is ready. It's bigger that I anticipated, yet it's very important as it fixes the dumped container. On 3.3, the issue also exists, but is less likely to hit for two independent reasons:

  • the dumped container on 3.3 still uses $this->get() internally for public services, which has circular ref bundled
  • much more services are public

Loading

@@ -570,10 +570,13 @@ public function get($id, $invalidBehavior = ContainerInterface::EXCEPTION_ON_INV
return $this->doGet($id, $invalidBehavior);
}

private function doGet($id, $invalidBehavior = ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE)
private function doGet($id, $invalidBehavior = ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE, array &$inlineServices = array())
Copy link
Member Author

@nicolas-grekas nicolas-grekas Nov 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a reference is used several times to construct one service, only one instance of non-shared services should be created. That's already the case for the dumped container. Here is the fix for ContainerBuilder. Tested below, with "foobar4" case.

Loading

/**
* @expectedException \Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException
*/
public function testCircularReference()
Copy link
Member Author

@nicolas-grekas nicolas-grekas Nov 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this previous failure is not legitimate anymore: there is no circular reference here

Loading

if ('\\' === DIRECTORY_SEPARATOR) {
$dump = str_replace("'\\\\includes\\\\HotPath\\\\", "'/includes/HotPath/", $dump);
}

$this->assertStringEqualsFile(self::$fixturesPath.'/php/container_inline_requires.php', $dump);
$this->assertStringEqualsFile(self::$fixturesPath.'/php/services_inline_requires.php', $dump);
Copy link
Member Author

@nicolas-grekas nicolas-grekas Nov 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

naming was inconsistent with other files in the folder

Loading

@nicolas-grekas nicolas-grekas merged commit de5eecc into symfony:3.4 Nov 29, 2017
2 of 3 checks passed
Loading
nicolas-grekas added a commit that referenced this issue Nov 29, 2017
…ekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[DI] Fix circular reference when using setters

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

I did not manage to make a reproducing test case, yet @deguif provided me an app that I could play with to debug and fix.

Commits
-------

de5eecc [DI] Fix circular reference when using setters
@nicolas-grekas nicolas-grekas deleted the di-fix-circular branch Nov 29, 2017
@nicolas-grekas
Copy link
Member Author

@nicolas-grekas nicolas-grekas commented Nov 29, 2017

@symfony/deciders I merged this PR because it's ready on my side, and it's important to have before 4.0.0, so that I wanted to allow as many ppl as possible to test it, which is easier now.
But any review is still welcomed, I'll respond quickly to any report/comments until tomorrow, before the release.

Loading

@fabpot fabpot mentioned this pull request Nov 30, 2017
@fabpot fabpot mentioned this pull request Nov 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants