Problem/Motivation

From EntityStorageInterface:

  /**
   * Resets the internal, static entity cache.
   *
   * @param $ids
   *   (optional) If specified, the cache is reset for the entities with the
   *   given ids only.
   */
  public function resetCache(array $ids = NULL);

Implementation in ContentEntityStorageBase:

  /**
   * {@inheritdoc}
   */
  public function resetCache(array $ids = NULL) {
    if ($ids) {
      $cids = array();
      foreach ($ids as $id) {
        unset($this->entities[$id]);
        $cids[] = $this->buildCacheId($id);
      }
      if ($this->entityType->isPersistentlyCacheable()) {
        $this->cacheBackend->deleteMultiple($cids);
      }
    }
    else {
      $this->entities = array();
      if ($this->entityType->isPersistentlyCacheable()) {
        Cache::invalidateTags(array($this->entityTypeId . '_values'));
      }
    }
  }

This clears the persistent entity cache every time the static cache is reset. It means there is no way to clear the static cache without clearing the persistent caches for content entities. Other entity storage implementation work correctly, this should follow suit. The current implementation abuses the API.

This also means there are other things that should not happen, like the persistent cache being cleared when you call $storage->loadUnchanged($id)!

Proposed resolution

Remove it! See what breaks. Speaking to catch, resetting the persistent cache should be something that the storage cares about internally upon saving etc...

Remaining tasks

User interface changes

None

API changes

Content entity usage of the EntityStorageInterface will be fixed.

Data model changes

Comments

damiankloip created an issue. See original summary.

damiankloip’s picture

Status: Active » Needs review
FileSize
774 bytes

Firstly, see what breaks.

damiankloip’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 2: 2635440.patch, failed testing.

damiankloip’s picture

Issue summary: View changes
damiankloip’s picture

Status: Needs work » Needs review
FileSize
1.41 KB

So funny thing is that the current code relies on the fact that loadUnchanged() will call resetCache() which will clear the persistent cache ready for the new entity values :)

Let's see how this one gets on.

Status: Needs review » Needs work

The last submitted patch, 6: 2635440-6.patch, failed testing.

damiankloip’s picture

Title: Remove persistent cache clearing from ContentEntityStorageBase::ResetCache() » Remove persistent cache clearing from ContentEntityStorageBase::resetCache()
Status: Needs work » Needs review
FileSize
1.93 KB
816 bytes

And on delete..

damiankloip’s picture

Priority: Normal » Major

Status: Needs review » Needs work

The last submitted patch, 8: 2635440-8.patch, failed testing.

damiankloip’s picture

Looks like most of those fails now relate to comments.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
2.72 KB
1.62 KB

So likely the cache needs to be cleared after loadUnChanged but before the other hooks are invoked?

Status: Needs review » Needs work

The last submitted patch, 12: 2635440-12.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
2.19 KB
3.25 KB

Moved code into a clearPersistentCache method instead. Comments work fine normally, this is just a test issue, maybe with CommentTestBase::postComment()

damiankloip’s picture

+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
@@ -608,24 +616,41 @@ protected function setPersistentCache($entities) {
+   * Clears the persistent cache for an entity if it uses persistent caching.

s/an entity/entities

Status: Needs review » Needs work

The last submitted patch, 14: 2635440-14.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
2.32 KB
1.06 KB

ok, that's not a good idea then...

Status: Needs review » Needs work

The last submitted patch, 17: 2635440-17.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
2.19 KB
632 bytes

Duh, silly mistake. Based on the patch from #14 again.

Status: Needs review » Needs work

The last submitted patch, 19: 2635440-19.patch, failed testing.

Berdir’s picture

I know the current API is unfortunate, but it's how it always worked, also with entitycache in 7.x. This is IMHO an API change that we can not do in 8.x. All we can do is add a new method that only does the first.

There are valid cases where external code has to invalidate the persistent cache.

dawehner’s picture

What about adding a second parameter which is just added to the actual implementation instead of the interface? With that practically everything can use it, but it doesn't break APIs.

damiankloip’s picture

I don't agree. Content entities are breaking this API totally and not honouring what the method is meant for. Which is clear static cache only. Someone clearly added it in here for convenience and a semi-hack for tests passing (?).

If code wants to clear the persistent cache then shouldn't it use tags like everything else has to? Or we add a new method that does persistent cache clearing. Currently what we have is a method that is documented as only clearing the static cache and doing just that on config entities, but content entities it clears out everything. Leaving no way to clear the static cache during a request. That's quite a big oversight IMO.

damiankloip’s picture

@dawehner x-posted with you, weird after such a long time! Adding an additional parameter would work for me, ANYTHING that unbreaks this IMO.

Berdir’s picture

Well, as I said, this is older than cache tags and a direct port from entity_cache in 7.x. It always worked this there. We don't use entity cache tags for the entity storage, we had an issue once but it didn't make sense.

Changing this IMHO is an API break for existing code that calls this function. There *are* cases that expect that it clears the persistent cache, comment related stuff for example, it's not just for tests. Tests actually don't need that, they only need the static cache clear part.

damiankloip’s picture

ok, technically a break, yes :) This is just horrible though. The docs even say:

Resets the internal, static entity cache.

So they should say:

Resets the internal, static entity cache and/or the persistent cache.

So basically anything could happen when you call it :)

When I am iterating lots of entities, I have no choice but to clear the persistent cache when dealing with Content entities, which is terrible. I would ideally have a way to do that. As we can't rely on the API docs, we maybe need an alternative way to fix this?

catch’s picture

Changing this IMHO is an API break for existing code that calls this function. There *are* cases that expect that it clears the persistent cache, comment related stuff for example, it's not just for tests.

I think that's the kind of expectation it's OK to break in a minor release, given the documentation completely contradicts it and there are documented ways to do it properly.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

The last submitted patch, 19: 2635440-19.patch, failed testing.

Berdir’s picture

I think that's the kind of expectation it's OK to break in a minor release,
given the documentation completely contradicts it and there are documented
ways to do it properly.

What documented ways? This is the only way to invalidate the entity storage cache. We don't use cache tags here. On purpose, since it is a 1:1 relationship between tags and cache ids, not dependencies.

catch’s picture

Saving the entity resets the cache and should be the only time it needs to be reset.

Can also use the cache API directly since the cid is predictable if you're doing something unusual.

damiankloip’s picture

I have hit this problem again processing stuff in queues with drush queue-run. Just to use the entities I would have to clear their caches (static and persistent)...

Let's see how something like this works instead.

damiankloip’s picture

Status: Needs work » Needs review

Yes, E.g. the cids would be "values:user:1" etc..

Status: Needs review » Needs work

The last submitted patch, 32: 2635440-29-new-method.patch, failed testing.

Berdir’s picture

We have hook_entity_storage_load() and we tried to use it as much as possible (e.g. comment module). That allows you to attach stuff to entities when they're loaded and have it cached. Obviously, if that data changes, you have to invalidate the cache.

So the storage cache is *not* an internal, transparent thing. It's part of our API. And we need IMHO a way to invalidate it through the API.

damiankloip’s picture

ok, I understand that. but still, the documentation for resetCache() is clearly for static caching only... and it is very much needed.

So I am loading entities and processing stuff in a queue, which I am running with drush. There is no way for me to reset the static cache without wiping out the persistent cache. So I have to clear the persistent cache for entities that don't need it. Just so I can reduce the memory footprint. The persistent cache is something that should be additional to content entities, like it is now. Yes, my patch is internal etc.. but otherwise it is a total api break. Currently it is not really (IMO).

I stick by what I say, in it's current form, this is pretty much unusable for some cases, where you need to load a lot of entities. I have already hit problems trying to E.g. delete a lot of entities. You cannot load too many, and you can't use my iterator patch (related issue) because it will clear the persistent cache when ever it is used.

Do you have any suggestions then? :)

mikeryan’s picture

Related issues:

Migration is also a major use case for clearing the static cache: #2558857: Migrations invalidate entity caches when reclaiming memory.

andrewbelcher’s picture

+1 for a solution to this. My use case is a long running CLI process where the static cache essentially causes a memory leak, but invaliding tags and clearing the persistent cache is both resource intensive and undesirable.

My work around is to use my own storage class extending SqlContentEntityStorage adding a resetStaticCache() method which only deals with the static cache. Perhaps this is something we can consider adding to ContentEntityStorageBase as a means to make this possible? Happy to post a patch if people feel that would be a reasonable solution?

Fabianx’s picture

#38: I like resetStaticCache() a lot.

andrewbelcher’s picture

How would backward compatibility affect adding this to an interface?

Ideally it would go into EntityStorageInterface. I Otherwise I presume we'd need to add a new interface that implementing code can check for before using..?

I'm thinking it would be best added to EntityStorageInterface and can simply call self::resetCache, or even make resetCache call the new method so the interface documentation can be updated to be more accurate.

damiankloip’s picture

resetStaticCache will work fine, it just makes things confusing as resetCache is already just meant to do that :( IMO if we are adding a new method, it should be for clearing the persistent cache, not just the static cache but this way round is better for BC.

I don't think this is something that belongs on EntityStorageInterface as that is already documented (I think, correctly) with resetCache. This is only a problem with Content entities.

andrewbelcher’s picture

tl;dr: I'm very happy to work on this, but I think the decision of which base class and interface to add this to is beyond my level of understanding of the BC policy.

I'll get a patch up today which simply adds resetStaticCache() to ContentEntityStorageBase with no addition to/of interfaces - that can the be expanded/adjusted once the wider decisions are made...


It seems to me that there are a few choices we could make:
Methods

1) Deprecating resetCache() (for removal in 9) and having 2 new methods, one for static and one for everything/persistent (don't think you ever want to clear persistent without static, but maybe there is a use of that..?). Then we can update resetCache's documentation to match what it actually does. Everything in code is then switch to the appropriate new method and contrib can do the same.

2) Add a method for static cache and simply update the documentation for resetCache() and take the hit that people who were only expecting static caches to be reset will remain unaware unless they read the updated documentation.

3) Add a method for persistent cache and fix resetCache() with a change record and immediately update core to use the new method. This breaks BC, but only because it's fixing a mis-match between documentation and what it actually does.

What level to add it at

Once that's decided, we can decide if we do it for all entities or just content entities. I would lean towards all as non content entities may also have persistent even though core config doesn't.

How to deal with interfaces

1) We could add a new interface for just the new methods which the core base classes can implement and any calls to the methods need to check for this in case contrib has directly implemented the interface.

2) We could add the method to the interface breaking BC if we think that EntityStorageInterface is actually an internal and contrib should be extending the base classes (in which we will implement it) rather than the interfaces directly. Similar has been discussed elsewhere, but I don't know if it applies here.

3) Add an EntityStorageInterfaceFixed (with a better name) which extends EntityStorageInterface and becomes the new defacto interface with EntityStorageInterface kept as a backward compatibility layer to be deprecated/removed in 9.

4) Not add it to any interface and expect code to do a method exists/know that the entity they are dealing with implements it.

catch’s picture

Just a note I'm working on #1596472: Replace hard coded static cache of entities with cache backends which provides a workaround for this (you can clear the cache.static backend directly), but also doesn't conflict with this afaik.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

plach’s picture

Issue tags: +Performance
fago’s picture

Afais, resetCache() is used as the way to clear the cache of an entity API, i.e., whatever cache is there. So I'd have said it's the docs that need to be fixed. Alternatively, I could see us adding an $excludePermanentCache = FALSE parameter to it?

catch’s picture

Status: Needs work » Postponed

@fago increasingly I think we should do #1596472: Replace hard coded static cache of entities with cache backends and mark this as duplicate. For now I'm going to postpone it.

andrewbelcher’s picture

Status: Postponed » Active

@catch I think that #1596472: Replace hard coded static cache of entities with cache backends is probably the best solution to the original issue of not being able to clear the static, but at the moment there is an outstanding bug, whether it be documentation or API. I'm not sure this should be postponed, rather the plan of cache backends should influence how far we go in solving the current issue.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

Title: Remove persistent cache clearing from ContentEntityStorageBase::resetCache() » Document what cache clearing from ContentEntityStorageBase::resetCache() actually clears
Component: entity system » documentation
Category: Bug report » Task
Priority: Major » Normal

The change requested here we will handle as a duplicate of #1596472: Replace hard coded static cache of entities with cache backends. We should rescope this issue to simply correct documentation to explain what is cleared.