Opened 15 years ago

Closed 11 years ago

#9595 closed New feature (fixed)

Add support for cache to not expire

Reported by: keseldude Owned by: otherjacob
Component: Core (Cache system) Version: 1.0
Severity: Normal Keywords: cache timeout
Cc: mkdzmail@…, justinlilly@…, Jeff Balogh, simon@…, ed@…, ethan.jucovy@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

I noticed that for the memcached backend, it is not possible (without modifying the backend code) to actually set the cache timeout value to 0, meaning the cache will not expire. Instead, it sets the timeout value to the 'default_timeout', which is not always preferable. This patch should fix that and make the memcached backend that much more flexible.

The path to the file is django/core/cache/backends/memcached.py.

Attachments (1)

memcached.diff (679 bytes ) - added by keseldude 15 years ago.
Patch that allows the cache timeout to actually be 0, thus allowing it to not expire.

Download all attachments as: .zip

Change History (22)

by keseldude, 15 years ago

Attachment: memcached.diff added

Patch that allows the cache timeout to actually be 0, thus allowing it to not expire.

comment:1 by Malcolm Tredinnick, 15 years ago

Patch needs improvement: set

That patch is backwards-incompatible (not necessarily in a documented way, but in practice), so can't be taken as is (if it was that easy, we would have done it ages ago). There's some argument for allowing "don't expire" in the cache backend, but it needs a different way of specifying it.

comment:2 by keseldude, 15 years ago

At which version [of memcached] does the patch become backwards-incompatible? It might be better to accept it if the version is really old and/or there is no reason to use the older versions.

comment:3 by Malcolm Tredinnick, 15 years ago

It's not about memcached. It's about Django. Existing Django code that passes in 0 for the cache timeout currently behaves in a particular, well-defined way. Your patch changes that behaviour.

in reply to:  2 comment:4 by keseldude, 15 years ago

Ah, I see now. Thanks for the clarification :).

comment:5 by Justin Lilly, 15 years ago

Cc: justinlilly@… added

Forgive my ignorance, but does the current system do something special with a cache timeout of 0? Or is it just "Stash this for 0 second" at which point I question why caches something for 0 seconds?

comment:6 by Jacob, 15 years ago

Triage Stage: UnreviewedDesign decision needed

comment:7 by Jeff Balogh, 14 years ago

Cc: Jeff Balogh added

comment:8 by otherjacob, 13 years ago

milestone: 1.4
Needs documentation: set
Needs tests: set

I'll re-open this for DDN once 1.3 gets out.

My recommendation is a consistent way to set live-forever (or very very long in the case of backends that don't support forever) expirations without setting an expiration as 0, as there may be some cases people would prefer the default python-memcached behavior of essentially deleting the key (although I can't think of any use cases). But I'm game for someone convincing me 0 should reign supreme.

comment:9 by otherjacob, 13 years ago

Owner: changed from nobody to otherjacob

comment:10 by Luke Plant, 13 years ago

Severity: Normal
Type: New feature

comment:11 by anonymous, 13 years ago

Easy pickings: unset

The current behaviour on passing 0 as timeout to cache.set() is:

  • filecache - essentially deletes the key (sets expiry to now, so that the key will be deleted on get)
  • memcached - sets the expiry time to the default timeout
  • locmem - essentially deletes the key (sets expiry time to now).

So the current behaviour isn't consistent across backends anyway. Changing the memcached backend to make 0 = infinite cache doesn't strike me as worse than having memcache have 0 = default timeout, and it means that the useful memcache behaviour of infinite timeout is available.

comment:12 by anonymous, 13 years ago

If we were interested in supporting both cases, -1 makes sense for infinite expiration and 0 for expire now.

comment:13 by Simon Litchfield, 13 years ago

Cc: simon@… added

comment:14 by ed@…, 13 years ago

Cc: ed@… added
UI/UX: unset

comment:15 by Jacob, 13 years ago

milestone: 1.4

Milestone 1.4 deleted

comment:16 by Guttorm Flatabø, 11 years ago

I looked into this issue, and found that there are at least two plugins seeking to address this issue...

comment:17 by Ethan Jucovy, 11 years ago

Cc: ethan.jucovy@… added

comment:18 by Aymeric Augustin, 11 years ago

Triage Stage: Design decision neededAccepted

I'm not sure what design decision is required. At least the inconsistency described in comment 11 must be fixed; either None or a negative value can be used to specify no expiration (I favor None if that can be backwards compatible, since that's intuitive for "no expiration date").

comment:19 by Aymeric Augustin, 11 years ago

Here's the currenty behavior of the cache backends when cache.set() is called with timeout=None explicitly:

  • memcached has def set(..., timeout=0, ...), and then there's this line: timeout = timeout or self.default_timeout
  • locmem/filebased/db have def set(..., timeout=None, ...), and then there's if timeout is None: timeout = self.default_timeout

For all backends except memcached None is a guard value that's intended to be replaced by the default timeout. It isn't intended to be passed explicitly (even though, obviously, that works). I don't know why memcached is implemented differently, but as far as I can tell, the intent is the same.

Docs say that the "timeout argument is optional" and never show an example of passing None.

Therefore, I repeat my support in using None for no expiration. Negative integers feels like C, not Python; and once we define an API we'll be stuck with it for the foreseeable future.

comment:20 by otherjacob, 11 years ago

+1 With aaugustin regarding None

If we want to streamline the use of '0' as "set and then expire immediately," we'll need to add a special case for memcache -- easiest solution would be to pass in -1 into the memcache cache client, as (as noted here) is uses 0 for "forever"

comment:21 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

Resolution: fixed
Status: newclosed

In 89955cc35f3636684ea6f2a6c9504b35a3050f0f:

Fixed #9595 -- Allow non-expiring cache timeouts.

Also, streamline the use of 0 and None between cache backends.

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