coderedcorp/wagtail-cache

Tested/supported cache backends

vsalvino opened this issue · 11 comments

Built-in backends:

  • Memcached (django.core.cache.backends.memcached.MemcachedCache) - untested
  • Database (django.core.cache.backends.db.DatabaseCache) - tested, working
  • Filesystem (django.core.cache.backends.filebased.FileBasedCache) - tested, working
  • Local memory (django.core.cache.backends.locmem.LocMemCache) - tested, working. But not ideal for production (see Django docs for reasons why)

Other backends:

  • django-redis (django_redis.cache.RedisCache) - tested, working. Added in v0.3.
  • django-redis-cache (redis_cache.RedisCache) - tested, not working. Could be easily implemented similar to django-redis support. Feel free to make a PR or comment if you need it :)

I've been testing django-redis-cache this morning and it has the same behaviour as django-redis, which is that at some point during the first load of an uncached page it returns a True instead of a response object. There's a gist of the debug session here, but here's the salient parts...

> /usr/srv/app/src/wagtail-cache/wagtailcache/cache.py(63)_wrapped_view_func()
     62
---> 63                 return response
     64

--Return--
<TemplateResp...harset=utf-8">

wagtail-cache returns a TemplateResponse object here, but then it all goes wrong here:

> /usr/lib/python3.6/site-packages/django/template/response.py(107)render()
    106             self.content = self.rendered_content
--> 107             for post_callback in self._post_render_callbacks:
    108                 newretval = post_callback(retval)

ipdb> retval
<TemplateResponse status_code=200, "text/html; charset=utf-8">
ipdb> s
> /usr/lib/python3.6/site-packages/django/template/response.py(108)render()
    107             for post_callback in self. _post_render_callbacks:
--> 108                 newretval = post_callback(retval)
    109                 if newretval is not None:

ipdb> s

As we step through the _post_render_callbacks, one of these is the set method of the cache backend...

> /usr/lib/python3.6/site-packages/django/middleware/cache.py(102)<lambda>()
    101                 response.add_post_render_callback(
--> 102                     lambda r: self.cache.set(cache_key, r, timeout)
    103                 )

In this case, it's the set method on BaseRedisCache which rather than sending back the response, sends back True.

> /usr/lib/python3.6/site-packages/redis_cache/backends/base.py(33)wrapped()
     32             key = self.make_key(key, version=version)
---> 33             return method(self, client, key, *args, **kwargs)
     34

ipdb> method
<function BaseRedisCache.set at 0x7faa0a424378>
ipdb> n
--Return--
True

Since this all happens after the request/response has left wagtail-cache, it feels like overriding the UpdateCacheMiddleware might be the only way around it.

More testing on django-redis, it looks like it's the default Redis client behaviour to return true when a cache key is set, so the behaviour between the two Redis options is identical.

Excellent sleuthing! What if we wrote a special cache backend, called wagtail-cache-redis (which would only be for use with this page cache), that simply extends the RedisCache backend and alters the set() method to return the value rather than the result? That might be the path of least resistance, without having to fork or extend any parts of the Django cache middleware.

The DatabaseCache backend can now be marked as working!

I think I solved our redis problem. Is this too hacky? 596014e

Install django-redis, check out my "django-redis" branch of this repo, and then create a separate cache backend using wagtailcache.compat_backends.django_redis.RedisCache :

    'redis': {
        "BACKEND": "wagtailcache.compat_backends.django_redis.RedisCache",
        "LOCATION": "xxx",
        'KEY_PREFIX': 'redis_test',
        "OPTIONS": {
            "CLIENT_CLASS": "django_redis.client.DefaultClient",
            "PASSWORD": 'xxx'
        }
    }

I can confirm that this works, nice work! Doesn't feel too hacky to me, I get why the underlying Redis client returns a boolean at that point, but for our purposes that's kinda useless. This feels like the simplest solution.

Great! Well, if you want to test that out for a little bit and see how it goes, if it appears stable and performant, I can merge this in and get a new release out.

My main concern is that if you are using a redis server for custom caching, and the same redis server for this page cache, clearing the pagecache will also clear the entire server. Not sure how that will work if you create two separate cache aliases/backends with the same redis server LOCATION.

At the moment I've got them separated via the Redis DB they point to...

CACHES = {
    'default': {
        'BACKEND': 'django_redis.cache.RedisCache',
        'LOCATION': redis_host,
        'OPTIONS': {
            "PARSER_CLASS": "redis.connection.HiredisParser",
            'CLIENT_CLASS': 'django_redis.client.DefaultClient',
            'PASSWORD': os.environ.get('REDIS_PASSWORD'),
            "IGNORE_EXCEPTIONS": True,
        }
    },
    'pagecache_redis1': {  # Uses django-redis and hiredis
        "BACKEND": "wagtailcache.compat_backends.django_redis.RedisCache",
        'LOCATION': redis_host,
        'TIMEOUT': 60 * 60 * 24 * 7,  # Seven days
        'OPTIONS': {
            "PARSER_CLASS": "redis.connection.HiredisParser",
            'CLIENT_CLASS': 'django_redis.client.DefaultClient',
            'PASSWORD': os.environ.get('REDIS_PASSWORD'),
            "IGNORE_EXCEPTIONS": True,
        }
    },
}

This seems to work, though I need to test how much strain this puts on Redis. I'm also thinking that having the admin Clear Cache button clear them both might actually be the right strategy from what a content editor would expect.

I'll deploy this to our staging server so we can give it a good test.

Cool. A few other ideas, might want to use a key prefix so you can tell which is which on the server. Or use separate redis locations i.e. /1 and /2.

We could also add before and after hooks in clear_cache() to allow for custom logic. It would be more wagtail-ish.

Can't wait to hear how it works out in your staging environment!

I was thinking last night about how useful before and after hooks for clear_cache() would be. Happy to have a go at doing them.