Opened 7 years ago

Closed 2 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@…, jbalogh, 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 7 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 7 years ago by keseldude

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

comment:1 Changed 7 years ago by mtredinnick

  • Needs documentation unset
  • Needs tests unset
  • 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 follow-up: Changed 7 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 7 years ago by mtredinnick

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

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

comment:5 Changed 6 years ago by justinlilly

  • 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 6 years ago by jacob

  • Triage Stage changed from Unreviewed to Design decision needed

comment:7 Changed 6 years ago by jbalogh

  • Cc jbalogh added

comment:8 Changed 4 years ago by otherjacob

  • milestone set to 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 4 years ago by otherjacob

  • Owner changed from nobody to otherjacob

comment:10 Changed 4 years ago by lukeplant

  • Severity set to Normal
  • Type set to New feature

comment:11 Changed 4 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 4 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 4 years ago by simon29

  • Cc simon@… added

comment:14 Changed 4 years ago by ed@…

  • Cc ed@… added
  • UI/UX unset

comment:15 Changed 4 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

comment:16 Changed 3 years ago by dittaeva

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

comment:17 Changed 3 years ago by ejucovy

  • Cc ethan.jucovy@… added

comment:18 Changed 2 years ago by aaugustin

  • Triage Stage changed from Design decision needed to Accepted

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 2 years ago by aaugustin

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

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

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