Fix expiry calculation #32
Conversation
Expiry is a timestamps and ttl is an offset so they have to be standardized before doing comparisons.
|
not sure if I would need to update something to show the travis results, it did create the job but it's still queued: https://travis-ci.org/github/md-systems/redis/jobs/740887346 |
|
But yes, as I suspected. This breaks the existing test coverage in GenericCacheBackendUnitTestBase.php::156 that ensures that you can get an expired item if you request to do so, which is of course not possible if you actually set the expiration date in redis. So the current behaviour is kind of by design, to satisfy the expirations that Drupal core has on the cache API. (in a weirdly broken way instead of just hardcoding the expiration with an explanation) I think if we "fix" it, then we have to make it configurable and opt-in and document the drawback of doing so. Also, still think there are existing issues on d.o about this (beside yours) |
|
Interesting. That is some weirdness. That's just a side effect of core's database cleanup happening on a schedule though right? I found this looking into what seemed like an overly large redis database. It seems like a ton of caches I'd tuned to roughly a session are basically never expiring because they're getting the year out ttl and never getting GC'd. It seems like in an effort to replicate one semi broken core behavior the side effect is another broken behavior since there is no GC on the redis classes. 😞 Could we maybe "fake" the GC by keeping logic like this patch but setting something like a expire + 5min as the ttl so there is a ttl that redis will GC but also something similar to cores expired but needs rebuild window can exist as well? We could even make that a setting you could 0 out to have redis expire things immediately if that tuning worked better for a site. |
|
Sounds good to me. Not sure if we should set it to a sane default or the current max ttl for BC. I'd be open to set it to a lower value if we don't make it too low. I mean, as you correctly pointed out, in core it's also only until the next system_cron() run, so... FWIW, most cache items in Drupal don't have an explicit expiration I would expect, but I suppose there are cases where that is commonly used. |
| @@ -267,7 +273,7 @@ public function setPermTtl($ttl = NULL) { | |||
| else { | |||
| if ($iv = DateInterval::createFromDateString($ttl)) { | |||
| // http://stackoverflow.com/questions/14277611/convert-dateinterval-object-to-seconds-in-php | |||
| $this->permTtl = ($iv->y * 31536000 + $iv->m * 2592000 + $iv->days * 86400 + $iv->h * 3600 + $iv->i * 60 + $iv->s); | |||
| $this->permTtl = ($iv->y * 31536000 + $iv->m * 2592000 + $iv->d * 86400 + $iv->h * 3600 + $iv->i * 60 + $iv->s); | |||
There was a problem hiding this comment.
Oh nice catch ! This was my mistake, sorry for this.
No description provided.