jaysonsantos / python-binary-memcached

A pure python module (thread safe) to access memcached via it's binary protocol with SASL auth support.
MIT License
164 stars 57 forks source link

Recursion error when serializing dict containing Django SafeText object #212

Closed hugorodgerbrown closed 4 years ago

hugorodgerbrown commented 4 years ago

x-posted to Django bug tracker: https://code.djangoproject.com/ticket/31140

--

Summary

When attempting to cache a dictionary that contains a SafeString object that itself contains an HTML entity, a RecursionError is raised.

Expected behaviour:

Object is cached.

>>> from django.utils.safestring import SafeString
>>> from django.core.cache import cache
>>> cache.set("test", SafeString("foo"))
>>> cache.set("test", SafeString("£"))

Actual behaviour

Serialization fails with RecursionError: maximum recursion depth exceeded while getting the str of an object

>>> from django.utils.safestring import SafeString
>>> from django.core.cache import cache
>>> cache.set("test", {"foo": SafeString("£")})
Traceback (most recent call last):
  File "/python3.7/site-packages/django/core/cache/backends/memcached.py", line 78, in set
    if not self._cache.set(key, value, self.get_backend_timeout(timeout)):
  File "/python3.7/site-packages/bmemcached/client/replicating.py", line 112, in set
    returns.append(server.set(key, value, time, compress_level=compress_level))
  File "/python3.7/site-packages/bmemcached/protocol.py", line 604, in set
    return self._set_add_replace('set', key, value, time, compress_level=compress_level)
  File "/python3.7/site-packages/bmemcached/protocol.py", line 561, in _set_add_replace
    flags, value = self.serialize(value, compress_level=compress_level)
  File "/python3.7/site-packages/bmemcached/protocol.py", line 347, in serialize
    pickler.dump(value)
  File "/python3.7/copyreg.py", line 66, in _reduce_ex
    state = base(self)
RecursionError: maximum recursion depth exceeded while getting the str of an object

Django cache configuration:

CACHES = {
    "default": {
        "BACKEND": "django_bmemcached.memcached.BMemcached",
        "BINARY": True,
        "OPTIONS": {
            "no_block": True,
            "tcp_nodelay": True,
            "tcp_keepalive": True,
            "remove_failed": 4,
            "retry_timeout": 2,
            "dead_timeout": 10,
            "_poll_timeout": 2000,
        },
    },
}

Running on Django 2.2, Python 3.7.

jaysonsantos commented 4 years ago

Hey there, you could set a pickle_protocol > 1 and this should work.

here where it breaks:

from django.utils.safestring import SafeString
from pickle import dumps

dumps(SafeString('foo'), protocol=1)
---------------------------------------------------------------------------
RecursionError                            Traceback (most recent call last)
<ipython-input-39-f9b20ea78960> in <module>
----> 1 dumps(SafeString('foo'), protocol=1)

/private/tmp/django-test/lib/python3.7/copyreg.py in _reduce_ex(self, proto)
     64         if base is self.__class__:
     65             raise TypeError("can't pickle %s objects" % base.__name__)
---> 66         state = base(self)
     67     args = (self.__class__, base, state)
     68     try:

RecursionError: maximum recursion depth exceeded

but with a newer protocol

dumps(SafeString('foo'), protocol=2)
b'\x80\x02cdjango.utils.safestring\nSafeText\nq\x00X\x03\x00\x00\x00fooq\x01\x85q\x02\x81q\x03.'
jaysonsantos commented 4 years ago

I think that the best approach here would be doing the same thing that pylibmc does on pickle_protocol https://github.com/lericson/pylibmc/blob/52c0a0b62ead820538abd3b72bf60527494374b1/docs/behaviors.rst#non-libmemcached-behaviors

hugorodgerbrown commented 4 years ago

Thanks @jaysonsantos - I'll try that.

BTW - that specific arg is missing from the docstring - https://github.com/jaysonsantos/python-binary-memcached/blob/master/bmemcached/client/mixin.py#L11-L26 - I can add a PR for that if you like?

jaysonsantos commented 4 years ago

Hey @hugorodgerbrown, yes I would gladly merge it :)

hugorodgerbrown commented 4 years ago

Unfortunately I can't get the OPTIONS passed through to the root bmemcached.client object.

In [1]: from django.core.cache import caches
   ...: cache = caches["default"]
   ...:
Initialising django_bmemcached.memcached.BMemcached object; params={'BINARY': True, 'OPTIONS': {'no_block': True, 'tcp_nodelay': True, 'tcp_keepalive': True, 'remove_failed': 4, 'retry_timeout': 2, 'dead_timeout': 10, 'pickle_protocol': 2, '_poll_timeout': 2000}}
Initialising django.core.cache.backends.memcached.BaseMemcachedCache, params={'BINARY': True, 'OPTIONS': {'no_block': True, 'tcp_nodelay': True, 'tcp_keepalive': True, 'remove_failed': 4, 'retry_timeout': 2, 'dead_timeout': 10, 'pickle_protocol': 2, '_poll_timeout': 2000}}
Initialising django.core.cache.backends.base.BaseCache; params={'BINARY': True, 'OPTIONS': {'no_block': True, 'tcp_nodelay': True, 'tcp_keepalive': True, 'remove_failed': 4, 'retry_timeout': 2, 'dead_timeout': 10, 'pickle_protocol': 2, '_poll_timeout': 2000}}

In [2]: cache.get("test")
Initialising bmemcached.client.mixin.ClientMixin; pickle_protocol=0  # <----- pickle_protocol not passed through

In [3]: type(cache)
Out[3]: django_bmemcached.memcached.BMemcached

In [4]: type(cache._cache)
Out[4]: bmemcached.client.replicating.ReplicatingClient

In [5]:
jaysonsantos commented 4 years ago

Hey there, it's been many years I don't use django so this may be wrong. It seems that your OPTIONS is inside params, maybe that is the problem? Here is the code where they send it as kwargs https://github.com/django/django/blob/stable/2.2.x/django/core/cache/backends/memcached.py#L34

jaysonsantos commented 4 years ago

Ah, I found the problem options are not being sent here https://github.com/jaysonsantos/django-bmemcached/blob/master/django_bmemcached/memcached.py#L43

hugorodgerbrown commented 4 years ago

I have submitted a couple of PRs - one for this project (https://github.com/jaysonsantos/python-binary-memcached/pull/213 - updating docstring), and one for the django-bmemcached project to pass the options through. (https://github.com/jaysonsantos/django-bmemcached/pull/14)