Opened 15 years ago

Closed 10 years ago

#12982 closed New feature (fixed)

Add get_or_set method to cache API

Reported by: Alex Gaynor Owned by: Berker Peksag
Component: Core (Cache system) Version: dev
Severity: Normal Keywords: cache
Cc: selwin@…, snow0x2d0@…, mumino Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Add a cache.get_or_set() method to Django's cache abstraction, this takes the same arguments as get() except the 2nd argument is a callable, simple implementation looks like:

def get_or_set(self, key, func, **kwargs):
    val = self.get(key)
    if val is None:
        val = func()
        self.set(key, val)
    return val

Attachments (2)

get_or_set.diff (2.7 KB ) - added by James Snow 14 years ago.
get_or_set2.diff (3.5 KB ) - added by mumino 12 years ago.

Download all attachments as: .zip

Change History (23)

comment:1 by Chris Petrilli, 15 years ago

There's a big race condition in here, unfortunately. If you're using caching (and you should be), and multiple threads access the cache at the same time, then you end up with them over-writing one another. Depending on how this is used, it could be a big issue, or not. If you're talking to memcached, then what you want is add/replace or append/prepend. If you really want to use this approach, then you'll need to use cas, which is "check and set".

I don't believe that these are exposed at this point, unfortunately.

comment:2 by Russell Keith-Magee, 15 years ago

Triage Stage: UnreviewedAccepted

The race condition is a problem, but as Alex noted on IRC, we should be able to avoid this by doing an add() rather than a set().

comment:3 by anonymous, 15 years ago

Cc: selwin@… added

comment:4 by jmeed, 14 years ago

Owner: changed from nobody to jmeed
Status: newassigned

comment:5 by James Snow, 14 years ago

Cc: snow0x2d0@… added

comment:6 by James Snow, 14 years ago

Is work still ongoing on this? If not, I have a patch, docs and tests waiting in the wings.

by James Snow, 14 years ago

Attachment: get_or_set.diff added

comment:7 by James Snow, 14 years ago

Has patch: set

Not sure how else to go about contacting jmeed. I've gone ahead and added my patch, hopefully I'm not stepping on any toes.

comment:8 by Jannis Leidel, 14 years ago

milestone: 1.31.4
Summary: Pony: cache.get_or_set()Add get_or_set method to cache API

Removing 1.3 milestone since we are way over the feature freeze.

comment:9 by Julien Phalip, 14 years ago

Component: Core frameworkCache system

comment:10 by Luke Plant, 14 years ago

Type: New feature

comment:11 by Luke Plant, 14 years ago

Severity: Normal

comment:12 by Preston Holmes, 13 years ago

Easy pickings: unset
Owner: changed from jmeed to James Snow
Status: assignednew
Triage Stage: AcceptedReady for checkin
UI/UX: unset

patch applies to rev 16843
Docs are clear
tests look pretty thorough

comment:13 by Alex Gaynor, 13 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

Moving back to accepted, there are a few issues: 1) in the docs, get_or_set() should show its return values, 2) get_or_set() should *only* take a callable second argument, it's too easy to get the parameters wrong otherwise, 3) it should handle when add() raises an error (which it will if the key is already set).

Thanks for working on this ticket!

comment:14 by Jacob, 13 years ago

milestone: 1.4

Milestone 1.4 deleted

by mumino, 12 years ago

Attachment: get_or_set2.diff added

comment:15 by mumino, 12 years ago

Cc: mumino added

I made a patch based on snow0x2d0's patch.
my proposal is a little different signature for get_or_set:

   cached_data, is_set = cache.get_or_set(key, default, timeout=None, version=None)

maybe we shouldn't force second parameter be callable. I prefer

cache.get_or_set(score, 0}

to

cache.get_or_set(score, lambda: 0}

comment:16 by James Snow, 12 years ago

Wow, I completely missed the updates to this apparently, my apologizes.

I created two topic github branches:

https://github.com/snow0x2d0/django/tree/ticket_12982
https://github.com/snow0x2d0/django/commit/07d441763ce2fbae801cf8fe024aa8ab72e204e9

https://github.com/snow0x2d0/django/tree/ticket_12982_b
https://github.com/snow0x2d0/django/commit/5942333a7994e1a33e050d85da03dfa30ae22b12

They both address two of Alex's concerns. Documentation has been updated to show the return values and an additional condition has been added to catch when add fails if a key exists. From looking over the other backend code it looks like in most cases add() will return False if a key exists already (this is the case for both memcache backends). In the case of database backend an add() with an existing key simply functions like a set().

Personally I don't know that there is likely to be confusion over the order of arguments. The order of parameters is just like the get() function and also follows the function name (namely get, then set). That being the case the primary difference between the two branches above is that the ticket_12982_b branch the second parameter _may_ be callable and in the ticket_12982 branch the second argument _must_ be callable (and a typeerror is thrown if not).

Both branches have been rebased to the current master commit (as of about 6am CST 10-29-12) and all tests pass. I can submit a proper pull request for either once approved.

comment:17 by Berker Peksag, 10 years ago

Owner: changed from James Snow to Berker Peksag
Patch needs improvement: unset
Status: newassigned

comment:18 by Tim Graham, 10 years ago

Patch needs improvement: set

comment:19 by Berker Peksag, 10 years ago

Patch needs improvement: unset

I've just updated the PR.

comment:20 by Tim Graham, 10 years ago

Triage Stage: AcceptedReady for checkin

Looks good, pending a few cosmetic tweaks.

comment:21 by Berker Peksag <berker.peksag@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

In 34fb9091:

Fixed #12982 -- Added a get_or_set() method to the BaseCache backend.

Note: See TracTickets for help on using tickets.
Back to Top