[Django] #29867: cache.get_or_set won't cache None results
Django
-----------------------------------------------+------------------------
Reporter: phill-tornroth | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: 2.1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------------+------------------------
`get_or_set` docstring says "If the key does not exist, add the key and
set it to the default value." -- but that's not quite what it does. It
will perform a set if the key doesn't exist, ''or if the cached value is
None''.
I think in order to be doing what it says on the tin it'd need to be:
{{{
if self.has_key(key, version=version):
return self.get(key, version=version)
else:
if callable(default):
default = default()
if default is not None:
self.add(key, default, timeout=timeout, version=version)
# Fetch the value again to avoid a race condition if another
# caller added a value between the first get() and the add()
# above.
return self.get(key, default, version=version)
}}}
I'd find this useful in cases where None was an expensive result to arrive
at. If there's spiritual alignment with the suggestion, I'm happy to
prepare and submit a change with tests.
--
Ticket URL: <https://code.djangoproject.com/ticket/29867>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+------------------------------------
Reporter: phill-tornroth | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: 2.1
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* stage: Unreviewed => Accepted
Comment:
I agree with your analysis. I read through previous related issues
(#26792, #28601) and didn't see a good reason for the current behavior. I
would question if `if default is not None:` should be there in your
proposal (i.e. why shouldn't `None` by cached?).
--
Ticket URL: <https://code.djangoproject.com/ticket/29867#comment:1>
Django
-------------------------------------+------------------------------------
Type: Bug | Status: new
Component: Core (Cache system) | Version: 2.1
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by Phill Tornroth:
Old description:
> `get_or_set` docstring says "If the key does not exist, add the key and
> set it to the default value." -- but that's not quite what it does. It
> will perform a set if the key doesn't exist, ''or if the cached value is
> None''.
>
> I think in order to be doing what it says on the tin it'd need to be:
>
> {{{
> if self.has_key(key, version=version):
> return self.get(key, version=version)
> else:
> if callable(default):
> default = default()
> if default is not None:
> self.add(key, default, timeout=timeout, version=version)
> # Fetch the value again to avoid a race condition if another
> # caller added a value between the first get() and the add()
> # above.
> return self.get(key, default, version=version)
> }}}
>
> I'd find this useful in cases where None was an expensive result to
> arrive at. If there's spiritual alignment with the suggestion, I'm happy
> to prepare and submit a change with tests.
New description:
`get_or_set` docstring says "If the key does not exist, add the key and
set it to the default value." -- but that's not quite what it does. It
will perform a set if the key doesn't exist, ''or if the cached value is
None''.
I think in order to be doing what it says on the tin it'd need to be:
{{{
if self.has_key(key, version=version):
return self.get(key, version=version)
else:
if callable(default):
default = default()
self.add(key, default, timeout=timeout, version=version)
# Fetch the value again to avoid a race condition if another
# caller added a value between the first get() and the add()
# above.
return self.get(key, default, version=version)
}}}
I'd find this useful in cases where None was an expensive result to arrive
at. If there's spiritual alignment with the suggestion, I'm happy to
prepare and submit a change with tests.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/29867#comment:2>
Django
-------------------------------------+------------------------------------
Type: Bug | Status: new
Component: Core (Cache system) | Version: 2.1
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Phill Tornroth):
Replying to [comment:1 Tim Graham]:
> I agree with your analysis. I read through previous related issues
(#26792, #28601) and didn't see a good reason for the current behavior. I
would question if `if default is not None:` should be there in your
proposal (i.e. why shouldn't `None` by cached?).
You're right, I sketched that out too quickly and shouldn't write code in
bug trackers (edited) :) I'll put a PR up with code that I have some
evidence is working.
--
Ticket URL: <https://code.djangoproject.com/ticket/29867#comment:3>
Django
-------------------------------------+------------------------------------
Type: Bug | Status: new
Component: Core (Cache system) | Version: 2.1
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Phill Tornroth):
So it turns out the issue I'm identifying is very intentional behavior. In
working on a fix I ran into tests that explicitly defend the behavior.
Commit:
https://github.com/django/django/commit/4d60261b2a77460b4c127c3d832518b95e11a0ac
Bug the behavior addresses: https://code.djangoproject.com/ticket/28601
I'm confused by the ticket #28601 though. I agree that the inconsistency
of a callable vs literal default was concerning, but I think I disagree
with the solution to treat None as a unique value. I'd argue that None
ought to be a validly cached value and that in the context of the original
ticket the behavior ought to be:
{{{
cache.get_or_set('foo', None) # None
cache.get_or_set('foo', 5) # None, because we just said so
cache.get('foo') # None :)
cache.get_or_set('bar', lambda: None) # None
cache.get_or_set('bar', 5) # None, because we just said so
cache.get('bar') # None :)
}}}
Want to raise this though in case my take here is controversial. Tim, it
looks like you stewarded that fix in so maybe this link jogs your memory?
Let me know if with this context you disagree with my proposed solution.
If there's still consensus I have a PR just about ready.
--
Ticket URL: <https://code.djangoproject.com/ticket/29867#comment:4>
Django
-------------------------------------+------------------------------------
Type: Bug | Status: new
Component: Core (Cache system) | Version: 2.1
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Phill Tornroth):
Oh apologies, I just noted that you already read through #26801 in
considering this ticket so I'll carry on an put a Pull Request up that
modifies the tests which expect this behavior.
--
Ticket URL: <https://code.djangoproject.com/ticket/29867#comment:5>
Django
-------------------------------------+------------------------------------
Type: Bug | Status: new
Component: Core (Cache system) | Version: 2.1
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Phill Tornroth):
Okay, I put up a pull request here:
https://github.com/django/django/pull/10558
This would be a breaking change (though I'd imagine dependent code would
be rare, and it's much more likely this would improve existing code that
isn't aware it's not benefiting from caching in these cases). Anyone who
was depending on re-calling a callable default that returns None would
need to change their expectation. I'm struggling to imagine what those
cases look like, but in those cases I image they'd be best served by not
using `get_or_set` and instead inspecting the results of `has_key` or
`get` directly and then do something special when `None` is the cached
value.
--
Ticket URL: <https://code.djangoproject.com/ticket/29867#comment:6>
Django
-------------------------------------+------------------------------------
Type: Bug | Status: new
Component: Core (Cache system) | Version: 2.1
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Phill Tornroth):
So this change is more complicated than I thought/hoped as it turns out.
Not all cache backends have a bespoke implementation of `has_key` and the
base cache implementation for `has_key` simply does `return self.get(key,
version=version) is not None`.
The cache code depends pretty severely on `default=None` being the absence
of a default, instead of a normal value. Right now the MemCache and
PyLibMCCache tests are failing as a result (`has_key` returns `False` and
cached `None` is treated as a miss).
Additionally I don't love that introducing `has_key` into the `get_or_set`
logic will introduce an additional cache round trip that wasn't there
before. Ideally the `get` method wouldn't treat `None` as the lack of a
default and we'd have another way to detect a cache miss (`raise
KeyDoesNotExist()` maybe) -- but that's obviously a large change to
introduce that effectively incurs a major version change of the django
cache API contracts, and it's plausible that a number of the backend
libraries will also not provide enough information for their cache
backends to know the difference between a missing key and a None/null
cached value.
So choices that occur to me are:
1/ Maintain current status quo. Update the `get_or_set` documentation to
be more accurate about the implementation.
2/ The cache system is modified somewhat extensively to support `None` as
a cacheable concept (essentially base class functionality and contracts
are changed to assume they can depend on this behavior across cache
backends).
3/ Leave the behavior for `None` up to the individual cache backends, and
consider the current proposed get_or_set implementation for LocMemCache.
I find option 1 fairly compelling, frankly. I was using LocMemCache in a
production situation but most uses are likely to be in testing where
there's broader community benefit for it working similarly to other
caches. Confused users may be more likely to discover caching `None` isn't
likely to work across backends if LocMemCache behaves more similarly to
existing backends.
So I find myself un-recommending my own proposal and updating the
documentation instead (which I'm happy to put a new PR up for if that's
consensus).
--
Ticket URL: <https://code.djangoproject.com/ticket/29867#comment:7>
Django
-------------------------------------+------------------------------------
Type: Bug | Status: new
Component: Core (Cache system) | Version: 2.1
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Nasir Hussain):
Replying to [comment:6 Phill Tornroth]:
Hi Phill,
Are you still working on this one?
--
Ticket URL: <https://code.djangoproject.com/ticket/29867#comment:8>
Django
Reporter: Phill Tornroth | Owner: Ahsan
| Shafiq
Type: Bug | Status: assigned
Component: Core (Cache system) | Version: 2.1
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Ahsan Shafiq):
* owner: nobody => Ahsan Shafiq
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/29867#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Phill Tornroth | Owner: Ahsan
| Shafiq
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Ahsan Shafiq):
I've created a PR against the issue.
https://github.com/django/django/pull/11812
The issue is fixed now.
--
Ticket URL: <https://code.djangoproject.com/ticket/29867#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Phill Tornroth | Owner: Ahsan
| Shafiq
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/11812 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/29867#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Phill Tornroth | Owner: Ahsan
| Shafiq
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 0 => 1
Comment:
I think we should move forward with
[https://github.com/django/django/pull/13671 Nick's PR] which is to handle
`None` in all affected methods. Marking as "needs improvement" because
it's still PoC.
--
Ticket URL: <https://code.djangoproject.com/ticket/29867#comment:12>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: assigned
Component: Core (Cache system) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Nick Pope):
* owner: Ahsan Shafiq => Nick Pope
* needs_better_patch: 1 => 0
* version: 2.1 => master
Comment:
I polished off my [https://github.com/django/django/pull/13671 PR].
This fixes all operations that make use of ``.get()`` without passing a
default argument.
--
Ticket URL: <https://code.djangoproject.com/ticket/29867#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Phill Tornroth | Owner: Nick Pope
Type: Bug | Status: assigned
Component: Core (Cache system) | Version: master
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/29867#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: Phill Tornroth | Owner: Nick Pope
Component: Core (Cache system) | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"bb64b99b78a579cb2f6178011a4cf9366e634438" bb64b99b]:
{{{
#!CommitTicketReference repository=""
revision="bb64b99b78a579cb2f6178011a4cf9366e634438"
Fixed #29867 -- Added support for storing None value in caches.
Many of the cache operations make use of the default argument to the
.get() operation to determine whether the key was found in the cache.
The default value of the default argument is None, so this results in
these operations assuming that None is not stored in the cache when it
actually is. Adding a sentinel object solves this issue.
Unfortunately the unmaintained python-memcached library does not support
a default argument to .get(), so the previous behavior is preserved for
the deprecated MemcachedCache backend.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29867#comment:15>