Build in workaround for Enterprise_PageCache's not sane management strategy

[mpchadwick]

Hi @colinmollenhour -

Apologies in advance for the long-winded issue (feature request).

EnterprisePageCache uses a pretty awful strategy for managing the cache.

It works like this...

• No expiry is set when storing request responses to cache
• Once the cache gets bigger than the "Max Cache Size" stop saving anything and "invalidate" the cache (does nothing other than show a message in the admin)

There are many problems with this approach...

• A document that is only accessed once ever will stay in the cache forever
• The cache will reach a point where writes are longer possible, which can be dangerous if a promo email goes out to a new URL after the cache has been "invalidated"

This is a topic I cover in both a blog post and a talk.

There is a closed issue that is related to this where it was incorrectly assumed that Magento was defaulting to false (it defaults to NULL).

It would be possible to handle this the application as suggested then by subclassing Enterprise_PageCache_Model_Processor, redefining processRequestResponse and using the child class as the request processor, however I feel like it would be nice to not have to do that by adding the ability to work around this limitation (bug?) of Magento directly in the module that is actually responsible for saving to the cache.

We have in fact already done this at Something Digital by subclassing this module, but I'd like to see if you would accept it upstream. The idea is to introduce 2 additional configuration values (currently called sd_expire and sd_refresh_on_load) although we can rename them for upstream.

The strategy is to use the sd_expire configuration value to set the TTL (if it hasn't already been set) and refresh the TTL on access (assuming sd_refresh_on_load is set). This idea behind this strategy is to keep the cache primed with the responses that are accessed frequently and expire out the things that aren't.

I've included the code I have in use at SD below. Are you open to including this?

<?php

class SomethingDigital_Cache_Backend_ExpiringFpcRedis extends Cm_Cache_Backend_Redis
{
    public function __construct(array $options)
    {
        parent::__construct($options);

        if (!empty($options['sd_expire'])) {
            $this->_sdExpire = $options['sd_expire'];
        }

        if (!empty($options['sd_refresh_on_load'])) {
            $this->_sdRefreshOnLoad = $options['sd_refresh_on_load'];
        }
    }

    public function save($data, $id, $tags = array(), $specificLifetime = false)
    {
        if(!is_array($tags))
            $tags = $tags ? array($tags) : array();
        else
            $tags = array_flip(array_flip($tags));

        $lifetime = $this->getLifetime($specificLifetime);

        $lifetime = $this->getSdExpiringLifetime($lifetime, $id);

        if ($this->_useLua) {
            $sArgs = array(
                self::PREFIX_KEY,
                self::FIELD_DATA,
                self::FIELD_TAGS,
                self::FIELD_MTIME,
                self::FIELD_INF,
                self::SET_TAGS,
                self::PREFIX_TAG_IDS,
                self::SET_IDS,
                $id,
                $this->_encodeData($data, $this->_compressData),
                $this->_encodeData(implode(',',$tags), $this->_compressTags),
                time(),
                $lifetime ? 0 : 1,
                min($lifetime, self::MAX_LIFETIME),
                $this->_notMatchingTags ? 1 : 0
            );

            $res = $this->_redis->evalSha(self::LUA_SAVE_SH1, $tags, $sArgs);
            if (is_null($res)) {
                $script =
                    "local oldTags = redis.call('HGET', ARGV[1]..ARGV[9], ARGV[3]) ".
                    "redis.call('HMSET', ARGV[1]..ARGV[9], ARGV[2], ARGV[10], ARGV[3], ARGV[11], ARGV[4], ARGV[12], ARGV[5], ARGV[13]) ".
                    "if (ARGV[13] == '0') then ".
                        "redis.call('EXPIRE', ARGV[1]..ARGV[9], ARGV[14]) ".
                    "end ".
                    "if next(KEYS) ~= nil then ".
                        "redis.call('SADD', ARGV[6], unpack(KEYS)) ".
                        "for _, tagname in ipairs(KEYS) do ".
                            "redis.call('SADD', ARGV[7]..tagname, ARGV[9]) ".
                        "end ".
                    "end ".
                    "if (ARGV[15] == '1') then ".
                        "redis.call('SADD', ARGV[8], ARGV[9]) ".
                    "end ".
                    "if (oldTags ~= false) then ".
                        "return oldTags ".
                    "else ".
                        "return '' ".
                    "end";
                $res = $this->_redis->eval($script, $tags, $sArgs);
            }

            // Process removed tags if cache entry already existed
            if ($res) {
                $oldTags = explode(',', $this->_decodeData($res));
                if ($remTags = ($oldTags ? array_diff($oldTags, $tags) : FALSE))
                {
                    // Update the id list for each tag
                    foreach($remTags as $tag)
                    {
                        $this->_redis->sRem(self::PREFIX_TAG_IDS . $tag, $id);
                    }
                }
            }

            return TRUE;
        }

        // Get list of tags previously assigned
        $oldTags = $this->_decodeData($this->_redis->hGet(self::PREFIX_KEY.$id, self::FIELD_TAGS));
        $oldTags = $oldTags ? explode(',', $oldTags) : array();

        $this->_redis->pipeline()->multi();

        // Set the data
        $result = $this->_redis->hMSet(self::PREFIX_KEY.$id, array(
          self::FIELD_DATA => $this->_encodeData($data, $this->_compressData),
          self::FIELD_TAGS => $this->_encodeData(implode(',',$tags), $this->_compressTags),
          self::FIELD_MTIME => time(),
          self::FIELD_INF => $lifetime ? 0 : 1,
        ));
        if( ! $result) {
            throw new CredisException("Could not set cache key $id");
        }

        // Set expiration if specified
        if ($lifetime) {
          $this->_redis->expire(self::PREFIX_KEY.$id, min($lifetime, self::MAX_LIFETIME));
        }

        // Process added tags
        if ($tags)
        {
            // Update the list with all the tags
            $this->_redis->sAdd( self::SET_TAGS, $tags);

            // Update the id list for each tag
            foreach($tags as $tag)
            {
                $this->_redis->sAdd(self::PREFIX_TAG_IDS . $tag, $id);
            }
        }

        // Process removed tags
        if ($remTags = ($oldTags ? array_diff($oldTags, $tags) : FALSE))
        {
            // Update the id list for each tag
            foreach($remTags as $tag)
            {
                $this->_redis->sRem(self::PREFIX_TAG_IDS . $tag, $id);
            }
        }

        // Update the list with all the ids
        if($this->_notMatchingTags) {
            $this->_redis->sAdd(self::SET_IDS, $id);
        }

        $this->_redis->exec();

        return TRUE;
    }

    public function load($id, $doNotTestCacheValidity = false)
    {
        $data = parent::load($id, $doNotTestCacheValidity);

        if ($data && $this->_sdExpire && strpos($id, 'REQEST') !== false && $this->_sdRefreshOnLoad) {
            $this->_redis->expire(self::PREFIX_KEY.$id, min($this->_sdExpire, self::MAX_LIFETIME));
        }

        return $data;
    }

    protected function getSdExpiringLifetime($lifetime, $id) {
        // If it's already truthy, go with it...
        if ($lifetime) {
            return $lifetime;
        }

        // Whitelist approach. We definitely don't want to set a lifetime on FPC_CACHE_SIZE_CAHCE_KEY
        // Not sure if there's anything else?
        if (strpos($id, 'REQEST') === false) {
            return $lifetime;
        }

        // Return our custom expiry if its set
        if ($this->_sdExpire) {
            return $this->_sdExpire;
        }

        // Fine, just return was you had originally...
        return $lifetime;
    }
}

[colinmollenhour]

Thanks for the PR. I appreciate your frustration with the FPC not managing the expiry in a sane way.. I would be open to a PR with a few changes:

• sd_expire should be something like auto_expire_lifetime (int) and auto_expire_pattern (string) which is a PCRE pattern like /^(?!.*REQEST)/ in your case.
• sd_refresh_on_load should be auto_expire_refresh_on_load (bool)

The above isn't set in stone, but it would be meeting your needs I think while providing a generally useful feature. It technically should be a Zend_Cache_Frontend feature but that is obviously never going to happen. :)

[mpchadwick]

Closing as #111 was merged.