Problem/Motivation

I've recently encountered a place where the entity storage has been kept as property on the entity class. If the entity type doesn't support static cache then everything is fine, but if it doesn't then this might lead to a memory leak on each entity load, as then the the entity is written to the persistent cache and when a lot of entities are being loaded the serialized storage will get bigger and bigger and so each serialized entity.

Proposed resolution

We could prevent this automatically and there are the two options:
In EntityStorageBase::__sleep() don't return the property for the static entity cache.
In DependencySerializationTrait::__sleep() and ::__wakeup() take care of entity storages kind of like taking care of service objects.

The first option feels like the more natural to do, as the static cache is part of the entity storage, but on the other side there is no point to serialize the storage object as this takes more memory and actually having different storage instances for the same entity type around might break things as well, therefore not serializing it should be the better option as this could prevent other bugs beside memory leaks.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#9 interdiff.txt1018 bytesslasher13
#9 2939491-9.patch4.55 KBslasher13
#4 interdiff-2-4.txt1.35 KBhchonov
#4 2939491-4.patch4.55 KBhchonov
#2 2939491-2.patch3.2 KBhchonov
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hchonov created an issue. See original summary.

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov as a volunteer and at bio.logis Genetic Information Management GmbH commented
Status: Active » Needs review
FileSize
2939491-2.patch3.2 KB

Actually we could do the both options mentioned in the issue summary.

Status: Needs review » Needs work

The last submitted patch, 2: 2939491-2.patch, failed testing. View results

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov as a volunteer and at bio.logis Genetic Information Management GmbH commented
Status: Needs work » Needs review
FileSize
2939491-4.patch4.55 KB
interdiff-2-4.txt1.35 KB

Fixing the failing test for properly cloning content entities.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

Did you consider introducing an interface for classes to change the behaviour how they are serialized/unserialized when serializing/unserializing a different class?

+++ b/core/lib/Drupal/Core/DependencyInjection/DependencySerializationTrait.php
@@ -61,6 +80,19 @@ public function __wakeup() {
+    if ($this->_entityStorages && !($phpunit_bootstrap && !$container->has('entity_type.manager'))) {

This condition is weird: A && !(B && !C), can't we simplify this to A && (!B || c). Not sure this is more readable to be honest.

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov as a volunteer and at bio.logis Genetic Information Management GmbH commented

Did you consider introducing an interface for classes to change the behaviour how they are serialized/unserialized when serializing/unserializing a different class?

Could you please elaborate on this as I am not sure I fully understand what you mean?

This condition is weird: A && !(B && !C), can't we simplify this to A && (!B || c). Not sure this is more readable to be honest.

I've just reused the expression from the existing code above and thought it would be better if it is the same at both places.

slasher13’s picture

slasher13
Primary language German
CreditAttribution: slasher13 as a volunteer and at Wolters Kluwer commented
Status: Needs review » Reviewed & tested by the community

Had multiple "PHP Fatal error: Allowed memory size ..." issues. Didn't find the root cause, but applying this patch helps.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/lib/Drupal/Core/DependencyInjection/DependencySerializationTrait.php
@@ -61,6 +80,19 @@ public function __wakeup() {
+    if ($this->_entityStorages && !($phpunit_bootstrap && !$container->has('entity_type.manager'))) {

I agree with @dawehner that the if is really odd but double negatives are certainly confusing. I'd make this
if ($this->_entityStorages && (!$phpunit_bootstrap || $container->has('entity_type.manager'))) {
To avoid the double negative as it is simpler to reason about. I think the earlier use in the for loop is not the same expression because there is no double negative.

slasher13’s picture

slasher13
Primary language German
CreditAttribution: slasher13 as a volunteer and at Wolters Kluwer commented
Status: Needs work » Needs review
FileSize
2939491-9.patch4.55 KB
interdiff.txt1018 bytes

#8 done

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Reviewed & tested by the community

Agreed, this is easier to read.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!

  • catch committed 9d60012 on 8.6.x 
    Issue #2939491 by hchonov, slasher13, alexpott, dawehner: Prevent memory...

  • catch committed 751a6d4 on 8.5.x 
    Issue #2939491 by hchonov, slasher13, alexpott, dawehner: Prevent memory...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.