[Django] #29867: cache.get_or_set won't cache None results

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

[Django] #29867: cache.get_or_set won't cache None results

Django
#29867: cache.get_or_set won't cache None results
-----------------------------------------------+------------------------
               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.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/057.2c68274dde1c84af4836f591c112a85d%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29867: Allow cache.get_or_set() to cache a None result (was: cache.get_or_set won't cache None results)

Django
#29867: Allow cache.get_or_set() to cache a None result
-------------------------------------+------------------------------------
     Reporter:  phill-tornroth       |                    Owner:  nobody
         Type:  Bug                  |                   Status:  new
    Component:  Core (Cache system)  |                  Version:  2.1
     Severity:  Normal               |               Resolution:
     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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/072.69554de0aa01cf0f8e50b9003395d5d4%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29867: Allow cache.get_or_set() to cache a None result

Django
In reply to this post by Django
#29867: Allow cache.get_or_set() to cache a None result
-------------------------------------+------------------------------------
     Reporter:  Phill Tornroth       |                    Owner:  nobody
         Type:  Bug                  |                   Status:  new
    Component:  Core (Cache system)  |                  Version:  2.1
     Severity:  Normal               |               Resolution:
     Keywords:                       |             Triage Stage:  Accepted
    Has patch:  0                    |      Needs documentation:  0
  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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/072.bb8e7e77e72d64cbf584818411e58bbc%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29867: Allow cache.get_or_set() to cache a None result

Django
In reply to this post by Django
#29867: Allow cache.get_or_set() to cache a None result
-------------------------------------+------------------------------------
     Reporter:  Phill Tornroth       |                    Owner:  nobody
         Type:  Bug                  |                   Status:  new
    Component:  Core (Cache system)  |                  Version:  2.1
     Severity:  Normal               |               Resolution:
     Keywords:                       |             Triage Stage:  Accepted
    Has patch:  0                    |      Needs documentation:  0
  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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/072.a5169039cc95e21d30f7a95600530fc0%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29867: Allow cache.get_or_set() to cache a None result

Django
In reply to this post by Django
#29867: Allow cache.get_or_set() to cache a None result
-------------------------------------+------------------------------------
     Reporter:  Phill Tornroth       |                    Owner:  nobody
         Type:  Bug                  |                   Status:  new
    Component:  Core (Cache system)  |                  Version:  2.1
     Severity:  Normal               |               Resolution:
     Keywords:                       |             Triage Stage:  Accepted
    Has patch:  0                    |      Needs documentation:  0
  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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/072.1590d776566ea0da040868a8a210002e%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29867: Allow cache.get_or_set() to cache a None result

Django
In reply to this post by Django
#29867: Allow cache.get_or_set() to cache a None result
-------------------------------------+------------------------------------
     Reporter:  Phill Tornroth       |                    Owner:  nobody
         Type:  Bug                  |                   Status:  new
    Component:  Core (Cache system)  |                  Version:  2.1
     Severity:  Normal               |               Resolution:
     Keywords:                       |             Triage Stage:  Accepted
    Has patch:  0                    |      Needs documentation:  0
  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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/072.4805c182b79bb96a9417e9b361ee6315%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29867: Allow cache.get_or_set() to cache a None result

Django
In reply to this post by Django
#29867: Allow cache.get_or_set() to cache a None result
-------------------------------------+------------------------------------
     Reporter:  Phill Tornroth       |                    Owner:  nobody
         Type:  Bug                  |                   Status:  new
    Component:  Core (Cache system)  |                  Version:  2.1
     Severity:  Normal               |               Resolution:
     Keywords:                       |             Triage Stage:  Accepted
    Has patch:  0                    |      Needs documentation:  0
  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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/072.5bc0bb43e2aa3831b3e4eea2da2706fc%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29867: Allow cache.get_or_set() to cache a None result

Django
In reply to this post by Django
#29867: Allow cache.get_or_set() to cache a None result
-------------------------------------+------------------------------------
     Reporter:  Phill Tornroth       |                    Owner:  nobody
         Type:  Bug                  |                   Status:  new
    Component:  Core (Cache system)  |                  Version:  2.1
     Severity:  Normal               |               Resolution:
     Keywords:                       |             Triage Stage:  Accepted
    Has patch:  0                    |      Needs documentation:  0
  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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/072.4bab7b31831c7115348e1fc6fd2e4380%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29867: Allow cache.get_or_set() to cache a None result

Django
In reply to this post by Django
#29867: Allow cache.get_or_set() to cache a None result
-------------------------------------+------------------------------------
     Reporter:  Phill Tornroth       |                    Owner:  nobody
         Type:  Bug                  |                   Status:  new
    Component:  Core (Cache system)  |                  Version:  2.1
     Severity:  Normal               |               Resolution:
     Keywords:                       |             Triage Stage:  Accepted
    Has patch:  0                    |      Needs documentation:  0
  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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/072.4a9c43ef5e6ee8bcf936cf981bfa45a9%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29867: Allow cache.get_or_set() to cache a None result

Django
In reply to this post by Django
#29867: Allow cache.get_or_set() to cache a None result
-------------------------------------+-------------------------------------
     Reporter:  Phill Tornroth       |                    Owner:  Ahsan
                                     |  Shafiq
         Type:  Bug                  |                   Status:  assigned
    Component:  Core (Cache system)  |                  Version:  2.1
     Severity:  Normal               |               Resolution:
     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 Ahsan Shafiq):

 * owner:  nobody => Ahsan Shafiq
 * status:  new => assigned


--
Ticket URL: <https://code.djangoproject.com/ticket/29867#comment:9>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/072.235e4adc07ed9a6b012acfbc4fbacc86%40djangoproject.com.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #29867: Allow cache.get_or_set() to cache a None result

Django
In reply to this post by Django
#29867: Allow cache.get_or_set() to cache a None result
-------------------------------------+-------------------------------------
     Reporter:  Phill Tornroth       |                    Owner:  Ahsan
                                     |  Shafiq
         Type:  Bug                  |                   Status:  assigned
    Component:  Core (Cache system)  |                  Version:  2.1
     Severity:  Normal               |               Resolution:
     Keywords:                       |             Triage Stage:  Accepted
    Has patch:  0                    |      Needs documentation:  0
  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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/072.2e4e8eafc2905df2fd0927cb277351f2%40djangoproject.com.