Opened 9 years ago

Closed 6 years ago

Last modified 6 years ago

#6447 closed (fixed)

cache backends that are primarily for local dev should enforce same key restrictions as memcached

Reported by: nicois <nicois.nicois@…> Owned by: Carl Meyer
Component: Core (Cache system) Version: master
Severity: Keywords: memcached sprintSep2010
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Using a locmem:// or postgres cache backend, I can generate a cache key using the following expression:
repr(f.__module__) + repr(f.__name__) + repr(args) + repr(kwargs)
but if I configure memcached, it fails silently, causing an empty page to be sent to the client.

I have confirmed this is the source of the problem by generating an md5.hexdigest of the above key - in which case, memcached works correctly.

I have included the context for the above key, in case it is relevant. It is a decorator method which caches the return value of an arbitrary function, assuming its value depeonds solely on its parameters, and that the method never returns None

from django.core.cache import cache as _djcache
def cache(seconds = 3600):
    def doCache(f):
        def x(*args, **kwargs):
                key = md5(repr(f.__module__) + repr(f.__name__) + repr(args) + repr(kwargs)).hexdigest()
                result = _djcache.get(key)
                if result is None:
                    result = f(*args, **kwargs)
                    _djcache.set(key, result, seconds)
                return result
        return x
    return doCache

Attachments (5)

6447_r13405.diff (7.0 KB) - added by Carl Meyer 6 years ago.
patch to enforce same key restrictions on all cache backends
6447_r13405_withdoc.diff (7.5 KB) - added by Carl Meyer 6 years ago.
same as previous patch, with added doc note re key restrictions
ticket_6447.diff (7.4 KB) - added by Carl Meyer 6 years ago.
updated patch, applies to trunk
ticket_6447_warnings.diff (13.5 KB) - added by Carl Meyer 6 years ago.
updated patch using warnings
ticket_6447_warnings_2.diff (13.4 KB) - added by Carl Meyer 6 years ago.
fixed a couple documentation issues in the previous patch

Download all attachments as: .zip

Change History (20)

comment:1 Changed 9 years ago by john@…

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

The problem is that memcached doesn't allow whitespace in keys. I ran into this a while back, and dealing with it in your code is the right solution. Changing the memcached backend to do it makes examining the cache next to impossible, as Jacob explained while closing #3241 as "wontfix". You could try urllib.quote(key) or re.sub('\s+','',key) if you want to make the keys less opaque (and if you know they'll end up under 250 characters, I suppose).

comment:2 Changed 9 years ago by MichaelBishop

Triage Stage: UnreviewedDesign decision needed

Decision needed. It appears that John has a good point. I'd suggest to either fix the code or add a note to the docs. At the very least I'd suggest adding a note to the docs. I don't feel qualified in this area to attempt to take a shot at adding this to the docs.

comment:3 Changed 8 years ago by Julian Bez

Resolution: duplicate
Status: newclosed

#7460 tackles the issue, too.

comment:4 Changed 8 years ago by wam

Resolution: duplicate
Status: closedreopened

This ticket was apparently closed because #7460 dealt with bad keys and a solution was committed for that ticket. The problem is that this happened only at the template tag interface. Applications that use the low level interface to the cache system still break with bad keys. The documentation for the cache system still does not mention this limitation of the memcache backend, and having inconsistent keying limitations (especially one of such a basic level as whitespace vs no whitespace) in various backends makes it much harder to build reusable apps (a cache key that works for my localmem:// cache may not work with the memcached backend being used on someone elses server).

Ideally, I'd like to see the memcached backend have similar logic applied to it as was used in in #7460 (e.g. use urlquote to ensure keys have no spaces in them) so that all interfaces are protected from spaces. Lacking that, I'd suggest an update to the cache docs that draws attention to memcached's limitation and provides a recommendation to application programmers who are writing applications that use the low-level cache layer to always escape their keys if their app might be used on a server that utilizes memcached.

comment:5 Changed 6 years ago by Carl Meyer

Owner: changed from nobody to Carl Meyer
Status: reopenednew

Following discussion with jacobkm and jezdez in IRC, the plan is to leave the memcached backend unmodified (as any key mangling there could slow down a critical code path), but modify the other builtin backends (locmem, dummy, file, db) so they throw an error (or perhaps just a warning?) if you pass them a key that would be invalid on memcached. This discourages/prevents writing cache code that would not be portable to memcached.

I'm working on a patch.

comment:6 Changed 6 years ago by Carl Meyer

Status: newassigned

Oh, and #13066 was a duplicate of this.

comment:7 Changed 6 years ago by Carl Meyer

Triage Stage: Design decision neededAccepted

Marking as accepted per the above IRC conversation with core committers.

Changed 6 years ago by Carl Meyer

Attachment: 6447_r13405.diff added

patch to enforce same key restrictions on all cache backends

comment:8 Changed 6 years ago by Carl Meyer

Comments on the above patch:

  1. I chose to make an invalid key an exception, rather than a warning. My feeling is cache code ought to be backend-portable, so if it'll blow up on memcached it might as well blow up elsewhere too. I'll change this if core devs prefer it as a warning.
  1. If the idea is that cache-code ought to be backend-portable, is there any reason for the dummy backend to still accept *args, kwargs on most of its methods, where the other backends do not? What's the rationale for keeping this?
  1. As noted before, the design decision here is not to apply any additional key checking to the memcached backend, for speed reasons. The restrictions applied match memcached's restrictions (it appears that some control characters are accepted in keys by memcached itself, but python-memcached applies the same restrictions we apply here), so the result should be the same on all backends, except that memcached will raise a different exception.

Changed 6 years ago by Carl Meyer

Attachment: 6447_r13405_withdoc.diff added

same as previous patch, with added doc note re key restrictions

Changed 6 years ago by Carl Meyer

Attachment: ticket_6447.diff added

updated patch, applies to trunk

comment:9 Changed 6 years ago by Carl Meyer

Keywords: sprintSep2010 added

comment:10 Changed 6 years ago by Carl Meyer

Summary: memcached does not support arbitrary string keyscache backends that are primarily for local dev should enforce same key restrictions as memcached

comment:11 Changed 6 years ago by Malcolm Tredinnick

Since Django supports third-party cache backends, we can't control (thankfully!) what possible external caches people might choose to use in production. So an error here, restricting everybody to memcache's particular limitations, feels inappropriate. A warning -- in it's own class so that it can be easily silenced via warnings.ignore() -- would be my preference. Definitely warning people of importabilities is a good plan. Forcing them to effectively choose memcache for now and forever is bad.

Changed 6 years ago by Carl Meyer

Attachment: ticket_6447_warnings.diff added

updated patch using warnings

comment:12 Changed 6 years ago by Carl Meyer

Added a new patch following design discussion with Russell K-M, Malcolm T, Jannis L, and Jeremy Dunck.

Changed 6 years ago by Carl Meyer

Attachment: ticket_6447_warnings_2.diff added

fixed a couple documentation issues in the previous patch

comment:13 Changed 6 years ago by Carl Meyer

Has patch: set

comment:14 Changed 6 years ago by Malcolm Tredinnick

Resolution: fixed
Status: assignedclosed

(In [13766]) Add warning when using cache keys that might not work with memcached.

This means testing with local dev caches (not memcache) will warn
developers if they are introducing inadvertent importabilities. There is
also the ability to silence the warning if a dev is not planning to use
memcache and knows what they are doing with their keys.

Thanks to Carl Meyer for the patch. Fixed #6447.

comment:15 Changed 6 years ago by Malcolm Tredinnick

(In [13767]) [1.2.X] Add warning when using cache keys that might not work with
memcached.

This means testing with local dev caches (not memcache) will warn
developers if they are introducing inadvertent importabilities. There
is also the ability to silence the warning if a dev is not planning to
use memcache and knows what they are doing with their keys.

Thanks to Carl Meyer for the patch. Fixed #6447.

Backport of r13766 from trunk.

Note: See TracTickets for help on using tickets.
Back to Top