Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix 500 errors when using django redis cache for response caching #11849

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions kolibri/core/content/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ def wrapper_func(*args, **kwargs):
# Prevent the Django caching middleware from caching
# this response, as we want to cache it ourselves
request._cache_update_cache = False
key_prefix = get_cache_key(request)
key_prefix = cache_key_func(request)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

surprised by this... until I re-read the PR description

url_key = hashlib.md5(
force_bytes(iri_to_uri(request.build_absolute_uri()))
).hexdigest()
Expand All @@ -129,7 +129,7 @@ def wrapper_func(*args, **kwargs):
response = view_func(*args, **kwargs)
if response.status_code == 200:
if key_prefix is None:
key_prefix = get_cache_key(request)
key_prefix = cache_key_func(request)
if (
key_prefix is not None
and hasattr(response, "render")
Expand Down
13 changes: 13 additions & 0 deletions kolibri/core/utils/cache.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
from django.core.cache import caches
from django.core.cache import InvalidCacheBackendError
from django.utils.functional import SimpleLazyObject
from redis_cache import RedisCache as BaseRedisCache


logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -61,3 +62,15 @@ def save(self):
logger.info("Overwriting Redis config")
self.client.config_rewrite()
self.changed = False


class RedisCache(BaseRedisCache):
def set(self, *args, **kwargs):
"""
Overwrite the set method to not return a value, in line with the Django cache interface
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brilliant!

This causes particular issues for Django's caching middleware, which expects the set method to return None
as it invokes it directly in a lambda in the response.add_post_render_callback method
We use a similar pattern in our own caching decorator in kolibri/core/content/api.py and saw errors
due to the fact if the lambda returns a value, it is interpreted as a replacement for the response object.
"""
super(RedisCache, self).set(*args, **kwargs)
2 changes: 1 addition & 1 deletion kolibri/deployment/default/cache.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@

if cache_options["CACHE_BACKEND"] == "redis":
base_cache = {
"BACKEND": "redis_cache.RedisCache",
"BACKEND": "kolibri.core.utils.cache.RedisCache",
"LOCATION": cache_options["CACHE_LOCATION"],
# Default time out of each cache key
"TIMEOUT": cache_options["CACHE_TIMEOUT"],
Expand Down
Loading