[Django] #20536: File based cache is not safe for concurrent processes
Django
-------------------------+-------------------------------------------------
Reporter: jaap3 | Owner: nobody
Type: Bug | Status: new
Component: Core | Version: master
(Cache system) | Keywords: filebasedcache concurrency
Severity: Normal | unicodeerror
Triage Stage: | Has patch: 0
Unreviewed | UI/UX: 0
Easy pickings: 0 |
-------------------------+-------------------------------------------------
Django's FileBasedCache is not safe for concurrent processes (as mentioned
in the thread [https://groups.google.com/forum/#!msg/django-
developers/PCgYCInrL0g/Hnr5x2FDNhYJ Is file based cache is safe for
concurrent process?] on django-developers)
I've attached a simple python script that demonstrates the issue
consistently (on my PC) and also provides an alternative implementation of
the `set` method based on the werkzeug's filesystem cache.
I'm wondering if the approach I used to demonstrate the issue is suitable
for use within Django's testsuite and if the tempfile write through
approach is an acceptable solution (it makes writing to the cache slightly
slower).
P.S. "The file based cache is primarily there as a proof of concept"
This is not well documented and I suspect a fair amount of people are
using it in production scenarios.
--
Ticket URL: <https://code.djangoproject.com/ticket/20536>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: jaap3 | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: filebasedcache | Triage Stage:
concurrency unicodeerror | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by gerdemb):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
I've been running something very similar in production for a long time
with no problems. Here's my code:
{{{
def _cull(self):
return
def set(self, key, value, timeout=None, version=None):
key = self.make_key(key, version=version)
self.validate_key(key)
fname = self._key_to_file(key)
dirname = os.path.dirname(fname)
if not os.path.exists(dirname):
try:
os.makedirs(dirname)
except OSError, e:
if e.errno == errno.EEXIST:
pass
else:
raise
tmpfile = tempfile.NamedTemporaryFile(dir=self._dir, delete=False)
if timeout is None:
timeout = self.default_timeout
self._cull()
now = time.time()
pickle.dump(now + timeout, tmpfile, pickle.HIGHEST_PROTOCOL)
pickle.dump(value, tmpfile, pickle.HIGHEST_PROTOCOL)
# tmpfile.flush()
# os.fsync(tmpfile.fileno())
tmpfile.close()
os.rename(tmpfile.name, fname)
}}}
A couple of notes:
1. The only exception I catch is when makedirs() fails due to an already
existing directory. This can happen when two cache keys that point to the
same directory are added at the same time. I have not seen any other
exceptions raised in production.
2. Somewhere I read that to be truly atomic the temporary file must be
flushed and the file system fsynced. I have commented out those two lines
for better performance and have not had any problems, but I am not an
expert here.
3. I moved the cull() function to a management command that I run
periodically with cron. This improves performance, BUT I suspect there
could be conflicts when there are cache writes or reads during a cull
operation. Since I am only culling once a day, my chances of having a
conflict are quite small and I have not seen any errors in production.
--
Ticket URL: <https://code.djangoproject.com/ticket/20536#comment:1>
Django
Reporter: jaap3 | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: filebasedcache | Triage Stage:
concurrency unicodeerror | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by jaap3):
I've created a pull request that should fix this issue
(https://github.com/django/django/pull/1360)
I borrowed some code from django.contrib.sessions.backends.file as that
one has already been made safer.
Haven't done anything about _cull: but if anyone is interested there's
also ticket #15825 (File-based caching doesn't cull truly randomly)
--
Ticket URL: <https://code.djangoproject.com/ticket/20536#comment:2>
Django
Reporter: jaap3 | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: filebasedcache | Triage Stage:
concurrency unicodeerror | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by jaap3):
Created a new patch: https://github.com/django/django/pull/1514 also
addresses #15825
--
Ticket URL: <https://code.djangoproject.com/ticket/20536#comment:3>
Django
Reporter: jaap3 | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
concurrency unicodeerror | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by marfire):
* cc: k@… (added)
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
* needs_tests: 0 => 1
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/20536#comment:4>
Django
Reporter: jaap3 | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
concurrency unicodeerror | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by marfire):
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/20536#comment:5>
Django
Reporter: jaap3 | Owner: jaap3
Type: Bug | Status: assigned
Component: Core (Cache system) | Version: master
Severity: Normal | Resolution:
Keywords: filebasedcache | Triage Stage: Ready for
concurrency unicodeerror | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => jaap3
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/20536#comment:6>
Django
Reporter: jaap3 | Owner: jaap3
Type: Bug | Status: assigned
Component: Core (Cache system) | Version: master
Severity: Normal | Resolution:
concurrency unicodeerror | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by akaariai):
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
The error handling has changed, and I don't see a mention why it has to
change. So, marking patch needs improvement based on that.
In addition I need a confirmation that this is tested on Windows.
Sorry for not dealing with this sooner, we should be doing better with
ready-for-checkin patches.
--
Ticket URL: <https://code.djangoproject.com/ticket/20536#comment:7>
Django
Reporter: jaap3 | Owner: jaap3
Component: Core (Cache system) | Version: master
Keywords: filebasedcache | Triage Stage: Accepted
concurrency unicodeerror | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by Anssi Kääriäinen <akaariai@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"7be638390e18fcbfaaed638f9908673360c280d3"]:
{{{
#!CommitTicketReference repository=""
revision="7be638390e18fcbfaaed638f9908673360c280d3"
Fixed #20536 -- rewrite of the file based cache backend
* Safer for use in multiprocess environments
* Better random culling
* Cache files use less disk space
* Safer delete behavior
Also fixed #15806, fixed #15825.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20536#comment:8>
Django
Reporter: jaap3 | Owner: jaap3
Type: Bug | Status: closed
Component: Core (Cache system) | Version: master
Severity: Normal | Resolution: fixed
Keywords: filebasedcache | Triage Stage: Accepted
concurrency unicodeerror |
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"1aba0e4c68111c99e15d9a4f4d7b4a0ec3c5da01" 1aba0e4c]:
{{{
#!CommitTicketReference repository=""
revision="1aba0e4c68111c99e15d9a4f4d7b4a0ec3c5da01"
Refs #25501 -- Fixed a typo in django/core/cache/backends/filebased.py
The original intent in refs #20536 was to use the highest protocol.
Calling zlib.compress() with a compression level of -1 seems to
fall back to the default level of 6.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20536#comment:9>