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

[dauerbaustelle]

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.

[dauerbaustelle]

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.

[Alex Gaynor]

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).

[Malcolm Tredinnick]

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.

[Jacob]

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.

[andrewfong]

A proposed better patch

[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.

[anonymous]

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.

[Preston Timmons]

The anonymous post was by me, by the way.

[Jacob]

(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.

[Jacob]

Milestone 1.2 deleted