Opened 15 years ago

Closed 10 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)

Changed 15 years ago by keseldude

Attachment: memcached.diff added

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

comment:1 Changed 15 years ago by Malcolm Tredinnick

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 Changed 15 years ago by keseldude

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 Changed 15 years ago by Malcolm Tredinnick

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.

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

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

comment:5 Changed 15 years ago by Justin Lilly

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 Changed 15 years ago by Jacob

Triage Stage: UnreviewedDesign decision needed

comment:7 Changed 14 years ago by Jeff Balogh

Cc: Jeff Balogh added

comment:8 Changed 13 years ago by otherjacob

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 Changed 13 years ago by otherjacob

Owner: changed from nobody to otherjacob

comment:10 Changed 12 years ago by Luke Plant

Severity: Normal
Type: New feature

comment:11 Changed 12 years ago by anonymous

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 Changed 12 years ago by anonymous

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

comment:13 Changed 12 years ago by Simon Litchfield

Cc: simon@… added

comment:14 Changed 12 years ago by ed@…

Cc: ed@… added
UI/UX: unset

comment:15 Changed 12 years ago by Jacob

milestone: 1.4

Milestone 1.4 deleted

comment:16 Changed 11 years ago by Guttorm Flatabø

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

comment:17 Changed 11 years ago by Ethan Jucovy

Cc: ethan.jucovy@… added

comment:18 Changed 11 years ago by Aymeric Augustin

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 Changed 10 years ago by Aymeric Augustin

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 Changed 10 years ago by otherjacob

+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 Changed 10 years ago by Aymeric Augustin <aymeric.augustin@…>

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