Opened 6 years ago

Closed 6 years ago

Last modified 4 years ago

#10646 closed (fixed)

memcached's incr and decr should throw ValueError if keys don't exist

Reported by: dauerbaustelle Owned by: anonymous
Component: Core (Cache system) Version: 1.1-beta
Severity: Keywords: memcached, cache, incr, decr, sprint200912
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

The "incr" and "decr" methods of the "django.core.cache.backends.memcached.CacheClass" should throw a ValueError if the "key" to increase/decrease does not exist.

Right now, two things can happen trying to increase/decrease an invalid key, depending on the python memcached backend you're using: Using "cmemcache", you simply a None as return value. Using "memcache", you'll get some weird ValueErrors telling you that you cannot convert "NOT_FOUND" to an int ("memcache" tries to return the memcache server's return value as int).

See the attached patch for a fix.

Attachments (2)

memcache_incr_decr_require_key.patch (667 bytes) - added by dauerbaustelle 6 years ago.
memcached_valueerrors.patch (1.3 KB) - added by andrewfong 6 years ago.
A proposed better patch

Download all attachments as: .zip

Change History (15)

Changed 6 years ago by dauerbaustelle

comment:1 Changed 6 years ago by dauerbaustelle

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Aargh, some typos. (How can I update the previous post?)

The incr and decr methods of the django.core.cache.backends.memcached.CacheClass should throw a ValueError if the key to increase/decrease does not exist.

Right now, two things can happen trying to increase/decrease an invalid key, depending on the python memcached backend you're using: Using cmemcache, you simply get a None as return value. Using memcache, you'll get some weird ValueErrors telling you that you cannot convert "NOT_FOUND" to an int (memcache tries to return the memcache server's return value as int).

See the attached patch for a fix.

comment:2 Changed 6 years ago by Rob Hudson <treborhudson@…>

  • Component changed from Uncategorized to Cache system
  • Needs tests set
  • Owner changed from nobody to anonymous
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Accepted
  • Version changed from 1.0 to 1.1-beta-1

comment:3 Changed 6 years ago by russellm

  • milestone set to 1.1

comment:4 Changed 6 years ago by Alex

I think we should try to use the results that are already returned, instead of making this operation slower(after all, it's purpose is to be fast and atomic).

comment:5 Changed 6 years ago by mtredinnick

  • Needs documentation set
  • Patch needs improvement set

Agree with making the behaviour uniform (right now, client code would have to handle every possible case in a reusable app, which is unnecessarily error-prone and adding this check can be done without breaking atomicity guarantees). I don't have a firm opinion at the moment whether we should do this with a pre-lookup key check, a la this patch, or a post-lookup handling of the error cases.

  1. This needs tests added to demonstrate the problem and so that we can verify the behaviour is going to be the same across *all* backends. You can't write a single test that tests all backends, but we can run the same test against different settings files. So write a test that increments and decrements non-existent values (there are existing cache tests in tests/regressionstests/cache/) and make sure that they behave the same for all current cache backends.
  2. Also sounds like it might be worth a documentation update to describe the behaviour here.

comment:6 Changed 6 years ago by jacob

  • milestone changed from 1.1 to 1.2

This patch is bad: it defeats the whole purpose of an atomic, single-call incr/decr. The whole point is to make those operations screamingly fast, and doing a get first is just silly.

I agree that we should be consistent -- ValueError is right -- but should be a try/except/error code check for the different backends.

Also: punting to 1.2 since there's an easy workaround.

Changed 6 years ago by andrewfong

A proposed better patch

comment:7 Changed 6 years ago by andrewfong

As of r11267, unit tests for the memcached backend were failing because the test expected a value error to be raised and the incr / decr did not do so. It was bugging me, so I made a quick fix (see attached patch). Hope it helps.

comment:8 Changed 6 years ago by anonymous

  • Needs documentation unset
  • Needs tests unset

After looking at this ticket I think a little clarification would help to explain where it's at:

1) Malcolm noted above the need to write a test case to catch this problem but the existing test cases for test_incr and test_decr already catch this problem. Running the tests with the memcached backend using cmemcache will cause these tests to fail.

See here for where this is tested: http://code.djangoproject.com/browser/django/trunk/tests/regressiontests/cache/tests.py#L183

2) As for documentation, raising a ValueError is documented behavior for the incr/decr operation in the existing documentation and this bug violates this behavior. I don't think extra documentation is necessary.

You can see the documentation here: http://docs.djangoproject.com/en/dev/topics/cache/#topics-cache

3) Jacob's concern has been addressed about the incr/decr operation not being atomic and it is now implemented with try/except blocks after the incr/decr operation.

4) After applying the patch I have verified that the tests pass for both cmemcache and python-memcache.

comment:9 Changed 6 years ago by prestontimmons

The anonymous post was by me, by the way.

comment:10 Changed 6 years ago by jdunck

  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready for checkin

comment:11 Changed 6 years ago by jdunck

  • Keywords sprint200912 added

comment:12 Changed 6 years ago by jacob

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

(In [11855]) Fixed #10646: cache.incr() and decr() now fail consistantly under python-memcache and cmemcache.

Though it looks like this commit has no tests that's not so: this is tested for in regressiontests/cache/tests.py around like 183; these tests currently fail if ran against cmemcache.

Thanks, andrewfong.

comment:13 Changed 4 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

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