DI Configuration: Circural references

6 years ago

Filip Procházka
Moderator | 4693
+
0
-

When I was writing compiler extension for google/api-php-client, I had to solve a nasty circural reference. The class Google_Client is a simplified composition root for the classes of that repository and I needed to extend few of them and then set them back to the client, but all of them require the client in constructor.

So the simplified code would look like this

$builder->addDefinition($this->prefix('apiClient'))
    ->setClass('Google_Client')
    ->addSetup('setIo', array($this->prefix('@apiIo')));

$builder->addDefinition($this->prefix('apiIo'))
    ->setClass('Kdyby\Google\IO\Curl');

But this throws circural reference exception, becase the Curl cannot be created before the client is, but the client requires the curl to be configured.

I know it could be partially solved like this

$builder->addDefinition($this->prefix('apiClient'))
    ->setClass('Google_Client')
    ->addSetup('setIo', array(new Nette\DI\Statement('Kdyby\Google\IO\Curl', array('@self'))));

But this means I wouldn't have the other instance as service, loosing some benefits that I might need.

The simplest solution to this situation is to register the client service immediately after it's instantiated.

$builder->addDefinition($this->prefix('apiClient'))
    ->setClass('Google_Client')
    ->addSetup('$this->addService(?, ?)', array($this->prefix('apiClient'), '@self'))
    ->addSetup('setIo', array($this->prefix('@apiIo')));

$builder->addDefinition($this->prefix('apiIo'))
    ->setClass('Kdyby\Google\IO\Curl');

This way, the service is available for the curl.


This is ofcourse an ugly hack, but I was thinking that this might in fact be the default behaviour.

If the compiler would generate this code for services, 95% of circural references would be solved natively.

public function createServiceGoogle__apiClient()
{
    // $service = new Google_Client($this->getService('google.apiConfig'));
    $this->registry['google.apiClient'] = $service = new Google_Client($this->getService('google.apiConfig'));
    return $service;
}

It might create some side effects, that when there is a problem and exception is thrown during creation of the service, the service is still available. This could be solved by wrapping contents of the method in try/catch {unset(...)}, and with additional check in Container::createService() method.

Or there might be some “stash” for services that are beeing created, so instead of setting the service instance directly to Container::$registry, it would be set to some Container::$stash that would make it “temporarily” available for the time that part of the service graph is beeing created.

What do you guys think? I know there is already one RFC to solve this, but I didn't like that one, becase it adds too much method calls to DI Container, that must be as fast as possible.

6 years ago

Jan Tvrdík
Nette guru | 2563
+
0
-

I like the “stash” solution.

6 years ago

David Grudl
Nette Core | 6886
+
0
-

What about save $this->registry to a $tmp in createService and restore on exception?

6 years ago

Filip Procházka
Moderator | 4693
+
0
-

Do you think that would be faster and more memory effective? (I don't know, just asking).

6 years ago

Jan Tvrdík
Nette guru | 2563
+
0
-

My guess is that stash will require more code, but will be the fastest and cleanest solution.

5 years ago

mystik
Member | 77
+
0
-

Circular reference is often sign of design error. But sometimes there is no simple way to avoid it. I proposed solution before. Look here https://forum.nette.org/…v-containeru#… Basically it is only variant of your stash solution. What I think is that this behaviour should not be default but activated explicitly. Because you will be passing only partially initialized service. If you did not do any real work in constructor which you should not it does not matter but I think you should activate this feature only if you know what you are doing.

Last edited by mystik (2014-05-28 21:15)

5 years ago

Filip Procházka
Moderator | 4693
+
0
-

Your solution was too complex. I wouldn't wanna see that in Nette. (But let's not talk about that here, here we should discuss this RFC).

5 years ago

mystik
Member | 77
+
0
-

I agree solution proposed here is better than mine. It is much simpler with equivalent outcome. I just think this behaviour should be explicitly enabled not implicit.

5 years ago

Filip Procházka
Moderator | 4693
+
0
-

If it were implemented using the stash, there is no reason to have to enable it explicitly.

5 years ago

mystik
Member | 77
+
0
-

The problem could occur if partially initialized service is used during creation of service graph. If some service try to use partially initialized service which it just received in constructor or by setup it can result in unexpected behaviour. I think it would be better throw circular reference exception in this case unless it was explicitly stated that service can be passed partially initialized. It is not necessary but think it can prevent some hard to debug problems.

5 years ago

Filip Procházka
Moderator | 4693
+
0
-

The solution to that is not to register services to “the registry”, but always to stash during their creation. And when that's done, add them to “the registry”.

5 years ago

mystik
Member | 77
+
0
-

Yes, but if I understand your solution right then partially initialized services from stah will be passed to their dependencies.

In your original example when service apiIo is created it gets partially initialized apiClinet passed in it's constuctor.

In this case it is ok, but imagine situation where implementation of apiIo constructor will expect that apiClient is fully initialized a use it. If it throws exception then it will fail immediately and you can find solution.

But what if will be used something like this:

class ServiceA() {

  private $serviceB;
  private $serviceC;

  public function setServiceB($service) {
    $this->serviceB = $service;
  }

  public function setServiceC($service) {
    $this->serviceC = $service;
  }

  public function createSomethingWithServiceC() {
    return new Something($this->serviceC);
  }
class ServiceB() {

  private $serviceA;
  private $something;

  public function __construct($serviceA) {
    $this->serviceA = $serviceA;
    $this->something = $serviceA->createSomethingWithServiceC();
  }

Then can this sequence of call occur:

  • $serviceA = newServiceA()
  • $container->stash['serviceA'] = $serviceA
  • when trying to call setup for setServiceB creation of ServiceB is triggered
  • $serviceB = newServiceB($container->stash['serviceA'])
  • serviceB is creted but in it's $this->something is wrongly initialized Something because null was passed to it's constuctor, setServiceC of ServiceA was not called yet
  • we return to setup of ServiceA
  • $serviceA->setServiceB($serviceB)
  • $serviceA->setServiceC($serviceC)

This problem can be very hard to debug buecause when you try to test it from outside of container when you call createSomething() you get corect result, serviceC seems to be correctly set in serviceA yet we still have Something in serviceB with null passed to it's constructor.

Or did I somehow misunderstood how your solution should work?

Last edited by mystik (2014-05-29 08:12)

5 years ago

Filip Procházka
Moderator | 4693
+
0
-

First of all, the apiClient and the apiIO would have to be both in stash untill they're fully initialized.

To your other problem – logic in constructors is an anti-pattern. In such a complex situation, you will have a problem and will get a fatal error saing you're calling a method on non-object or something similar. The point of this RFC is to solve the basic circural references, not all of them.

The problem can be solved by carefully explaining it in documentation.

5 years ago

mystik
Member | 77
+
0
-

Well I agree with all your points :-)

  • problem occurs only during initalization of circulary references services (when they are all in stash)
  • logic in constructor is anti-patters but I still see it in beginners code quite often, if you do not do any real work in constructor or methods called during service setup it is ok (I wrote that before)
  • if this cause error or exception during service setup then again everything is ok (you get error and you know that you need to fix something in service setup)
  • problem occurs if this setup error will be silent and will cause problems later, after service is initialized. In my example when somebody try to use $something any time after services are created.
  • For such cases I would prefer fail-fast with circular reference exception during service setup.

In my opinion you should explicitly state that some service can be passed to it's dependencies during setup partially initalized. Therefore is you meet conditions described in my example, you will see in code that this service is explicitly allowed passed partially initialized. It it is problem for your code you can change it's structure, setup order or soemthing to solve it.

For example with something like:

$builder->addDefinition($this->prefix('apiClient'))
    ->setClass('Google_Client')
    ->addSetup('setIo', array($this->prefix('@apiIo')));
    ->deferredSetup()

or:

serviceA:
  class: \My\ServiceA
  deferredSetup: true

(I'm not sure about right name – it could be something like lazySetup, allowCircular, …)

Similarly I think that you should get warning that you have circular reference. It it will solve it silently you can miss that fact.

But maybe this would be such exceptional conditions it does not make sense to complicate service creation with it and just let container to solve it for you automatically.

Last edited by mystik (2014-05-29 14:32)

5 years ago

David Grudl
Nette Core | 6886
+
0
-

I think it can be tested in master, will you prepare PR?

5 years ago

Filip Procházka
Moderator | 4693
+
0
-

I will try to prepare it.

4 years ago

enumag
Member | 2128
+
0
-

+1 for this although I think the stash is not actually needed. I can't think of an use-case where I would catch a exception raised during service creation let alone that I would still use the DIC afterward. Putting the service directly to the registry would be fine in my opinion, the stash seems like a useless overhead.