Opened 7 years ago

Closed 4 years ago

#29403 closed Cleanup/optimization (needsinfo)

Make PyLibMCCache backend handle TooBig exception from pylibmc

Reported by: SKisContent Owned by: Hasan Ramezani
Component: Core (Cache system) Version: 1.11
Severity: Normal Keywords: memcached, TooBig
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I'm trying to use the memcached backend cache in a project, and I'm get a TooBig error on some requests. The error occurs in django/core/cache/backends/memcached.py on line 86:

        val = self._cache.get(key)
        if val is None:
            return default
        return val
    def set(self, key, value, timeout=DEFAULT_TIMEOUT, version=None):
        key = self.make_key(key, version=version)
**        if not self._cache.set(key, value, self.get_backend_timeout(timeout)):**
            # make sure the key doesn't keep its old value in case of failure to set (memcached's 1MB limit)
            self._cache.delete(key)
    def delete(self, key, version=None):
        key = self.make_key(key, version=version)
        self._cache.delete(key)

I can reproduce it using the Django shell:

$ ./manage.py shell
Python 2.7.12 (default, Nov 19 2016, 06:48:10) 
[GCC 5.4.0 20160609] on linux2
Type "help", "copyright", "credits" or "license" for more information.
(InteractiveConsole)
>>> from django.core.cache import cache
>>> cache.get('foo')
>>> cache.set('foo', 'bar')
>>> cache.get('foo')
'bar'
>>> big = [i for i in range(1024*1024)]
>>> cache.set('big_foo', big)
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/local/lib/python2.7/site-packages/django/core/cache/backends/memcached.py", line 86, in set
    if not self._cache.set(key, value, self.get_backend_timeout(timeout)):
TooBig

I can replicate the same behavior in pylibmc:

$ python
Python 2.7.12 (default, Nov 19 2016, 06:48:10) 
[GCC 5.4.0 20160609] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from pylibmc.test import make_test_client
>>> mc = make_test_client()
>>> mc.get('foo')
>>> mc.set('foo', 'bar')
True
>>> mc.get('foo')
'bar'
>>> big = [i for i in range(1024*1024)]
>>> mc.set('big_foo', big)
Traceback (most recent call last):
  File "<console>", line 1, in <module>
TooBig

The TooBig exception was added to pylibmc in 2016: https://github.com/lericson/pylibmc/commit/736e21276ad04d557b68bd81b7f28be1a4b4e1ec

The uncaught error breaks the application. Looking at the comment about the 1MB size limit on the next line (line 87), it seems like the original intent was to unset the value and fail silently. Therefore, the if block needs to be wrapped in try...except.

Change History (5)

comment:1 by SKisContent, 7 years ago

To clarify, I'm using PyLibMCCache:

CACHES = {
    'default': {
        'BACKEND': 'django.core.cache.backends.memcached.PyLibMCCache',
        'LOCATION': '127.0.0.1:11211',
        'TIMEOUT': 1000,
        'BINARY': True,
        'OPTIONS': {
            'tcp_nodelay': True,
            'remove_failed': 4
        }
    }
}

comment:2 by Tim Graham, 7 years ago

Summary: TooBig error in memcachedMake PyLibMCCache backend handle TooBig exception from pylibmc
Triage Stage: UnreviewedAccepted

comment:3 by Flavio Curella, 5 years ago

I'm not convinced we should fail silently. I understand that some versions o memcached will, but I'd rather be warn the user that something went wrong instead of being consistent.

If we know something went wrong we should tell the user. I'd recommend raising a more descriptive exception, possibly using raise ... from ....

comment:4 by Hasan Ramezani, 4 years ago

Has patch: set
Owner: changed from nobody to Hasan Ramezani
Status: newassigned
Version 0, edited 4 years ago by Hasan Ramezani (next)

comment:5 by Mariusz Felisiak, 4 years ago

Resolution: needsinfo
Status: assignedclosed
Type: BugCleanup/optimization

Like Flavio Curella and Ed Morley, I'm not convinced that we should catch this exception, it was shortly discussed in #19914. See also arguments in #28342 (this is not a duplicate but is related). The main question is: should we force consistency between these two backends? IMO it's better to respect their different policies of handling errors.

I think we should go back to the DevelopersMailingList and reach a consensus to move it forward.

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