[Django] #26619: BaseCache incr method will reset the timeout
Django
-------------------------+-------------------------------------------------
Reporter: nitely | Owner: nobody
Type: Bug | Status: new
Component: Core | Version: 1.8
(Cache system) | Keywords: cache, incr, DatabaseCache,
Severity: Normal | BaseCache
Triage Stage: | Has patch: 0
Unreviewed |
Easy pickings: 0 | UI/UX: 0
-------------------------+-------------------------------------------------
`BaseCache.incr()` resets the timeout to the default timeout.
`DatabaseCache` and `FileBasedCache` are affected by this, since they do
not override the method. I'm not sure what the memcache behaviour is.
To reproduce this, set the default cache backend to
`django.core.cache.backends.db.DatabaseCache` and call `incr()` on the
cache. Although,
[https://github.com/django/django/blob/df8d8d4292684d6ffa7474f1e201aed486f02b53/django/core/cache/backends/base.py#L186
the bug is easy to spot] since `.set(...)` is being called without passing
a timeout.
I'm assuming this is a bug since `LocMemCache` doesn't reset the timeout,
the docs don't mention anything about it and it seems like a surprising
behaviour to me.
Django 1.8 and 1.9 are affected by this.
--
Ticket URL: <https://code.djangoproject.com/ticket/26619>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: nitely | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: cache, incr, | Triage Stage:
DatabaseCache, BaseCache | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
I guess a possible resolution would be to add a `timeout` argument to
those methods? I don't think we can increment/ decrement them while
keeping the current timeout?
--
Ticket URL: <https://code.djangoproject.com/ticket/26619#comment:1>
Django
Reporter: nitely | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: cache, incr, | Triage Stage:
DatabaseCache, BaseCache | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by nitely):
Honestly, I'm not sure. Your proposal is consistent with `set()`. However,
memcached and Redis don't set a timeout when incr/decr. I don't know of a
in-memory cache that does that.
The database backend could just update instead of doing a get/set, so it
could keep the timeout (and also be atomic), not sure why it was not
implemented that way. The filecache backend can also leave the timeout
untouched, AFAICT.
If I were to design the cache system from scratch I'd just `raise
NotImplementedError` within the incr/decr of the base backend. But at this
point, I don't know how many third-party libs that would break.
--
Ticket URL: <https://code.djangoproject.com/ticket/26619#comment:2>
Django
Reporter: nitely | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
DatabaseCache, BaseCache |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by timgraham):
* stage: Unreviewed => Accepted
Comment:
Some discussion on the DevelopersMailingList may be useful to decide on
how to proceed.
--
Ticket URL: <https://code.djangoproject.com/ticket/26619#comment:3>
Django
Reporter: Esteban C Borsani | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: cache, incr, | Triage Stage: Accepted
DatabaseCache, BaseCache |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by Simone Macri):
I’ve created a pull request to fix the issue and hope it can be merged
soon, as I’ve currently developed a class extending the original one to
bypass the problem.
--
Ticket URL: <https://code.djangoproject.com/ticket/26619#comment:4>
Django
Reporter: Esteban C Borsani | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: cache, incr, | Triage Stage: Accepted
DatabaseCache, BaseCache |
Changes (by Simone Macri):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/26619#comment:5>
Django
Reporter: Esteban C Borsani | Owner: Simone
| Macri
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: cache, incr, | Triage Stage: Accepted
DatabaseCache, BaseCache |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by Sarah Boyce):
* owner: nobody => Simone Macri
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/26619#comment:6>
Django
Reporter: Esteban C Borsani | Owner: Simone
| Macri
Type: Bug | Status: assigned
Component: Core (Cache system) | Version: 1.8
Severity: Normal | Resolution:
Keywords: cache, incr, | Triage Stage: Accepted
DatabaseCache, BaseCache |
Has patch: 1 | Needs documentation: 0
Changes (by Sarah Boyce):
--
Ticket URL: <https://code.djangoproject.com/ticket/26619#comment:7>