Skip to content

[DependencyInjection] Skip errored definitions deep-referenced as runtime exceptions #50763

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

Merged
merged 1 commit into from
Jun 24, 2023

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Jun 23, 2023

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #50708
License MIT
Doc PR -

This solves the linked issue by not throwing a compile-time error when the service graph looks like this (when ServiceC has errors):

ServiceA ---(runtime-exception-on-invalid-ref)---> ServiceB ---(exception-on-invalid-ref)---> ServiceC

@nicolas-grekas nicolas-grekas merged commit 201d9fb into symfony:5.4 Jun 24, 2023
@nicolas-grekas nicolas-grekas deleted the di-errored-defs branch June 24, 2023 12:49
This was referenced Jun 26, 2023
@NicolJamie
Copy link

Understand this has been merged/closed. Not seen any reports of this as of yet, however, I am getting this repeated OutOfMemoryError message.

Symfony\Component\ErrorHandler\Error\OutOfMemoryError:
Error: Allowed memory size of 134217728 bytes exhausted (tried to allocate 81920 bytes)

  at /application/vendor/symfony/dependency-injection/Compiler/DefinitionErrorExceptionPass.php:69 

@X-Coder264
Copy link
Contributor

X-Coder264 commented Jun 30, 2023

Same here. The other day I had an errored service definition and I also got the OutOfMemoryError. Calling bin/console ca:cl with memory_limit=-1 fixed the problem, though it took a very long time for it to finish. After setting a breakpoint in this compiler pass and fixing that service definition bin/console ca:cl went back to only taking a few seconds to finish again. When I replaced the code for this compiler pass with the one prior to this change and then tried running bin/console ca:cl with the errored service definition it also went back to taking just a few seconds. So this PR is definitively the culprit, it looks like some loop there is not exactly infinite but takes a very long time to finish.

@nicolas-grekas
Copy link
Member Author

Please open a dedicated issue and provide a reproducer (even a private one you'd send me on Slack). Commenting on closed PRs doesn't work since we can't track those.

@nicolas-grekas
Copy link
Member Author

Please also let me know if the following patch has any effect on your side, both in terms of memory and time:

--- a/DefinitionErrorExceptionPass.php
+++ b/DefinitionErrorExceptionPass.php
@@ -71,7 +71,7 @@ class DefinitionErrorExceptionPass extends AbstractRecursivePass
                     }
                 }
 
-                unset($this->sourceReferences[$id]);
+                unset($this->sourceReferences[$id], $this->targetReferences[$id]);
             }

nicolas-grekas added a commit that referenced this pull request Jul 7, 2023
… definitions (nicolas-grekas)

This PR was merged into the 5.4 branch.

Discussion
----------

[DepdencyInjection] Fix costly logic when checking errored definitions

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #50763 (comment)
| License       | MIT
| Doc PR        | -

`@NicolJamie` `@X`-Coder264 can you please give this patch a try and report back. Does that fix the perf issue you're having?

Commits
-------

81adcd6 [DepdencyInjection] Fix costly logic when checking errored definitions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants