Opened 8 years ago

Closed 5 years ago

Last modified 5 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: carljm
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 carljm 5 years ago.
patch to enforce same key restrictions on all cache backends
6447_r13405_withdoc.diff (7.5 KB) - added by carljm 5 years ago.
same as previous patch, with added doc note re key restrictions
ticket_6447.diff (7.4 KB) - added by carljm 5 years ago.
updated patch, applies to trunk
ticket_6447_warnings.diff (13.5 KB) - added by carljm 5 years ago.
updated patch using warnings
ticket_6447_warnings_2.diff (13.4 KB) - added by carljm 5 years ago.
fixed a couple documentation issues in the previous patch

Download all attachments as: .zip

Change History (20)

comment:1 Changed 8 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 7 years ago by MichaelBishop

  • Triage Stage changed from Unreviewed to Design 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 7 years ago by julianb

  • Resolution set to duplicate
  • Status changed from new to closed

#7460 tackles the issue, too.

comment:4 Changed 7 years ago by wam

  • Resolution duplicate deleted
  • Status changed from closed to reopened

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 5 years ago by carljm

  • Owner changed from nobody to carljm
  • Status changed from reopened to new

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 5 years ago by carljm

  • Status changed from new to assigned

Oh, and #13066 was a duplicate of this.

comment:7 Changed 5 years ago by carljm

  • Triage Stage changed from Design decision needed to Accepted

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

Changed 5 years ago by carljm

patch to enforce same key restrictions on all cache backends

comment:8 Changed 5 years ago by carljm

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 5 years ago by carljm

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

Changed 5 years ago by carljm

updated patch, applies to trunk

comment:9 Changed 5 years ago by carljm

  • Keywords sprintSep2010 added

comment:10 Changed 5 years ago by carljm

  • Summary changed from memcached does not support arbitrary string keys to cache backends that are primarily for local dev should enforce same key restrictions as memcached

comment:11 Changed 5 years ago by mtredinnick

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 5 years ago by carljm

updated patch using warnings

comment:12 Changed 5 years ago by carljm

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

Changed 5 years ago by carljm

fixed a couple documentation issues in the previous patch

comment:13 Changed 5 years ago by carljm

  • Has patch set

comment:14 Changed 5 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from assigned to closed

(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 5 years ago by mtredinnick

(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