Permanent items are delete from cache even if $cid is NULL

I feel like a better solution is to let Redis itself expire things. Just because the Drupal Database Cache does things a certain way doesn't mean it's the right way for another cache backend.

This patch gives CACHE_TEMPORARY items a somewhat arbitrary TTL. It's based off the cache_lifetime variable, but it is at least a minute. We don't want Redis to expire things as soon as we set them, after all. After that, we should only get many keys if the $wildcard parameter is TRUE. If you give cache_clear_all() a $cid of '*' without setting $wildcard, that's an error.

How about this patch?

"If caller is stupid a this operations gives"

There are two typos: an extra "a" after stupid and operations should be a singular operation. Saying more explicitly what is stupid would be more helpful than just being vaguely judgmental. How about

"If caller sends an expiry time before now this operation gives"

So you don't like a bit of humor in comments :) Thanks for revealing the typos, will fix it tomorow morning (it's almost night here).

Tagging

I fixed the typo as greggles suggested. Commiting the fix within the day, thanks everyone.

Ok, so this is really bad.

       case CACHE_TEMPORARY:
+        $pipe->expire($key, variable_get('cache_lifetime', 0));
+        break;
+

That means if you have cache_lifetime at 0, which is a recommended setting, the page cache essentially doesn't work, because it expires itself immediately. You may as well not do the cache set at all at that point.

This issue is a bit confusing, but if I'm understanding it right, we're just trying to make redis handle CACHE_TEMPORARY properly. However, I don't get why CACHE_TEMPORARY items are ever being expired, they should only be cleared on cache_clear_all().

Attached patch is how I would do this, by simply keeping an additional list of our CACHE_TEMPORARY items, and then clearing them all on a cache_clear_all().

Also, that patch is 100% untested, just sketching out the idea.

It would also be good to set a far future expire on those items so that they'd get cleaned up when redis' LRU kicks in.

Yes it is untested, and yes it is really bad, sorry for this. Fixing it right now, thanks.

        case CACHE_TEMPORARY:
          $lifetime = variable_get('cache_lifetime', 0);
          if (0 < $lifetime) {
            $pipe->expire($key, $lifetime);
          }
          break;

Should fix it, what's your opinion about this?

Here is a patch that I'm going to commit right away. It fixes both Predis and PhpRedis backend, added some documentation in code and complete unit tests for this issue and expiry checks. Thank you very much once again, that's a huge bug you found!

New 7.x-2.1 release on its way, thank you again.

That seems much better, thanks for fixing that so fast.

What do you think of my idea on just keeping a list of cache temporary items so the behavior can better match existing cache backends? With the current patch that went in, won't they never be expired, even on a cache_clear_all()?

What do you think of my idea on just keeping a list of cache temporary items

Not sure this would be a good idea to keep lists of cache items: I would be afraid to uselessly raise CPU and RAM usage for cache handling which would IMO defy the purpose of it. But if you have a concrete scenario or real life measures that prooves me wrong I'd be happy to discuss.

It would also be good to set a far future expire on those items so that they'd get cleaned up when redis' LRU kicks in.

Nevertheless, that seems to be quite a good workaround for wiping them out, I'd be a good thing to experiment.

With the current patch that went in, won't they never be expired, even on a cache_clear_all()?

I didn't see that side of the fix but I think you are right: with the current patch temporary items with a cache lifetime set to 0 are never been cleared when cron kicks in.

Looking at the actual database backend code confirms that items are dropped at cache_clear_all() time when they are temporary which does not happen into the Redis module. I know that this creates a different behavior between core and this module. Now pragmatically this happens during every hook_cron() run: we know that on a lot of hardcore badass sites, we need cron to run up to every 5 minutes: this means that all temporary caches (and page and block) will be dropped every 5 minutes with original algorithm: this is *very* bad for performances.

I'm thinking that I'd like to keep Redis not flushing them out arbitrarily and keep them for their lifetime or until a flush all caches call happen (or when the business code explicitely wipe them out). At least keeping that behavior configurable in settings.php.

What do you think?

Anyway, all of this needs a new issue, I will try to manage some time the next week to do that.

This totally breaks proper invalidation for the cache_page bin in D6 (we use Pressflow), and causes stale content displayed even if the node has been edited.

Here is our default configuration: http://drupalcode.org/project/octopus.git/blob/HEAD:/aegir/conf/global.i... which worked prior to this issue/commits.

And the fork with both last commits reverted to get it back to work (as before) properly: https://github.com/omega8cc/redis/commits/7.x-2.x-o8

I will try to debug this and provide a patch, so just a heads up for now, that the current stable is broken for D6 sites.

D6 only?

No, it affects both D6 and D7. Just tested this again.

While debugging this I have found something else related to D6 only, which is totally odd to me, and may affect caching in general.

function page_set_cache() {
  global $base_root;

  if (drupal_page_is_cacheable()) {
    // This will fail in some cases, see page_get_cache() for the explanation.
    if ($data = ob_get_contents()) {
      ob_end_clean();
      $cache_lifetime = variable_get('page_cache_lifetime', 0);

      if (variable_get('page_compression', TRUE) && extension_loaded('zlib')) {
        $data = gzencode($data, 9, FORCE_GZIP);
      }

      $cache = (object) array(
        'cid' => $base_root . request_uri(),
        'data' => $data,
        'expire' => $cache_lifetime > 0 ? $cache_lifetime : CACHE_TEMPORARY,
        'created' => $_SERVER['REQUEST_TIME'],
        'headers' => array(),
      );

      // Restore preferred header names based on the lower-case names returned
      // by drupal_get_header().
      $header_names = _drupal_set_preferred_header_name();
      foreach (drupal_get_header() as $name_lower => $value) {
        $cache->headers[$header_names[$name_lower]] = $value;
      }
      cache_set($cache->cid, $cache->data, 'cache_page', $cache->expire, serialize($cache->headers));
      drupal_page_cache_header($cache);
    }
  }
}

Note the line:

$cache_lifetime = variable_get('page_cache_lifetime', 0);

This is the only place in D6 core where such variable is used, so it is most probably a mistake/typo in the core which can do some really nasty things to the caching logic on its own. It should be cache_lifetime as everywhere else, no?

Not sure, Drupal 6 maintain a separate cache lifetime for pages if I remember correctly. I'm going to try to fix the Redis module right now.

I mean, it may indirectly affect also Redis integration and is an obvious core bug to me, because the page_cache_lifetime string literally exists in this one line only, so it is not used/effective. I'm going to open a bug report in core for this.

I want to slap so hard those who wrote the Drupal 7 cache backend interface... Seriously WTF most of the behaviors hardcoded in the DatabaseCacheBackend never have been documented at the interface level nor implemented at a higher level: this is the real problem.

Technically the Redis module cannot do that in the current state: either we force a full clear() (including permanent items) or we don't drop anything, there is no compromise easily possible except by iterating over the full bin and manually expire keys that are non permanent when cache_lifetime is set to 0.

But Redis has evolved so quickly, there is probably a way to do a better select operation in hashes.

Side note: I have opened a separate issue for this page_cache_lifetime thing in D6: #2130865: There is no such variable like page_cache_lifetime

Good thing you did.

I'm blocked with this very stupid cache usage here. There are a couple of "easy" solutions:

• Reproduce the hook_cron() behavior and force page and block cache bins to be flushed at cron time: no fix at the backend level, only an applicative fix at the redis.module level. Make this behavior configurable and enabled per default.
• Store temporary items cids in a SET of keys aside, flush them all iterating on the full set at clear() call time with no arguments. I'm not really fond of this solution because it'll make it a bit more memory consuming, and clear() operations won't be scalable if you have a huge amount of temporary items in your cache.

What do you think about those?

Since relying on cron is not enough here, because cache_page bin has to be properly managed on various events, like node edit etc, and adding complexity, with possible performance degradation (thanks to checks multiplied) may introduce more problems and regressions than we are trying to solve, I would prefer to keep it simple and just purge the bin when needed. Speed is not enough when people see stale content, checkout doesn't work and orders are lost etc. We need some balance between aggressive enough cache clears and cache backend consistency and reliability.

I will add the SET temporary item cid storage in order to achieve ISO functionality with the database backend meanwhile I'll leave this configurable with two additional behavior: replace the temporary flush by a full flush (legacy behavior of this module pre 1.0 release) and no temporary flush (for people that want aggressive caching). If I'm not too lazy I'd make this configurable per bin and put sensible defaults where only page and block are wiped out accordingly to spec.

But I probably won't be able to do it before next week.

Thanks for your time, it's highly appreciated, if you have any other comment or observation, please do! Thanks again.

Wow, that sounds exciting! Looking forward to see and test patches. Thank you for your continued support.

Ok, I fixed it! I think.

I implemented what I explained upper: cache backends have three behaviors and put sensible defaults depending on the cache bin. I also added massive unit tests about this specific issue, and fixed the lock backend unit tests at the same time (you can now run full core unit tests using this module as well as run both PhpRedis and Redis unit tests in the same PHP runtime, which is great).

See this commit #45dad1f.

And for a detailed explaination and avanced usage read this (link towards the README.txt file in the git repo).

Please review and test it and report any bugs. Any suggestions or comments about this patch are welcome.

This patch also fixes PhpRedis specific problems due to behavior changes in the PHP extension as well as fixing a few typo errors, minor optimizations, and moreover test it all!

I pushed two releases do not use the 7.x-2.2 which ships with a potential fatal error when using PhpRedis. 7.x-2.3 fixes that and embed all the previous fixes.

Wow, just wow! It works! I have tested various non-default modes to see if I can break something, but the defaults just work! Marvelous! We just released BOA-2.1.1 with previous version included, but I will now add 2.3 as a hot fix to stay up to date. Thank you!

Be careful with lock implementation, it's still experimental and we experience random deadlocks sometime. I should have documented this. I you ever reproduce such locks please provide feedback as soon as possible, I didn't manage to reproduce it on a development box.

Thanks you very much for your very quick feedback.

We have tested Redis lock both with D7 and D6 sites for a long time already, and there were literally zero issues, so we are enabling it by default now, which may help to catch edge case issues, if any will appear, so I will report them for sure. Thanks again!

You're welcome. Gald to see this is all working fine for you, having feedback is always a pleasure.

Have to reopen this, since we have tested this on D7 only (ouch!), while D6 still doesn't work and emits warnings:

Warning: Missing argument 2 for variable_get(), called in /data/all/000/modules/redis/lib/Redis/Cache/Base.php on line 136 and defined in /data/all/001/pressflow-6.28.3/includes/bootstrap.inc on line 638

Warning: Missing argument 2 for variable_get(), called in /data/all/000/modules/redis/lib/Redis/Cache/Base.php on line 139 and defined in /data/all/001/pressflow-6.28.3/includes/bootstrap.inc on line 638

Warning: Missing argument 2 for variable_get(), called in /data/all/000/modules/redis/lib/Redis/Cache/Base.php on line 136 and defined in /data/all/001/pressflow-6.28.3/includes/bootstrap.inc on line 638

Warning: Missing argument 2 for variable_get(), called in /data/all/000/modules/redis/lib/Redis/Cache/Base.php on line 139 and defined in /data/all/001/pressflow-6.28.3/includes/bootstrap.inc on line 638

Warning: Missing argument 2 for variable_get(), called in /data/all/000/modules/redis/lib/Redis/Cache/Base.php on line 136 and defined in /data/all/001/pressflow-6.28.3/includes/bootstrap.inc on line 638

Warning: Missing argument 2 for variable_get(), called in /data/all/000/modules/redis/lib/Redis/Cache/Base.php on line 139 and defined in /data/all/001/pressflow-6.28.3/includes/bootstrap.inc on line 638

Oh my bad, I didn't tested with cache_backport. I will fix that, thanks for reporting.

Maybe it is because in D6 empty variable_get() doesn't default to NULL, like it is in D7, so the check for NULL never works here?

I mean the check here:

    if (null !== ($mode = variable_get('redis_flush_mode_' . $this->bin))) {
      // A bin specific flush mode has been set.
      $this->clearMode = (int)$mode;
    } else if (null !== ($mode = variable_get('redis_flush_mode'))) {
      // A site wide generic flush mode has been set.
      $this->clearMode = (int)$mode;
    } else {

This commit fixes at least these warnings, but I will test it further to make sure everything else works as expected also in D6. Patch for review attached.

Status update.

Just realized that proposed fix to avoid warnings in D6 will change the logic, since it will no longer be able to check for D7 specific NULL and simply default to $mode = 0; which is probably not exactly what it is supposed to do, since it will never reach defaults defined in the else part.

Attached patch should do the trick.

Yet another patch for variable_get() to avoid silent fails on D6 sites.

Pass null if the call expects null where there is no value set! Easy :) Thanks for the patches I'll review this tomorow morining.

By the way, re: "Be careful with lock implementation, it's still experimental and we experience random deadlocks" -- maybe this happens in multisite, since lock implementation doesn't seem to be multisite-aware? At least, it doesn't use any prefix as cache bins do.

Right nice catch, this worth open an issue for adding prefixing to lock backend; Opened #2134001: Add missing key prefix usage to lock as well.

Git commit #ad6b76c should fix #42, #43 and #44. I did not apply directly your patches because the logic was indeed a bit broken 'cache_lifetime' has a default value which is supposedly "0" in core: so I added some class constants to fix that. Once again I have to thank you. I am releasing a new version (7.x-2.4) that should shows up soon on project page. Feedback will be appreciated as always.