#20536 closed Bug (fixed)
File based cache is not safe for concurrent processes
Reported by: | Jaap Roes | Owned by: | Jaap Roes |
---|---|---|---|
Component: | Core (Cache system) | Version: | dev |
Severity: | Normal | Keywords: | filebasedcache concurrency unicodeerror |
Cc: | k@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
Django's FileBasedCache is not safe for concurrent processes (as mentioned in the thread 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.
Attachments (1)
Change History (10)
by , 11 years ago
Attachment: | test_filebasedcache.py added |
---|
comment:1 by , 11 years ago
comment:2 by , 11 years ago
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)
comment:3 by , 11 years ago
Created a new patch: https://github.com/django/django/pull/1514 also addresses #15825
comment:4 by , 11 years ago
Cc: | added |
---|---|
Has patch: | set |
Needs tests: | set |
Patch needs improvement: | set |
Triage Stage: | Unreviewed → Accepted |
comment:5 by , 11 years ago
Needs tests: | unset |
---|---|
Patch needs improvement: | unset |
Triage Stage: | Accepted → Ready for checkin |
comment:6 by , 11 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:7 by , 11 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
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.
comment:8 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
I've been running something very similar in production for a long time with no problems. Here's my code:
A couple of notes: