From 96ec67a7cf89a136e793305343c5bba8521cdb47 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Przemys=C5=82aw=20Suliga?= Date: Sun, 6 Mar 2016 11:34:23 +0100 Subject: [PATCH] Fixed #26332 -- Fixed a race condition in BaseCache.get_or_set(). --- django/core/cache/backends/base.py | 10 +++++----- docs/releases/1.9.5.txt | 4 ++++ tests/cache/tests.py | 9 ++++++++- 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/django/core/cache/backends/base.py b/django/core/cache/backends/base.py index 75a131ac3a..12351c5bcd 100644 --- a/django/core/cache/backends/base.py +++ b/django/core/cache/backends/base.py @@ -154,8 +154,7 @@ class BaseCache(object): also be any callable. If timeout is given, that timeout will be used for the key; otherwise the default cache timeout will be used. - Returns the value of the key stored or retrieved on success, - False on error. + Return the value of the key stored or retrieved. """ if default is None: raise ValueError('You need to specify a value.') @@ -163,9 +162,10 @@ class BaseCache(object): if val is None: if callable(default): default = default() - val = self.add(key, default, timeout=timeout, version=version) - if val: - return self.get(key, default, version) + self.add(key, default, timeout=timeout, version=version) + # Fetch the value again to avoid a race condition if another caller + # added a value between the first get() and the add() above. + return self.get(key, default, version=version) return val def has_key(self, key, version=None): diff --git a/docs/releases/1.9.5.txt b/docs/releases/1.9.5.txt index 211cbba6ba..d14b1ac98e 100644 --- a/docs/releases/1.9.5.txt +++ b/docs/releases/1.9.5.txt @@ -12,3 +12,7 @@ Bugfixes * Made ``MultiPartParser`` ignore filenames that normalize to an empty string to fix crash in ``MemoryFileUploadHandler`` on specially crafted user input (:ticket:`26325`). + +* Fixed a race condition in ``BaseCache.get_or_set()`` (:ticket:`26332`). It + now returns the ``default`` value instead of ``False`` if there's an error + when trying to add the value to the cache. diff --git a/tests/cache/tests.py b/tests/cache/tests.py index e3ae3d83ce..4a3d4bb128 100644 --- a/tests/cache/tests.py +++ b/tests/cache/tests.py @@ -30,7 +30,7 @@ from django.template import engines from django.template.context_processors import csrf from django.template.response import TemplateResponse from django.test import ( - RequestFactory, SimpleTestCase, TestCase, TransactionTestCase, + RequestFactory, SimpleTestCase, TestCase, TransactionTestCase, mock, override_settings, ) from django.test.signals import setting_changed @@ -931,6 +931,13 @@ class BaseCacheTests(object): self.assertEqual(cache.get_or_set('brian', 1979, version=2), 1979) self.assertIsNone(cache.get('brian', version=3)) + def test_get_or_set_racing(self): + with mock.patch('%s.%s' % (settings.CACHES['default']['BACKEND'], 'add')) as cache_add: + # Simulate cache.add() failing to add a value. In that case, the + # default value should be returned. + cache_add.return_value = False + self.assertEqual(cache.get_or_set('key', 'default'), 'default') + @override_settings(CACHES=caches_setting_for_tests( BACKEND='django.core.cache.backends.db.DatabaseCache',