Constructor should not instantiate entity storage

Created on 8 April 2024, 9 months ago
Updated 17 May 2024, 7 months ago

Problem/Motivation

I don't have airtight proof this is the actual problem, unfortunately, but I think I have traced a problem we have to this module; we are running into problems with our memcache access, and this module is always involved (the problems started when we released the module to production). The code passes through the VotingApiReactionManager constructor and doing something I think is a best practice *not* to do, which is instantiating entity storage right in the constructor. See e.g. #3162827: Do not instantiate entity storages in constructors of services that do not always need them and
https://mglaman.dev/blog/dependency-injection-anti-patterns-drupal (heading "Assigning entity storage to a property instead of the entity manager"). These do not mention any hard problems like this, but still are a motivation to do this differently.

Below is the stack trace we are seeing, with identifying information removed:

Notice: MemcachePool::get(): Server [memcache server address] (tcp 11211, udp 0) failed with: Network timeout (0) in Drupal\memcache\Driver\MemcacheDriver->getMulti() (regel 60 van [project base]/web/modules/contrib/memcache/src/Driver/MemcacheDriver.php)
#0 [project base]/web/core/includes/bootstrap.inc(164): _drupal_error_handler_real()
#1 [internal function]: _drupal_error_handler()
#2 [project base]/web/modules/contrib/memcache/src/Driver/MemcacheDriver.php(60): MemcachePool->get()
#3 [project base]/web/modules/contrib/memcache/src/MemcacheBackend.php(144): Drupal\memcache\Driver\MemcacheDriver->getMulti()
#4 [project base]/web/modules/contrib/memcache/src/MemcacheBackend.php(136): Drupal\memcache\MemcacheBackend->getMultiple()
#5 [project base]/web/core/lib/Drupal/Core/Entity/EntityLastInstalledSchemaRepository.php(117): Drupal\memcache\MemcacheBackend->get()
#6 [project base]/web/core/lib/Drupal/Core/Entity/EntityFieldManager.php(488): Drupal\Core\Entity\EntityLastInstalledSchemaRepository->getLastInstalledFieldStorageDefinitions()
#7 [project base]/web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php(187): Drupal\Core\Entity\EntityFieldManager->getActiveFieldStorageDefinitions()
#8 [project base]/web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php(157): Drupal\Core\Entity\Sql\SqlContentEntityStorage->__construct()
#9 [project base]/web/core/lib/Drupal/Core/Entity/EntityTypeManager.php(273): Drupal\Core\Entity\Sql\SqlContentEntityStorage::createInstance()
#10 [project base]/web/core/lib/Drupal/Core/Entity/EntityTypeManager.php(262): Drupal\Core\Entity\EntityTypeManager->createHandlerInstance()
#11 [project base]/web/core/lib/Drupal/Core/Entity/EntityTypeManager.php(192): Drupal\Core\Entity\EntityTypeManager->getHandler()
#12 [project base]/web/modules/contrib/votingapi_reaction/src/VotingApiReactionManager.php(113): Drupal\Core\Entity\EntityTypeManager->getStorage()
#13 [project base]/web/core/lib/Drupal/Component/DependencyInjection/Container.php(259): Drupal\votingapi_reaction\VotingApiReactionManager->__construct()
#14 [project base]/web/core/lib/Drupal/Component/DependencyInjection/Container.php(177): Drupal\Component\DependencyInjection\Container->createService()
#15 [project base]/web/core/lib/Drupal/Core/DependencyInjection/DependencySerializationTrait.php(89): Drupal\Component\DependencyInjection\Container->get()
#16 [internal function]: Drupal\Core\Form\FormBase->__wakeup()
#17 [project base]/web/modules/contrib/memcache/src/Driver/MemcacheDriver.php(60): MemcachePool->get()
[...]

My current hypothesis is that we are running into trouble because we are still in the middle of a cache lookup when another cache lookup is started. This is upon unserializing a votinapi_reaction form, the Manager is also instantiated, which in turn needs to access to cache to get entity definitions.

Steps to reproduce

Unclear at this point.

Proposed resolution

Only inject the entity type manager, and retrieve specific entity storage handlers when they are actually needed.

Remaining tasks

  1. Get steps to reproduce (optional)
  2. Not sure if you'd agree, but I added some links to the problem/motivation
  3. Create a merge request
  4. Review
  5. Merge

User interface changes

None.

API changes

None, as the injected services are not considered part of the API.

Data model changes

None.

🐛 Bug report
Status

Fixed

Version

1.0

Component

Code

Created by

🇳🇱Netherlands eelkeblok Netherlands 🇳🇱

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

Production build 0.71.5 2024