Opened 10 years ago

Closed 10 years ago

Last modified 10 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: dev
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 by Althalus, 10 years ago

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 by Tim Graham, 10 years ago

Needs tests: set
Triage Stage: UnreviewedAccepted

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

comment:3 by Althalus, 10 years ago

Needs tests: unset

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

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

Resolution: fixed
Status: newclosed

In 4529af9ecf7debc9281021462531ec4d9697013b:

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

comment:5 by Tim Graham <timograham@…>, 10 years ago

In 6e248d8f8c95da9cbd654d43df76c20e6d0ec352:

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

Backport of 4529af9ecf from master

comment:6 by Tim Graham <timograham@…>, 10 years ago

In 8568e7cfa4fb87cd0a9ead1b3fbb7a39d19e98b9:

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

Thanks Kyle Owens for the report.

comment:7 by Tim Graham <timograham@…>, 10 years ago

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