Cron for cache handling breaks badly

Simply disabled the cron in the MR so that we can use the patch during deployment to ensure our sites won't break.

The cron job fixes caching for anonymous users, when office hours are displayed in some cases.
Indeed, making assumptions.

Do you have an alternative approach for your use case? OR is the caching problem in your installation already working fine?
In the mean time, i will make sure the job stops properly when the mentioned table does not exist.

I am not fluent with MR's. Please try attached patch.

The cron job fixes caching for anonymous users, when office hours are displayed in some cases. Indeed, making assumptions.

Not sure, why that should be needed.

Do you have an alternative approach for your use case? OR is the caching problem in your installation already working fine?

We've never seen any issue with caching of office hours. Why should that be different from other entity fields?

In the mean time, i will make sure the job stops properly when the mentioned table does not exist.

Well, catching an exception should be the last resort. The service should not even use the database connector at all. Why don't you use the cache API for this? That would make sure, that everyone would benefit from that feature, if it were necessary, regardless of the cache backend they're using. The database caching should really only be used in development environments, but never in production - it's just far too slow.

Please check 🐛 Fix Field cache for anonymous users using Cron job in Status formatter Closed: won't fix , which is the relevant issue.
Nodes with OfficeHours with an Open/Closed Status indicator are not refreshed when this status switches value. This is only for anonymous users, as Drupal caching works fine for authenticated users, not for anonymous users. See comment #5.

Is the above valid for your installations?
The issue's commit contains 4 options, as described in #14.
I found that the chosen option is the most accurate, and it only flushed the render cache, not the object cache.
Another option is to use EntityQuery, read all Entities that have a Status formatter (or have a formatter display that can expire) , and flush each entity for which the ache is expired. But i did not manage to connect the both (and it would read render_cache, too, since that contains the caching info per node)

I am open for suggestions.

Regarding the patch, it perhaps should bail out when the given cache bin/table does not exist. Not sure how to do that.

If render_cache is not OK, it should not be facilitated by Drupal.

Sounds interesting. I would have thought that when the office hour field set a correct time for how long the cache is valid, i.e. it should become invalid when the status of open/closed is going to change, then the normal cache invalidation should apply.

If that doesn't work, then you may have to set more specific cache tags to the fields so that you can use the cache API to find all relevant cache IDs that need to be invalidated manually, i.e. by cron.

In any event, using Database::getConnection()->select('cache_render') should be avoided. The module wants to provide a solution that always works, and going through the database query is not working in most cases. As mentioned before, database caching is discouraged on production sites.

Another consideration should be, if this really should be executed for every cron run. We have Drupal installations, where cron runs every minute. This would invalidate caching all the time and therefore would be counter productive.

While all this is not yet ready for prime time, it should be configurable and turned off by default. Otherwise, sites will just break unexpectedly.

The switch case in doCron() looks somehow broken too as it always goes for option 2 as this is using a constant where it never gets any different option.

As you have noticed, I have explored several options to enable this 'cache does not expire for anonymous users'.
I did not want to lose the alternatives, as provided by some users, so I decided to hardwire the selected option 2.
It does one selection for all expired render caches and then flushes only those. So, except for the mentioned flaws, it should be performant and precise. Cron jobs per minute are actually better than my 2-hour job in a test environment, since it may give the wrong Node display for almost 2 hours. :-)
IMO this should be handled by Drupal core, but apparently, it is not important enough.

The option 3 selects all Nodes with the Status enabled. But the determination if any such node is outdated is cumbersome. Using the cache tag is the best way. In your situation, where is the cache time stored, if not in render_cache?

Other issues (keyword 'cache') contain a solution proposing to use JS to fetch the open/closed status (for authenticated user caching). I discarded that option until now since the problem was always in the cache time calculation. And I never got feedback on the provided fixes.
But perhaps that is now a viable option? What do you think?

P.S. Now that 'exceptions' and 'seasons' are introduced, the complete formatter may need to be refreshed - not only the open/closed indicator.

"set more specific cache tags to the fields so that you can use the cache API to find all relevant cache IDs that need to be invalidated manually, i.e. by cron."

I could add the time to the cache tag, but there if no API to select from-to cache tags. So I need to use the bin storage.
is there a way to abstract the used bin?

Setting the status with javascript came to mind before too, I think that would be really nice.

Other than that, I'm unfortunately not a cache expert, only stumbled across this issue and saw the database query which is to be avoided. But I still wonder, if invalidation is really required as an expiration time is already set and that should identify those already as invalid at a certain point. Isn't Cache::garbageCollection() dealing with cleaning things up then?

Another idea could be to introduce your own cache bin for this purpose. But the javascript approach is the most attractive solution to me.

"Isn't Cache::garbageCollection() dealing with cleaning things up then?"

I do not recall, but given the code structure it seems OfficeHoursCacheHelper::invalidateTagsUsingRenderCache() is a copy of databaseBackend::garbageCollection().
However, the following DELETE code from databaseBackend has no effect:

     $this->connection->delete($this->bin)
        ->condition('expire', Cache::PERMANENT, '<>')
        ->condition('expire', REQUEST_TIME, '<')
        ->execute();

Whereas the following custom SELECT+INVALIDATETAGS code works fine:

      $result = $connection->select($bin)
        ->condition('expire', Cache::PERMANENT, '<>')
        ->condition('expire', $now, '<')
...
        ->fetchAllAssoc('tags');
...
    foreach ($result as $tags => $data) {
      $offset = strpos($tags, 'office_hours_status');
      $tag = strstr(substr($tags, $offset), ' ', TRUE);
      Cache::invalidateTags([$tag]);
    }

Is there an error in the core code?
I will test the custom code using a direct DELETE, and see what happens

But databaseBackend may not be used by a site. You should not make any assumptions on how to delete certain cache components, as each backend may implement that differently.

Indeed. I am not planning to call it directly, but in my test case this is the class that is doing the actual work.
And now I am puzzled: Is there an error in standard code? It looks like it should to the work already that i implemented recently.

I am happy to do the following pattern when I can find a proper API and the proper cache bin.

  foreach (Cache::getBins() as $cache_backend) {
    $cache_backend->garbageCollection();
  }

Only the rendering should be refreshed. THe entity cache is still valid.

Which cache bin would be appropriate in your case? Or is there no problem with anonymous caching when using OpCache/Varnish/Whaterver?

Fun fact: Issue for mentioned code , created today: 🐛 Not optimal query in DatabaseBackend::garbageCollection() Fixed

And now I am puzzled: Is there an error in standard code? It looks like it should to the work already that i implemented recently.

What do you mean by "standard code"? Are you referring to the code from the DatabaseBackend? That code only works, when the database backend is being used. But it isn't used when you e.g. use Redis as your caching backend, or APC or any of those multiple caching backends.

As you can't know what the backend is, you should not use any of the specific backend code. You should only go through the cache API and let the rest of magic happen by the upstream code.

Yes, I mean the code from DatabaseBackend.
I agree with you,
but when I have a problem "caching is not refreshed for anonymous users",
then I need to create a test case, taking into account the constraints of the test system (or do some crowd sourcing).
In this case: "1.edit a node set a closing time in the near future, 2.open a new anonymous session, 3. refresh the page before and after the end time. 5. Check each time the open/closed indicator".
Then the system in my test case should have the expected behaviour.

In any case, I will add all constraints to the cron code.

Serendipity: This comment from today proposes a JS solution : #2767779-48: Add Field Caching for 'Currently Open/Closed' formatter →

Well, I added a test for the existence of the render cache.
The selection of relevant cache items using the cache tags was already OK.
I think - whenever you update to new dev version or an upcoming 1.9 version - you will not experience any problem anymore.

I still am interested if the caching for anonymous users works fine in your case (of course, only if you have status formatter and anonymous users.)

Hey @johnv, building all those checks into the process, avoids the issue that people will see. But it is no solution to the problem you're trying to solve. While you are looking for the render bin in the database, you may be finding it there and then invalidate records there. But most people won't be using the database cache but something more performant instead. They still have a render cache, but not in the database. And the current code won't invalidate the caching tags there. So, for such setup it behaves as if it wasn't there in the first place.

I still am interested if the caching for anonymous users works fine in your case (of course, only if you have status formatter and anonymous users.)

I can't answer that question. We're using office hours module but not the status display.

Just came across a useful service which may well do what you're looking for:

\Drupal::service('cache_tags.invalidator')->invalidateTags(['thing:identifier']);

So, if you tag all your render arrays, that you eventually want to invalidate, with the same tag, then you can invalidate them with this command and don't have to worry about the used cache backend.

Thanks,
but my problem now is how to determine which caches/nodes must be invalidated for anonymous users. That is only listed in the cache_render table.

I will now focus on the better option ✨ Fix Field cache for anonymous users using JS callback in Status formatter Fixed

Thank you for fixing the fatal error on the cron script. With this change - which is hopefully soon included in a release - the code bails out silently instead of breaking badly.

I still want to emphasize that no functionality should rely on an assumption that the database is used as a caching backend. On busy sites, this should never be the case, and they might not have the cache_render table in their database at all. I would prefer not having such code in the codebase.

Hi Jukka,
I understand, and I agree.
the implemented solution is due to lacking knowledge from my part.

As per the other - above mentioned - issue, I will implement a solution without cron job - which is be definition too late.
Either (and for anonymous users for entity displays with status indicator):
- a JS callback, which is a common way, but I think it is not very nice
- a '#lazy_builder', which seems better, but which I do not seem to ge working.
- a cache_context per 'session' (functionally disabling the render cache in this case), which is now my favorite solution.

Per session cache context is effectively same as max-age: 0, no caching. Except that it still spends time on trying to cache things. Not recommended.

jurgenhaas → closed merge request !22

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

This issue only the error message, is there a new issue to fix the cache purge handling ?

As said before, no sql query should handle the cache, Cache API should be use exclusively.

I really don't understand what your trying to do in invalidateTagsUsingRenderCache ? Invalidate all entities mark with office_hours_status tag ?

As said by jurgenhaas you could simply mark all entities with a common cache tag in \Drupal\office_hours\OfficeHoursCacheHelper::getCacheTags:

      case 2:
        return [
          "$entity_type:$entity_id",
          "office_hours:status:all",
          "office_hours_status:$entity_type:$entity_id",
        ];

and clear it on cron \Drupal::service('cache_tags.invalidator')->invalidateTags(['office_hours:status:all']); which do the same thing.

Be aware that it seems a bit overkill to invalidate so much cache if no data is changed (but It didn't dig into the module code).

I also don't understand why you use the INVALIDATE_MODE constant + switches in you code, how do you change it ?