Posted by Bevan on July 29, 2009 at 5:12am
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | base system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
Issue Summary
cache_get() does not check for time() < $cache->expire before returning $cache, and does not document that this is the responsibility of the caller. I'm not sure if it's better to fix this in code or documentation.
I have verified this in Drupal 6, and believe it also applies to Drupal 5 and 7 (but have not verified).
Comments
#1
I blogged about this issue in more detail at http://civicactions.com/blog/2009/jul/28/drupal_gotchya_cache_get_return...
#2
We should confirm this behavior in 7.x and fix there before we look at 6.x.
#3
Note that any patches for Drupal 7 would be completely different for Drupal 6, not 'portable' and need to be rewritten.
#4
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.
#5
D6 patch attached.
#6
The last submitted patch failed testing.
#7
oops, forgot to handle CACHE_PERMANENT. New patches attached.
#8
Let's write that in multiple lines and add some code comments.
#9
I forgot to add in the ticket description that while this is a bug, it is not critical since cron acts as the garbage collector and deletes expired cache items. Nevertheless is an issue, since module developers expect a different behaviour.
As well as fixing Drupal's core cache mechanism (tables) should we notify/check contrib cache systems such as memcache?
#10
Also, this is an easy one to write regression tests for simpletest with, and IMHO shouldn't be committed without tests for this case.
#11
Now with multiple lines, comments, and a test to ensure expired cache items are not returned.
#12
The last submitted patch failed testing.
#13
Hmm... strange. Minor adjustment, and let's try again.
#14
... and uploaded the same patch again...
Here's Rev4 (the proper one)
#15
The last submitted patch failed testing.
#16
#17
The last submitted patch failed testing.
#18
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
#19
The last submitted patch, 534092-cache-returns-expired-objects-rev5.patch.patch, failed testing.
#20
#18: 534092-cache-returns-expired-objects-rev5.patch.patch queued for re-testing.
#21
The last submitted patch, 534092-cache-returns-expired-objects-rev5.patch.patch, failed testing.
#22
#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).
#23
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.
#24
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.
#25
#24 works overherethx 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).
#26
Subscribe.