Problem/Motivation

The APCu backend is immediately expiring its cache entries. This is our default FastBackend. The problem is a type safe comparison. In this case $expire is a "-1" - a string. It comes out of the ConsistentBackend that way. Can happen after a web server restart.

On top of that the following point of #2272685: Incorrect handling of invalid cache items in apcu backend also exists:

ApcuBackend::set() passes the $expire (timestamp) directly as the $ttl (duration) to apc_store().

Proposed resolution

For now the TTL is not strictly needed because we check the expire date on cache get processing anyway, so let's remove it.

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

moshe weitzman created an issue. See original summary.

Wim Leers’s picture

And *how* exactly can $expire be a string?

Wim Leers’s picture

Issue tags: +Needs tests

A failing regression test would answer #2.

neclimdul’s picture

Just looking through core calls to set, almost everything uses a constant. This snippet from ChainedFastBackend looks like a likely culprit:

      foreach ($this->consistentBackend->getMultiple($cids, $allow_invalid) as $item) {
        $cache[$item->cid] = $item;
        // Don't write the cache tags to the fast backend as any cache tag
        // invalidation results in an invalidation of the whole fast backend.
        $this->fastBackend->set($item->cid, $item->data, $item->expire);
      }
Berdir’s picture

Yes. It's coming from the database (for the default mysql backend). Everything that comes from the database is a string.

We can also type cast to integer there, but we've had plenty of bugs already due to incorrect type safe checks when e.g. values from the database were involved.

Also, the code is still wrong. We're using an absolute expiration date as a ttl.

We also have #2272685: Incorrect handling of invalid cache items in apcu backend, maybe we should just do that, but I'm also not sure that completely removing ttl is the right thing to do. We've had similar discussions in the redis backend decided to use ttl where possible and consider allow_invalid to apply mostly to cache tags.

dawehner’s picture

I tired to reproduce it in a test but well, couldn't really.

Status: Needs review » Needs work

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

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

moshe weitzman’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
969 bytes

@berdir suggests to get rid of the buggy code instead of fixing it. Sounds good to me! We rely on our own cache expiration in prepareItem() instead of APCu's own eviction as originally proposed at #227268: Autocomplete not working for certain characters (bdfs). Once this is in, we will tackle the invalidation part in that issue.

Since we are removing the buggy feature (APCu eviction via TTL), and already have tests for cache expiration, I'm removing the Needs Tests tag. If someone disagrees, please suggest how to test APCu eviction inside of our default chain.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/lib/Drupal/Core/Cache/ApcuBackend.php
@@ -178,14 +178,8 @@ public function set($cid, $data, $expire = CacheBackendInterface::CACHE_PERMANEN
+    // Expiration is handled by our own prepareItem(), not APC.

Nit: s/APC/APCu/

moshe weitzman’s picture

Re: #10 - that nit applies to a lot of code comments in this APCuBackEnd. I suggest we proceed without fixing the nit but I'm happy to change it committers prefer.

webchick’s picture

Issue tags: +rc target triage

I'd like to see this get in during RC, so adding the tag so the core committers can discuss it.

catch’s picture

Issue tags: -rc target triage +rc target

Discussed briefly with alexpott and we also think this is a good rc target. Tagging.

I'm mulling this issue, but I think all the things that I'm mulling are off-topic, so will probably commit a bit later today.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

I think we should test this behaviour in Drupal\system\Tests\Cache\ApcuBackendUnitTest so that we don't add it back again thinking it is missing.

Berdir’s picture

Discussed with alexpott about testing this.

Using "apc_cache_info('user')", we can get a list of cached entries and their ttl. So we just need to extend a test method there and check that entry.

Note that we need to get the correct internal name, see how the factory builds the site prefix that is then combined with the bin.

kylebrowning’s picture

Fixes the Nit in #10

kylebrowning’s picture

FileSize
575 bytes

INterdiff was wrong sorry,

moshe weitzman’s picture

Status: Needs work » Needs review
FileSize
1.96 KB
1.41 KB

With test.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests
dawehner’s picture

Issue summary: View changes

Fixed the issue summary

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 268f6c3 on 8.0.x
    Issue #2581395 by moshe weitzman, kylebrowning, dawehner: Incorrect...

Status: Fixed » Closed (fixed)

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