#962422: Empty cache stampede protection shifted the handling of expired cache entries from dmemcache.inc to cache.inc. It makes sense to do the cache stampede protection entirely in cache.inc, but this means lock.inc and session.inc are missing expires handling (although I haven't reviewed the code yet so this could be a false alarm).

CommentFileSizeAuthor
#9 memcache-expire-6.x-1150072-9.patch844 bytesjeffsheltren
#9 memcache-expire-7.x-1150072-9.patch742 bytesjeffsheltren
#5 expires.patch1.2 KBcatch
#4 expire_d7.patch1.18 KBcatch
#3 expires.patch1.19 KBcatch

Comments

catch’s picture

catch
he/him
Primary language English
commented
Priority: Major » Normal

This was a false alarm, while dmemcache.inc used to handle the expiry and stampede protection logic and this was moved into memcache.inc, only memcache.inc actually abuses the expires value - lock and sessions just set it as you'd expect. So it was fine both ways.

One thing I'm wondering, instead of CACHE_PERMANENT, could we do $expire * 2 - this would mean that items get a decent window for stampede protection, but if they actually do expire for a while, memcache could replace them instead of having to do evictions - this might increase overall hit rate, and would also mean there's a limit on how stale the items we serve from stampede protection are. I don't think this is worth putting into 1.9 though, so downgrading this and not posting a patch. If we wanted to do this we'd probably also want to shorten the lifetime for CACHE_TEMPORARY (say to 24 or 72 hours - it's currently a month - that might need to be a variable if we did that though).

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review

Here's a patch.

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new expires.patch1.19 KB
catch’s picture

catch
he/him
Primary language English
commented
Version: 6.x-1.x-dev » 7.x-1.x-dev
Issue tags: +Needs backport to D6
StatusFileSize
new expire_d7.patch1.18 KB

And for D7.

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new expires.patch1.2 KB

D6 doesn't have the REQUEST_TIME constant.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Fixed

OK I've tested this locally and it's working fine, so I've committed/pushed to both branches.

Status: Fixed » Closed (fixed)

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

jeremy’s picture

jeremy commented
Status: Closed (fixed) » Active

In the event that expire is CACHE_PERMANENT, it appears you'll end up setting the expire to a large negative number (and it's unclear to me how what memcache set does with that).

Perhaps should instead do something like (thanks Bdragon):

   $memcache_expire = $cache->expire ? $cache->expire + (($cache->expire - $_SERVER['REQUEST_TIME']) * 2) : 0;
jeffsheltren’s picture

jeffsheltren commented
Status: Active » Needs review
StatusFileSize
new memcache-expire-7.x-1150072-9.patch742 bytes
new memcache-expire-6.x-1150072-9.patch844 bytes

Here are some patches for 6.x and 7.x which do the same thing as Jeremy suggested, though I more explicitly check that $cache->expire is set to CACHE_PERMANENT instead of relying on the fact that CACHE_PERMANENT happens to be 0.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Active

Thanks for this, committed to both branches.

Looking at this again, I noticed we're also increasing the length of CACHE_TEMPORARY items which isn't really the intention, so moving this back to active for that. That won't really do any harm since they get invalidated very, very often anyway but wasn't supposed to change here.

jeremy’s picture

jeremy commented
Issue summary: View changes
Status: Active » Fixed

this logic was completely rewritten in the past eight (!) years -- this issue is no longer valid, closing

Status: Fixed » Closed (fixed)

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