Skip to content

[DependencyInjection] PhpDumper does not share single-use dependencies when a lazy service is cloned #59706

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

Closed
pvandommelen opened this issue Feb 5, 2025 · 1 comment

Comments

@pvandommelen
Copy link
Contributor

Symfony version(s) affected

7.x

Description

PhpDumper inlines dependencies when they are private, used only once and non-lazy. When clone-ing a lazy service using one of these dependencies, this check is not sufficient and the dependency is constructed twice with the cloned services not sharing the dependency.

This is a regression compared to the old method through symfony/proxy-manager-bridge available in 6.x. Using the container directly without using PhpDumper also correctly shares the dependency.

The blog post introducing the new implementation (https://symfony.com/blog/revisiting-lazy-loading-proxies-in-php) explicitly calls out support for clone and wither methods, so I believe the current behaviour where the dependency is not shared is a bug.

How to reproduce

The following script showcases the issue. The last two asserts fail. The asserts pass when the compiled container is used directly without using PhpDumper or when using 6.x with symfony/proxy-manager-bridge.

<?php
require_once "vendor/autoload.php";

use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Dumper\PhpDumper;
use Symfony\Component\DependencyInjection\Reference;

class Dependency {
	public static int $construction_count = 0;

	public function __construct() {
		++self::$construction_count;
	}
}

class Service {
	public function __construct(
		public readonly mixed $argument,
	) {}
}

function assert_construction_count(int $expected): void {
	assert(
		Dependency::$construction_count === $expected,
		sprintf('Construction count. Expected=%d, Actual=%d', $expected, Dependency::$construction_count)
	);
}

$container = new ContainerBuilder();
$container->register('dependency', Dependency::class);
$container->register('service', Service::class)
	->setArgument('$argument', new Reference('dependency'))
	->setLazy(true)
	->setPublic(true);

$container->compile();

// Dump and reconstruct the container.
$dumper = new PhpDumper($container);
file_put_contents('./container.php', $dumper->dump([
	'class' => 'DumpedContainer',
]));
include './container.php';
$container = new \DumpedContainer();

assert_construction_count(0);

// The service is lazy and will not yet be constructed.
$service = $container->get('service');
assert_construction_count(0);

// Cloning has no impact.
$first_cloned_service = clone $service;
$second_cloned_service = clone $service;
assert_construction_count(0);

// Construction of the first clone creates the clone and the internal argument.
$first_arg = $first_cloned_service->argument;
assert_construction_count(1);

// Construction of the second clone should only create the clone and reuse the dependency. Currently this fails and the dependency is constructed twice.
$second_arg = $second_cloned_service->argument;
assert_construction_count(1);
assert($first_arg === $second_arg);

Possible Solution

Ideally I would expect the service to be constructed when the object is cloned. I don't think PHP offers any method to hook into this process before the clone occurs (__clone is only called afterwards) and afterwards is too late.

An alternative approach is to make the inlining behaviour more conservative. Currently the assignment to singleUsePrivateIds through isSingleUsePrivateNode() in the PhpDumper controls the inlining of service factories. This already checks a lazy flag which considers the target service.

I'm unsure what the best solution would be. The check could be expanded to also check incoming edges. Another approach would be to mark the edge as lazy if the source definition is lazy in AnalyseServiceReferencesPass instead of only checking the target (possible through reordering when $this->lazy is reset).

Additional Context

No response

@nicolas-grekas
Copy link
Member

Up for a PR that'd add a failing test case as a start?

pvandommelen added a commit to pvandommelen/symfony that referenced this issue Feb 7, 2025
…encies by making the inlining behaviour more conservative

Fixes symfony#59706
pvandommelen added a commit to pvandommelen/symfony that referenced this issue Feb 7, 2025
…encies by making the inlining behaviour more conservative

Fixes symfony#59706
pvandommelen added a commit to pvandommelen/symfony that referenced this issue Feb 7, 2025
…encies by making the inlining behaviour more conservative

Fixes symfony#59706
pvandommelen added a commit to pvandommelen/symfony that referenced this issue Feb 7, 2025
…encies by making the inlining behaviour more conservative

Fixes symfony#59706
pvandommelen added a commit to pvandommelen/symfony that referenced this issue Feb 7, 2025
…encies by making the inlining behaviour more conservative

Fixes symfony#59706
nicolas-grekas added a commit that referenced this issue Feb 10, 2025
… their dependencies when dumped with PhpDumper (pvandommelen)

This PR was squashed before being merged into the 6.4 branch.

Discussion
----------

[DependencyInjection] Fix cloned lazy services not sharing their dependencies when dumped with PhpDumper

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #59706
| License       | MIT

Fixes referenced services not being shared when lazy services are cloned before being initialized. Adds tests for both the ghost and proxy scenario's.

This makes the inlining behaviour more conservative, which impacts the outputs of some other test cases.

First commit adds a test, second commit has the fix. Third commit also considers the proxy case, which is also affected. I can squash if necessary.

Targeted 6.4, but this also applies to 7.x.

Commits
-------

a60cff5 [DependencyInjection] Fix cloned lazy services not sharing their dependencies when dumped with PhpDumper
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants