cache_get() returns expired cache items

We should confirm this behavior in 7.x and fix there before we look at 6.x.

I wrote a little bit of code to test this in D7, and sure enough - it will return expired objects.

Patch attached fixes this in D7. I will look at one for D6 next.

D6 patch attached.

oops, forgot to handle CACHE_PERMANENT. New patches attached.

Let's write that in multiple lines and add some code comments.

Now with multiple lines, comments, and a test to ensure expired cache items are not returned.

Hmm... strange. Minor adjustment, and let's try again.

... and uploaded the same patch again...

Here's Rev4 (the proper one)

Thanks so much! This gotcha drove me crazy for a while until I found this issue.

Unless I'm mistaken, using REQUEST_TIME instead of time() could be problematic in this particular situation, especially doing tests that can run for minutes. Or do I have that wrong? (I'm relatively new to writing tests.)

In addition, I see a few problems with the test in rev4. It asserts true on cache_get (assertCacheExists ultimately calls cache_get), then immediately asserting false on cache_get. Also, as clever as setting a cache item as expiring 10(+) seconds in the past is, it seems a bit too forced -- as you're creating a cache item that has a expiration date that's even earlier than its create date.

I've changed the test to flow a bit more "naturally":

1) Set cache item to expire in 10 seconds from NOW
2) Immediately test that item has been created and is being returned
3) Wait 20 seconds
4) Test that item is no longer being returned by cache_get and, thus, is expired

#18: 534092-cache-returns-expired-objects-rev5.patch.patch queued for re-testing.

#18: Each test class is run in a separate request but these can be quite big too, yes. On a fast test server however, who processes all ~400 test classes in a few minutes, it shouldn't be minutes but only a few seconds though.

For that speed reason, I think we should really avoid a test that has a sleep of 20 seconds. That means that every single test run takes 20s longer, doing nothing in that time (Well not exactly, because multiple tests are run at the same time, but it still slows down).

Patch doesn't look right - we have CACHE_TEMPORARY too, and it will always return false for that.

In 8.x we need to completely kill CACHE_TEMPORARY and force people to set an explicit expiry, for D7 this could be an extra check for -1 I guess.

New patch. Changes:
- Added checking CACHE_TEMPORARY.
- Changes to tests, removing sleep().
- Comparing against REQUEST_TIME instead time(). However update.module does implement its own _update_cache_get() and uses time() to check for staleness... and it seems nowhere else in core are we setting a specific limited cache lifetime (except update.module... but that uses its own _update_cache_get instead of core cache_get).

Setting as needs review for testbot.

#24 works overhere
thx for the patch!

Edit:
seems like #24 still has an issue.
After logging in one day later, I get this error...

Notice: Undefined property: stdClass::$cache in DrupalDatabaseCache->prepareItem() (line 406 of C:\projects\MyProject\trunk\includes\cache.inc).

Subscribe.

The patch no longer applies. In fact it looks like cache_get() was removed from drupal 8.

New patch for review.

This also clarifies the documentation in set() in CacheBackendInterface.php for the third argument:

    *   - A Unix timestamp: Indicates that the item should be kept at least until
-   *     the given time, after which it behaves like CACHE_TEMPORARY.
+   *     the given time, after which it will behave as if it did not exist in
+   *     the cache (and will be completely removed at the next general cache
+   *     wipe).
    */

Agh, missed something. New patch soon.

New patch. Go testbot go!

#31: 534092-31-cache-expired-items.patch queued for re-testing.

Documentation on cache_set implies that current behaviour is by design and that the expire timestamp guarantees a minimum lifetime for an entry, not a maximum. This behaviour stems from Drupal 4.0 (or even before?) and was documented in Drupal 4.7.x on July 13th 2004.

Heine: I see your point. Yes, the documentation is correct; the data will still exist in the cache bin (in the DB/Memcached/etc). Even after this patch. However... the documentation is correct if "the item is kept at least until the given time" means "the item will still exist inside the cache storage mechanism at least until the given time".

I think, however that Developers expect also to be able to specify an exact time when the data, if it still exists in the cache, is "stale", which means "the item still exists in the cache storage mechanism, but it is no longer of use since it was specified to be so after the given time (which has already passed)".

In this case the patch does not change the "keep in the cache" part, it just makes sure that cache()->get() returns FALSE when that data is "stale". In this case most code in contrib has to add an extra if() that checks cache()->get('foo')->expires against REQUEST_TIME or similar. So the patch is moving things upstream.

On another note, even though it's a behavioral change, there's little danger of breaking things since now, modules (generally) don't know how much time is left before cron runs and their cache entries (set to an expire timestamp > 0) are cleared.

I think the arguments against not doing this could be if there were use cases where it would be necessary get back stale data from a cache()->get(). Since IMO the answer is "no" (but I could be wrong), I think the patch is a good idea:: better DX, less duplicate code checking for staleness on every cache get call that contains things that should expire at a certain time.

Now, perhaps we could still improve the wording in the documentation in set(). Below is another attempt.

    *   - A Unix timestamp: Indicates that the item should be kept at least until
-   *     the given time, after which it behaves like CACHE_TEMPORARY.
+   *     the given time. After this time, cache()->get() will return FALSE for
+   *     this item. The item will then be completely removed at the next general
+   *     cache wipe.
    */

And yes, perhaps not "bug" but "feature request".

I've run into this issue whereby I set items in the cache on the assumption that when the expiry time arrives, the next query will not return expired content and instead will return fresh content.

After reading the cache documentation it was my assumption that after $cache->expire is surpassed, cache_get would return false and I would be able to place fresh content in the cache. This simply meant that every time I implemented cache_get I would have to check that $cache->expire > REQUEST_TIME for it to work in the manner I expected and desired.

The above patch no longer applies to the current D8 HEAD so I've rewritten it for the current HEAD. It passes all tests created locally and seems to work as expected.

Alternative approach: #1774332: Better handling of invalid/expired cache entries

In this approach, $cache->get() by default only returns non-expired values, but a second optional argument allows stale (expired or invalidated via $cache->invalidateTags()) to be returned.

#36: cache-expired-items-534092-36.patch queued for re-testing.

With the fix for #1774332: Better handling of invalid/expired cache entries, cache()->get() no longer returns expired items, unless explicitly instructed to do so.

Closing this ticket.

Shouldn't we still fix this bug in D7 as per the original issue?

For anyone finding this issue for D7 and looking for a workaround, just check that time() < $cache->expire

It's a feature, not a bug.

This feature request changes the semantics of the $expire argument of cache_set(). This breaks BC, so I don't think this is D7 material.

Fair enough.

doc block for cache_get() should describe the behavior of expired items

See https://api.drupal.org/comment/24163#comment-24163

This is very lame but the $expire parameter, when set in a form of a Unix timestamp, is ignored if the parameter cache_lifetime set in admin/config/development/performance is not set to X minutes.

This will cause cache_get to return expired entries!

Sadly, the description for the cache_lifetime parameter is incorrect because it mentions "cached pages" so one would assume that it's applicable to whole pages and not specific cache entries.

This is not documented and should not even happen otherwise it destroys the purpose of setting an expire parameter.

I also posted this in the comments of cache_set and cache_get.