cache.set() returns None, even in the case of failure

[russellneufeld@…]

The set() method of class BaseMemcachedCache in django.core.cache.backends.memcached does not return the result of the call to self._cache.set(). See this code:

    def set(self, key, value, timeout=0, version=None):
        key = self.make_key(key, version=version)
        self._cache.set(key, value, self._get_memcache_timeout(timeout))

This means that all set() calls appear to work, even if memcached is down, misconfigured, timing out, etc... It would be much better to have this call return the result of _cache.set(), since then you could tell if your set failed and do something appropriate. This caused a very strange bug in my code, since it appeared that I couldn't read back values from memcached despite the appearance of a successful set().

I have patched my code with the following, simply adding "return" to the above code:

def patched_memcached_set(self, key, value, timeout=0, version=None):
    key = self.make_key(key, version=version)
    return self._cache.set(key, value, self._get_memcache_timeout(timeout))

CacheClass.set = patched_memcached_set

Would be nice to have this in the main code line though.

Thanks,

Russ

[Preston Holmes]

None of the cache backends have a return value for set, so it would not be something just for the memcache backend. Not sure there is a universal return value that would make sense. Especially if validating that there was success added any performance hit.

I'm not going to wontfix this myself, because I'm not super familiar with the design goals of the Cache Framework, but my hunch is this is either wontfix, or DDN with a much bigger look at all the backends.

[anonymous]

Validating success in the case of memcache is a no-op. The result is already in the python code. I just added the word "return", so there's no performance impact to this change.

[Preston Holmes]

Replying to anonymous:

Validating success in the case of memcache is a no-op. The result is already in the python code. I just added the word "return", so there's no performance impact to this change.

You're missing the point that code using cache is backend agnostic, so what might be no-op in memcache isn't sufficient alone to consider a change to the general cache API

[Luke Plant]

There are a few problems

Python usually does not use return values to indicate success/failure, but exceptions. From a backwards compatibility point of view, exceptions could be pretty bad, especially since a cache is usually used to increase performance, and if your cache backend goes down, you don't want the whole site to go down.

So, we could probably allow a return value instead of an exception here. However, we need to make sure that all backends can return a success/failure value with no performance hit. Can we do this? (I'm leaving this as a question for someone else to answer, therefore closing NEEDSINFO). For backwards compatibility with existing third-party backends, the return value for success would have to be None.

[andrew@…]

Could this be done with logging a warning, rather than return values or exceptions? That way it has no impact on existing code or API compatibility, but you'll still get a big fat warning rather than a silent fail. If I recall correctly, logging is already used to warn you that your key isn't a valid memcache key, even when you're not using memcache, so there's precedent for an arguably more backend-specific feature using logging.