Lazy services - service proxies #7527

Closed
wants to merge 21 commits into
from

Projects

None yet

10 participants

@Ocramius
Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #6140 (supersedes) #5012 #6102 (maybe)
License MIT (attached code) - BSD-3-Clause (transitive optional dependency)
Doc PR Please pester me to death so I do it

This PR introduces lazy services along the lines of zendframework/zendframework#4146

It introduces an OPTIONAL dependency to ProxyManager and transitively to "zendframework/zend-code": "2.*".

Lazy services: why? A comprehensive example

For those who don't know what this is about, here's an example.

Assuming you have a service class like following:

class MySuperSlowClass
{
    public function __construct()
    {
        // inject large object graph or do heavy computation
        sleep(10);
    }

    public function doFoo()
    {
        echo 'Foo!';
    }
}

The DIC will hang for 10 seconds when calling:

$container->get('my_super_slow_class');

With this PR, this can be avoided, and the following call will return a proxy immediately.

$container->getDefinitions('my_super_slow_class')->setLazy(true);
$service = $container->get('my_super_slow_class');

The 10 seconds wait time will be delayed until the object is actually used:

$service->doFoo(); // wait 10 seconds, then 'Foo!'

A more extensive description of the functionality can be found here.

When do we need it?

Lazy services can be used to optimize the dependency graph in cases like:

  • Webservice endpoints
  • Db connections
  • Objects that cause I/O in general
  • Large dependency graphs that are not always used

This could also help in reducing excessive service location usage as I've explained here.

Implementation quirks of this PR

There's a couple of quirks in the implementation:

  • Symfony\Component\DependencyInjection\CompilerBuilder#createService is now public because of the limitations of PHP 5.3
  • Symfony\Component\DependencyInjection\Dumper\PhpDumper now with extra mess!
  • The proxies are dumped at the end of compiled containers, therefore the container class is not PSR compliant anymore

Todos

  • Fix license compatibility
  • Fix: real instantiation of the service swaps the registered instance in the DIC
  • Generate runtime proxies via eval
  • Generate proxies in the compiled DIC
  • 5.3 compatibility
  • Check all links to ProxyManager to ensure the dependency is completely optional
  • Handle disabled eval case
  • Add class resources to handle changes in definitions
@Ocramius Ocramius referenced this pull request in zendframework/zendframework Mar 30, 2013
Merged

Lazy services #4146

@Crell

@jrobeson pointed me here.

The functionality definitely sounds valuable. I know Lukas has been talking about this for a long time. However, the dependency chain here is not fun.

I cannot speak for Fabien, but I'd be wary of adding a dependency on 2 additional libraries to DependencyInjection just for this functionality. Even that aside, the cg-library package is licensed under Apache 2. That's a problem. Specifically, it means that the DI Component cannot run without Apache 2 code, which in turn means that it has the same impact license-wise as being Apache 2. That is, it becomes incompatible with GPLv2 and GPLv2+ code.

While Symfony itself is not GPL licensed, many of the systems that are now depending on it are. In particular, Drupal and phpBB are both GPLv2+ licensed. GPLv2 is incompatible with Apache 2. That would make the DI component, and by extension Symfony fullstack, unusable for both Drupal and phpBB, as well as likely others. (I don't know the license of any of the other Symfony-based systems off hand.) It also makes it unsuable for bespoke systems by shops that use GPLv2 (and there are a lot of them) that want to build off of Symfony2.

So, yeah. Introducing an Apache2 dependency would be a massive license-breaking and ecosystem-crippling change. :-/ jrobeson indicated that there was an alternative library, or that the cg-library could be relicensed if we convince the maintainer to do so. Either way the licensing issue would go away. The question of introducing additional dependencies I leave to fabpot.

Disclaimer: I'm the former Director of Legal Affairs for the Drupal Association, so I've spent a fair bit of time looking into this subject. I'm not just making it up. :-)

@Ocramius

@Crell I've been told that today, and didn't reallly expect it (I have no idea whatsoever this involves, I write code :( ). If BSD-3-Clause is compliant, I can convert my library to use Zend\Code instead. That would take me a couple of days, but is no problem at all.

@Ocramius Ocramius referenced this pull request in Ocramius/ProxyManager Mar 30, 2013
Merged

License compatibility #18

@Ocramius

@Crell I removed usage of cg-lib in ProxyManager as of Ocramius/ProxyManager#18 - should be ok now

@lyrixx
Symfony member

Do you have any benchmark on a "classic" Symfony2 application ? (without sleep ;) ).

@Ocramius

@lyrixx no, I actually don't use Symfony SE. You can actually benchmark it yourself by insulating a portion of your object graph that is not used and marking the root node of that portion as lazy. From what I know, the security component should be quite large.

Another thing this tries to remove is excessive usage of service location for performance optimizations, as I've explained here.

You can also read a detailed description of how lazy loading proxies (as implemented here) work here

OOTB, as it is now, this PR does not introduce any overhead.

@stof stof commented on an outdated diff Mar 30, 2013
...ny/Component/DependencyInjection/ContainerBuilder.php
@@ -861,8 +864,24 @@ public function findDefinition($id)
* @throws RuntimeException When the service is a synthetic service
* @throws InvalidArgumentException When configure callable is not callable
*/
- private function createService(Definition $definition, $id)
+ public function createService(Definition $definition, $id, $tryProxy = true)
@stof
stof Mar 30, 2013

you need to update the phpdoc

@stof
stof Mar 30, 2013

And you should add @internal in the phpdoc to explain that it is only public because of the internal use in a closure, and should not be called by other code

@stof stof commented on an outdated diff Mar 30, 2013
...ny/Component/DependencyInjection/ContainerBuilder.php
+ if ($tryProxy && ($className = $definition->getClass()) && $definition->isLazy()) {
+ $factory = new LazyLoadingValueHolderFactory(new Configuration());
+ $container = $this;
+
+ return $factory->createProxy(
+ $className,
+ function (& $wrappedInstance, LazyLoadingInterface $proxy) use ($container, $definition, $id) {
+ $proxy->setProxyInitializer(null);
+
+ $wrappedInstance = $container->createService($definition, $id, false);
+
+ return true;
+ }
+ );
+ }
+
if ($definition->isSynthetic()) {
@stof
stof Mar 30, 2013

This should be done before creating the lazy proxy IMO.

@stof stof commented on an outdated diff Mar 30, 2013
...ny/Component/DependencyInjection/Dumper/PhpDumper.php
+ * @return string
+ */
+ private function addProxyLoading($id, Definition $definition)
+ {
+ if (!($definition->isLazy() && $definition->getClass())) {
+ return '';
+ }
+
+ $class = $this->dumpValue($definition->getClass());
+
+ if (0 === strpos($class, "'") && !preg_match('/^\'[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*(\\\{2}[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*)*\'$/', $class)) {
+ return '';
+ }
+
+ // @todo this should happen directly through the factory class, but we have to ensure that the proxy
+ // @todo class is generated during the dump process
@stof
stof Mar 30, 2013

you should not put @todo on the second line. It makes it quite hard to read (I first thought it was a second TODO)

@stof stof and 1 other commented on an outdated diff Mar 30, 2013
...ny/Component/DependencyInjection/Dumper/PhpDumper.php
+ * Generates code for the proxy classes to be attached after the container class
+ *
+ * @return string
+ */
+ private function addProxyClasses()
+ {
+ $definitions = $this->container->getDefinitions();
+
+ ksort($definitions);
+
+ $proxyDefinitions = array_filter(
+ $this->container->getDefinitions(),
+ function (Definition $definition) {
+ return $definition->isLazy() && $definition->getClass();
+ }
+ );
@stof
stof Mar 30, 2013

Shouldn't you to this before sorting ?

@Ocramius
Ocramius Mar 31, 2013

actually removed the ksort. It is quite useless in this location.

@stof stof commented on an outdated diff Mar 30, 2013
src/Symfony/Component/DependencyInjection/composer.json
@@ -20,7 +20,8 @@
},
"require-dev": {
"symfony/yaml": "~2.0",
- "symfony/config": ">=2.2,<2.4-dev"
+ "symfony/config": ">=2.2,<2.4-dev",
+ "ocramius/proxy-manager": "0.3.*"
@stof
stof Mar 30, 2013

It should also be suggested (and using a message explaining why would be better than a version number IMO)

@stof
Symfony member

Your current implementation breaks on 1 point: recreating the proxy class when the original class changes. The class has to be added as a container resource for lazy services so that the container is dumped again when the class changes, so that the proxies are dumped again. Otherwise, the proxy might not match the class anymore.

What you would need is the logic of ContainerBuilder::addObjectResource but with a class name rather than an instance (which is easy as addObjectResource could then be rewritten as $this->addClassResource(get_class($object))

@stof
Symfony member

For the ContainerBuilder case, using eval may be better than creating a temporary file on the fly and requiring it. You don't gain anything about caching the generated code (as you regenerate it each time) except potential race conditions (see Doctrine considering to change the generation of proxies when using auto_generate_proxies).
Btw, Twig is already using eval when you disable the template cache.

@stof
Symfony member

Your code has a big issue: it creates a new proxy instance each time the service is retrieved from the container instead of reusing the same instance for container-scoped services. So $container->get('router') !== $container->get('router') when marking it as lazy.

@stof
Symfony member

thus, getting the lazy service, then doing a method call (thus initializing the proxy) and then getting it again would give you the wrapped instance after that, being inconsistent.

@Ocramius

@stof very good catch! Couldn't write a for that test because of the conflicting container names. I'll see if I can change the name of a generated container name and write something that covers that.

@mvrhov

Also note, that the usage of eval may make the symfony2 projects that use lazy loading unusable on some shared hosts.

@Ocramius

@mvrhov ouch! Do they turn it off? I can build a switch for that eventually if there's a way of recognizing it.

@mvrhov

yeah. I worked for one a few years ago and we were disabling it. The problem was that that a large amount of code injections that were happening then abused eval to send a spam or worse. Trying to get unlisted from the sites that list spam sending IPs every few weeks also become troublesome.
There is disable_functions ini setting where all disabled functions a re listed.

@Ocramius

@mvrhov I spawned Ocramius/ProxyManager#24 from that. It should not be handled in symfony itself.

@Ocramius

@stof can you clarify when I should add a resource? I added addClassResource to the API, but now I'm wondering about what should trigger it. Should I do it in ContainerBuilder#register? Are there other locations where definitions are injected?

@jalliot

The feature by itself would be a great addition but I agree with @Crell that the added dependencies are not so good.
Being able to reuse the CG library (if license compliant of course) would be a little bit better as it is already used by some Symfony2 bundles.

Besides, the generated class names of proxies should follow the naming convention used by both the CG library and Doctrine (with a __CG__ prefix and all, see ClassUtils) as it allows easily identifying proxies, wherever they come from, without having to test for all possible proxies interfaces out there.

@Ocramius

To clarify, I modified the PR so that the dependencies are now optional.

@jalliot for CG Lib, I kicked that out and replaced it with Zend\Code, which is BSD-3-clause and therefore compatible. That's quite final, since I don't want to get back and rewrite this backwards without any real benefit.

That's anyway stuff that doesn't affect this PR's code directly. SF2 just benefits from the abstraction layer of ProxyManager and should not really care about how that is achieved except for BC breaks, performance and security issues (and license compatibility, as introduced by @Crell).

For __CG__ I explicitly wanted to use __PM__ (or something different) because ProxyManager allows to hook in its own autoloader with its own rules depending on the naming strategy set in the proxy manager configuration. If that autoloader conflicts with the one in doctrine common, we have a problem. We don't want that, do we? :)

If you want to identify a ProxyManager proxy, you can use the utility within ProxyManager itself. An idea would be to drop the __PM__ constant from there so that you're forced to use the inflector instance and don't rely on a convention anymore.

@stof stof and 1 other commented on an outdated diff Apr 2, 2013
...ny/Component/DependencyInjection/ContainerBuilder.php
@@ -11,6 +11,11 @@
namespace Symfony\Component\DependencyInjection;
+use ProxyManager\Configuration;
+use ProxyManager\Factory\LazyLoadingValueHolderFactory;
+use ProxyManager\GeneratorStrategy\EvaluatingGeneratorStrategy;
+use ProxyManager\Proxy\LazyLoadingInterface;
+use ReflectionClass;
@stof
stof Apr 2, 2013

Symfony does not add use statements for classes in the global namespace

@Ocramius
Ocramius Apr 2, 2013

Will revert :)

@stof stof commented on the diff Apr 2, 2013
...ny/Component/DependencyInjection/ContainerBuilder.php
+ && ($className = $definition->getClass())
+ && $definition->isLazy()
+ && class_exists('ProxyManager\\Factory\\LazyLoadingValueHolderFactory')
+ ) {
+ $config = new Configuration();
+
+ $config->setGeneratorStrategy(new EvaluatingGeneratorStrategy());
+
+ $factory = new LazyLoadingValueHolderFactory($config);
+ $container = $this;
+ $proxy = $factory->createProxy(
+ $className,
+ function (& $wrappedInstance, LazyLoadingInterface $proxy) use ($container, $definition, $id) {
+ $proxy->setProxyInitializer(null);
+
+ $wrappedInstance = $container->createService($definition, $id, false);
@stof
stof Apr 2, 2013

Would the call to createService replace the shared instance ?

@Ocramius
Ocramius Apr 2, 2013

No, this has also a test for it, see ContainerBuilderTest:276, where I explicitly check:

$this->assertSame($foo1, $builder->get('foo1'), 'The same proxy is retrieved after initialization'); 
@lsmith77

its an optional dependency but i agree that adding 2 more dependencies can be a concern. doctrine proxy and the cg lib are commonly used already in the Symfony2 ecosystem while proxy manager and zend code are less used.

i didnt check but ideally the code should maybe gracefully ignore the lazy setting in case the dependencies are missing so that no code would break if those optional deps are not installed.

@Ocramius

@lsmith77 perfect, I will add the code required to let any lazy markers being ignored in the case where dependencies are not loaded.

@stof stof commented on the diff Apr 3, 2013
...ny/Component/DependencyInjection/ContainerBuilder.php
+ $this->addClassResource(new \ReflectionClass($object));
+ }
+
+ return $this;
+ }
+
+ /**
+ * Adds the given class hierarchy as resources.
+ *
+ * @param \ReflectionClass $class
+ *
+ * @return ContainerBuilder The current instance
+ *
+ * @api
+ */
+ public function addClassResource(\ReflectionClass $class)
@stof
stof Apr 3, 2013

why not passing a class name and building the ReflectionClass internally ? addObjectResource expects an object, not a ReflectionObject

@Ocramius
Ocramius Apr 3, 2013

Strict typing. This disallows unexisting classes upfront

@Ocramius

Still need to fix the 5.3 failures

@lsmith77

would love to see support for proxied services in 2.3

@Ocramius

@lsmith77 I'll fix the remaining points later tonight

@fabpot fabpot commented on an outdated diff Apr 25, 2013
...ny/Component/DependencyInjection/ContainerBuilder.php
@@ -413,13 +434,14 @@ public function has($id)
* Gets a service.
*
* @param string $id The service identifier
- * @param integer $invalidBehavior The behavior when the service does not exist
+ * @param int $invalidBehavior The behavior when the service does not exist
@fabpot
fabpot Apr 25, 2013

This change should be reverted.

@fabpot fabpot commented on an outdated diff Apr 25, 2013
...ny/Component/DependencyInjection/ContainerBuilder.php
*
+ * @throws InvalidArgumentException
+ * @throws InactiveScopeException
+ * @throws LogicException
+ * @throws \Exception
@fabpot
fabpot Apr 25, 2013

you should keep the @throws lines after the @return one like it was before. We also try to document when each exception is thrown.

@fabpot
Symfony member

@Ocramius If we want to include this into 2.3, this should be ready before Tuesday next week. Do you think it will be possible?

@Ocramius

@fabpot I was just working on this tonight. You can check it again tomorrow and it should be ok

@Ocramius

Not satisfied with Ocramius/symfony@d87086c, but I didn't find a better alternative since I couldn't get a 5.3.3 box to reproduce the phpunit issue

@Ocramius

@fabpot if it wasn't clear, this is good to go. Tests fail for an un-related feature

@jameshalsall

@Ocramius I was going to say that symfony isn't recommended to run on <= 5.3.8 anyway (according to http://symfony.com/doc/current/book/installation.html#installing-a-symfony2-distribution), but the site then contradicts itself here http://symfony.com/doc/current/reference/requirements.html#required saying that 5.3.3 is required. Maybe @fabpot can shed some light on that

@Ocramius

@jaitsu87 it's not related with this PR anyway

@beberlei beberlei commented on the diff Apr 30, 2013
...ny/Component/DependencyInjection/ContainerBuilder.php
{
if ($definition->isSynthetic()) {
throw new RuntimeException(sprintf('You have requested a synthetic service ("%s"). The DIC does not know how to construct this service.', $id));
}
+ if (
@beberlei
beberlei Apr 30, 2013

Can we move this whole block of code into a new service? This adds a hidden dependency to ZendFramework/ThirdParty code inside the ContainerBuilder that is not replaceable. This violates both Single Responsibilty and Dependency Inversion Principle here, not keeping the code against abstract concepts.

I say introduce a LazyLoad something interface, optionally inject it into the ContainerBuilder and then use the ProxyManager as one possible implementation there.

@beberlei beberlei commented on the diff Apr 30, 2013
...ny/Component/DependencyInjection/Dumper/PhpDumper.php
@@ -11,6 +11,9 @@
namespace Symfony\Component\DependencyInjection\Dumper;
+use ProxyManager\Generator\ClassGenerator;
@beberlei
beberlei Apr 30, 2013

Again here, can we abstract this against an interface to not violate the DIP? This dependency is optional, but the code makes it "required".

@beberlei

To clarify my comments above, the proxy manager is an optional dependency and should therefore be hidden in an adapter, maybe even in its own Bridge, implementing a generic interface from DependencyInjection component

@Ocramius

Agreed - fixing later today

Ocramius added some commits Mar 29, 2013
@Ocramius Ocramius First implementation of lazy services via proxy manager f503ad8
@Ocramius Ocramius Adding basic logic to generate proxy instantiation into a php dumped …
…container
67aef45
@Ocramius Ocramius Compiling proxies into the generated DIC file f4a19c7
@Ocramius Ocramius Upgrading dependency to ProxyManager 0.3.* d2760f1
@Ocramius Ocramius Suggesting ProxyManager in composer.json, removing useless calls 4a13f82
@Ocramius Ocramius Adding failing test to demonstrate that proxy initialization breaks s…
…hared services
a6a6572
@Ocramius Ocramius Fixing shared service instance 35fdded
@Ocramius Ocramius Sharing services in the container should only happen when proxying fa…
…iled
bec7774
@Ocramius Ocramius Adding tests for proxy sharing within dumped containers 468e92e
@Ocramius Ocramius Fixing shared proxies into the container 4ecd5ad
@Ocramius Ocramius Bumping required version of ProxyManager 11a1da9
@Ocramius Ocramius Docblock for ContainerBuilder#shareService 5870fed
@Ocramius Ocramius Adding `ContainerBuilder#addClassResource` 695e3c5
@Ocramius Ocramius Adding test to check that class resources are registered for lazy ser…
…vices
c5a5af0
@Ocramius Ocramius Fixing tests, registering class resources for lazy services 29899ec
@Ocramius Ocramius Reverting import of global namespace classes b5d0298
@Ocramius Ocramius Lazier checks on the proxy structure (avoiding whitespace-based test …
…failures
cda390b
@Ocramius Ocramius Getters for proxied services are public for 5.3.3 compatibility 1eb4cf7
@Ocramius Ocramius Enforcing soft dependency to ProxyManager b417969
@Ocramius Ocramius Reverting documentation changes, adding exception types description in ` 1e24767
@Ocramius Ocramius Lazier checks on the proxy structure 450635a
@Ocramius Ocramius referenced this pull request Apr 30, 2013
Closed

ProxyManager Bridge #7890

@Ocramius

I've opened #7890 with the suggestions of @beberlei

@fabpot
Symfony member

closing in favor of #7890

@fabpot fabpot closed this May 1, 2013
@fabpot fabpot added a commit that referenced this pull request May 6, 2013
@fabpot fabpot merged branch Ocramius/feature/proxy-manager-bridge (PR #7890)
This PR was squashed before being merged into the master branch (closes #7890).

Discussion
----------

ProxyManager Bridge

As of @beberlei's suggestion, I re-implemented #7527 as a new bridge to avoid possible hidden dependencies.

Everything is like #7527 except that the new namespace (and possibly package/subtree split) `Symfony\Bridge\ProxyManager` is introduced

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #6140 (supersedes) #5012 #6102 (maybe) #7527 (supersedes)
| License       | MIT (attached code) - BSD-3-Clause (transitive dependency)
| Doc PR        | Please pester me to death so I do it

This PR introduces lazy services along the lines of zendframework/zendframework#4146

It introduces an **OPTIONAL** dependency to [ProxyManager](https://github.com/Ocramius/ProxyManager) and transitively to [`"zendframework/zend-code": "2.*"`](https://github.com/zendframework/zf2/tree/master/library/Zend/Code).

## Lazy services: why? A comprehensive example

For those who don't know what this is about, here's an example.

Assuming you have a service class like following:

```php
class MySuperSlowClass
{
    public function __construct()
    {
        // inject large object graph or do heavy computation
        sleep(10);
    }

    public function doFoo()
    {
        echo 'Foo!';
    }
}
```

The DIC will hang for 10 seconds when calling:

```php
$container->get('my_super_slow_class');
```

With this PR, this can be avoided, and the following call will return a proxy immediately.

```php
$container->getDefinitions('my_super_slow_class')->setLazy(true);
$service = $container->get('my_super_slow_class');
```

The 10 seconds wait time will be delayed until the object is actually used:

```php
$service->doFoo(); // wait 10 seconds, then 'Foo!'
```

A more extensive description of the functionality can be found [here](https://github.com/Ocramius/ProxyManager/blob/master/docs/lazy-loading-value-holder.md).

## When do we need it?

Lazy services can be used to optimize the dependency graph in cases like:

 * Webservice endpoints
 * Db connections
 * Objects that cause I/O in general
 * Large dependency graphs that are not always used

This could also help in reducing excessive service location usage as I've explained [here](http://ocramius.github.com/blog/zf2-and-symfony-service-proxies-with-doctrine-proxies/).

## Implementation quirks of this PR

There's a couple of quirks in the implementation:

 * `Symfony\Component\DependencyInjection\CompilerBuilder#createService` is now public because of the limitations of PHP 5.3
 * `Symfony\Component\DependencyInjection\Dumper\PhpDumper` now with extra mess!
 * The proxies are dumped at the end of compiled containers, therefore the container class is not PSR compliant anymore

Commits
-------

78e3710 ProxyManager Bridge
dfd605f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment