Code

Opened 2 years ago

Closed 2 years ago

Last modified 12 months ago

#17249 closed Bug (needsinfo)

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

Reported by: russellneufeld@… Owned by: nobody
Component: Core (Cache system) Version: 1.3
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

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

Attachments (0)

Change History (5)

comment:1 Changed 2 years ago by ptone

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

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.

comment:2 follow-up: Changed 2 years ago by 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.

comment:3 in reply to: ↑ 2 Changed 2 years ago by ptone

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

comment:4 Changed 2 years ago by lukeplant

  • Resolution set to needsinfo
  • Status changed from new to closed

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.

comment:5 Changed 12 months ago by 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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.