Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#22845 closed Bug (fixed)

Memcached backend handles infinite (None) timeout defined in default settings improperly

Reported by: Althalus Owned by: nobody
Component: Core (Cache system) Version: master
Severity: Release blocker Keywords: cache, memcached
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

#9595 introduced a way to use None as way to set infinite memcached timeout.
It works fine while you specify this timeout explicitly like cache.set('key', 'val', None).
But it works wrong if you define it as a default cache timeout in django.conf.settings.CACHES.

Consider the following parts:

# base.py
class BaseCache(object):
    def __init__(self, params):
        timeout = params.get('timeout', params.get('TIMEOUT', 300))
        if timeout is not None:
            try:
                timeout = int(timeout)
            except (ValueError, TypeError):
                timeout = 300
        self.default_timeout = timeout
    # memcached.py
    def get_backend_timeout(self, timeout=DEFAULT_TIMEOUT):
        """
        Memcached deals with long (> 30 days) timeouts in a special
        way. Call this function to obtain a safe value for your timeout.
        """
        if timeout == DEFAULT_TIMEOUT:
            return self.default_timeout

        if timeout is None:
            # Using 0 in memcache sets a non-expiring timeout.
            return 0
        elif int(timeout) == 0:
            # Other cache backends treat 0 as set-and-expire. To achieve this
            # in memcache backends, a negative timeout must be passed.
            timeout = -1

        if timeout > 2592000:  # 60*60*24*30, 30 days
            # See http://code.google.com/p/memcached/wiki/FAQ
            # "You can set expire times up to 30 days in the future. After that
            # memcached interprets it as a date, and will expire the item after
            # said date. This is a simple (but obscure) mechanic."
            #
            # This means that we have to switch to absolute timestamps.
            timeout += int(time.time())
        return int(timeout)

As you can see special handling of None, 0 and long timeouts is performed in get_backend_timeout.
But it only handles explicit timeouts. For default timeout there's no special processing while it's needed - timeout is just taken from BaseCache.__init__.

So when trying to set None as default timeout django just transfers it to the memcached driver and which leads to errors.
Also when timeout is set to some high value (more then 30 days) it is not converted to timestamp as memcached requires. In my case it caused all my cache entries to expire immidiately.

Change History (7)

comment:1 Changed 4 years ago by Althalus

Has patch: set
Severity: NormalRelease blocker

In django 1.6 docs there's no information about infinite (None) cache timeout (though code to use it was already delivered).
While in 1.7 it's described properly so I think we can consider it as a new feature. Assuming that this ticket should be a release blocker.
https://docs.djangoproject.com/en/1.7/topics/cache/#cache-arguments.

Also I've made a small and quite straightforward pull request:
https://github.com/django/django/pull/2817

On my current project that patch worked as expected.

comment:2 Changed 4 years ago by Tim Graham

Needs tests: set
Triage Stage: UnreviewedAccepted

[6fe22b30] is the commit that introduced the new behavior. The pull request lacks a test.

comment:3 Changed 4 years ago by Althalus

Needs tests: unset

Added 2 regression tests. All cache tests on my local machine run successfully (debian / python 2.7 / python-memcached).

comment:4 Changed 4 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 4529af9ecf7debc9281021462531ec4d9697013b:

Fixed #22845 -- Correctly handled memcached default timeout value.

comment:5 Changed 4 years ago by Tim Graham <timograham@…>

In 6e248d8f8c95da9cbd654d43df76c20e6d0ec352:

[1.7.x] Fixed #22845 -- Correctly handled memcached default timeout value.

Backport of 4529af9ecf from master

comment:6 Changed 4 years ago by Tim Graham <timograham@…>

In 8568e7cfa4fb87cd0a9ead1b3fbb7a39d19e98b9:

Added backwards incompatibility note for refs #22845; refs #23082.

Thanks Kyle Owens for the report.

comment:7 Changed 4 years ago by Tim Graham <timograham@…>

In 663af270b58f350bcbd9df5cd04ffe69e8ac37c8:

[1.7.x] Added backwards incompatibility note for refs #22845; refs #23082.

Thanks Kyle Owens for the report.

Backport of 8568e7cfa4 from master

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