Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Config] Don't add object-value of static properties in the signature of container metadata-cache#32809

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
nicolas-grekas merged 1 commit intosymfony:3.4fromarjenm:3.4
Jul 30, 2019

Conversation

@arjenm
Copy link
Contributor

@arjenmarjenm commentedJul 30, 2019
edited
Loading

QA
Branch?3.4
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed ticketsNo ticket created, see below
LicenseMIT

Running bin/console cache:clear can take a very long time and/or uses explosive amounts of memory if large protected/public static objects are available in classes for which metadata will be generated during the 'container dump'.

I.e. in \Symfony\Component\HttpKernel\Kernel::dumpContainer eventually a call to \Symfony\Component\Config\ResourceCheckerConfigCache::write is done. That writes a serialized version of the $metadata.
In that serialize a call to \Symfony\Component\Config\Resource\ReflectionClassResource::generateSignature for each service will eventually do a print_r on the class's (default) properties.

Class properties can also be (public/protected) static. PHP Reflection doesn't differentiate between those in that context. Andthose static objects can also have non-scalar values, including objects.

If such an object is very large, for instance a service that references the Container (with many services, some also referencing the container, etc), the print_r will run into very deep recursions. I.e. it will either take "forever" or use so much memory the OOM killer is activated (it got to 25GB...).

This scenario only triggers when a cache existed prior to running cache:clear, i.e. when 'isFresh' is called. When first removing the cache dir (or running cache:clear with --no-warmup) and than running cache:clear does not trigger the behavior, apparently the hash is created in a saver time in that scenario.

This sample project offers code that will prime a static property into a service (the trick is having a command with such a class-reference/service):
https://github.com/arjenm/symfony-service-recursion
Add a var_dump to the$defaults = $class->getDefaultProperties(); in \Symfony\Component\Config\Resource\ReflectionClassResource::generateSignature to see it in effect.
The first cache:clear will show a few null's; the second run - without removing the cache - will show a container instead of null.
Its still a tiny container, so it'll not trigger serious issues.

This PR prevents an object "default value" for a property from being fed to 'print_r'. Since normally an object-property should be null on a "fresh" look at a non-instantiated class it should be save to use (i.e. no BC).

linaori and n3o77 reacted with hooray emoji
@nicolas-grekasnicolas-grekas added this to the3.4 milestoneJul 30, 2019
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment
edited
Loading

Choose a reason for hiding this comment

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

TIL thatReflectionClass:getDefaultProperties() doesn't return the class' default value for static properties...
The patch in this PR doesn't account for objects nested in arrays, but detecting them would be costly I believe, so let's go with this simple check.

linaori reacted with thumbs up emoji
@nicolas-grekasnicolas-grekas changed the titleDon't add object-value of static properties in the signature of container metadata-cache[Config] Don't add object-value of static properties in the signature of container metadata-cacheJul 30, 2019
@nicolas-grekas
Copy link
Member

Thank you@arjenm.

@nicolas-grekasnicolas-grekas merged commita80e56c intosymfony:3.4Jul 30, 2019
nicolas-grekas added a commit that referenced this pull requestJul 30, 2019
…re of container metadata-cache (arjenm)This PR was merged into the 3.4 branch.Discussion----------Don't add object-value of static properties in the signature of container metadata-cache| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | No ticket created, see below| License       | MITRunning bin/console cache:clear can take a very long time and/or uses explosive amounts of memory if large protected/public static objects are available in classes for which metadata will be generated during the 'container dump'.I.e. in \Symfony\Component\HttpKernel\Kernel::dumpContainer eventually a call to \Symfony\Component\Config\ResourceCheckerConfigCache::write is done. That writes a serialized version of the $metadata.In that serialize a call to \Symfony\Component\Config\Resource\ReflectionClassResource::generateSignature for each service will eventually do a print_r on the class's (default) properties.Class properties can also be (public/protected) static. PHP Reflection doesn't differentiate between those in that context. And _those_ static objects can also have non-scalar values, including objects.If such an object is very large, for instance a service that references the Container (with many services, some also referencing the container, etc), the print_r will run into very deep recursions. I.e. it will either take "forever" or use so much memory the OOM killer is activated (it got to 25GB...).This scenario only triggers when a cache existed prior to running cache:clear, i.e. when 'isFresh' is called. When first removing the cache dir (or running cache:clear with --no-warmup) and than running cache:clear does not trigger the behavior, apparently the hash is created in a saver time in that scenario.This sample project offers code that will prime a static property into a service (the trick is having a command with such a class-reference/service):https://github.com/arjenm/symfony-service-recursionAdd a var_dump to the `$defaults = $class->getDefaultProperties();` in \Symfony\Component\Config\Resource\ReflectionClassResource::generateSignature to see it in effect.The first cache:clear will show a few null's; the second run - without removing the cache - will show a container instead of null.Its still a tiny container, so it'll not trigger serious issues.This PR prevents an object "default value" for a property from being fed to 'print_r'. Since normally an object-property should be null on a "fresh" look at a non-instantiated class it should be save to use (i.e. no BC).Commits-------a80e56c Don't add value of (default/static) objects to the signature
This was referencedAug 26, 2019
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@xabbuhxabbuhxabbuh approved these changes

+1 more reviewer

@TobionTobionTobion approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

3.4

Development

Successfully merging this pull request may close these issues.

5 participants

@arjenm@nicolas-grekas@Tobion@xabbuh@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp