Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#18 | interdiff.txt | 1.41 KB | moshe weitzman |
#18 | incorrect_expiration_in-2581395-18.patch | 1.96 KB | moshe weitzman |
#17 | interdiff.txt | 575 bytes | kylebrowning |
#16 | incorrect_expiration_in-2581395-16.patch | 970 bytes | kylebrowning |
#9 | incorrect_expiration_in-2581395-9.patch | 969 bytes | moshe weitzman |
Comments
Comment #2
Wim LeersAnd *how* exactly can $expire be a string?
Comment #3
Wim LeersA failing regression test would answer #2.
Comment #4
neclimdulJust looking through core calls to set, almost everything uses a constant. This snippet from ChainedFastBackend looks like a likely culprit:
Comment #5
BerdirYes. 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.
Comment #6
dawehnerI tired to reproduce it in a test but well, couldn't really.
Comment #9
moshe weitzman CreditAttribution: moshe weitzman at Acquia commented@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.
Comment #10
Wim LeersNit: s/APC/APCu/
Comment #11
moshe weitzman CreditAttribution: moshe weitzman at Acquia commentedRe: #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.
Comment #12
webchickI'd like to see this get in during RC, so adding the tag so the core committers can discuss it.
Comment #13
catchDiscussed 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.
Comment #14
alexpottI 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.
Comment #15
BerdirDiscussed 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.
Comment #16
kylebrowning CreditAttribution: kylebrowning commentedFixes the Nit in #10
Comment #17
kylebrowning CreditAttribution: kylebrowning commentedINterdiff was wrong sorry,
Comment #18
moshe weitzman CreditAttribution: moshe weitzman at Acquia commentedWith test.
Comment #19
Wim LeersComment #20
dawehnerFixed the issue summary
Comment #21
catchCommitted/pushed to 8.0.x, thanks!