#16108 closed Bug (fixed)
File removal race condition in FileSystemStorage
Reported by: | Aymeric Augustin | Owned by: | nobody |
---|---|---|---|
Component: | File uploads/storage | Version: | 1.3 |
Severity: | Normal | Keywords: | FileSystemStorage file deletion race condition |
Cc: | piethon@… | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
#16082 describes a race condition in directory creation. It was fixed, however, the same race condition still exists in the file removal code, a few lines below.
While this bug is unlikely to show up in production, it's probably worth fixing in the same way, for the sake of consistency and robustness.
Attached patch proposes a fix similar to r16280. Also, it improves slightly the test case added at that revision.
Attachments (1)
Change History (10)
by , 13 years ago
Attachment: | django-file-storage-remove-race.patch added |
---|
comment:1 by , 13 years ago
Triage Stage: | Unreviewed → Ready for checkin |
---|
comment:2 by , 13 years ago
Keywords: | FileSystemStorage file deletion race condition added |
---|
comment:4 by , 13 years ago
UI/UX: | unset |
---|
I know this is fixed in trunk but for what it's worth I have seen this multiple times in production this week. We are using django-compressor for our CSS and JS which delete files in get_available_name
https://github.com/jezdez/django_compressor/blob/develop/compressor/storage.py#L36. When new CSS/JS is deployed there is a race to create the new cached files and this error appears.
comment:5 by , 13 years ago
Thanks for reporting that, would you mind to opening a ticket in the django_compressor issue tracker, too?
comment:6 by , 13 years ago
Can this (and #16082) be included for 1.3.2?
(Is there process to follow to request this, for release management?)
comment:7 by , 13 years ago
Added django_compressor issue: https://github.com/jezdez/django_compressor/issues/159
comment:8 by , 13 years ago
+1 for getting this into 1.3.2. I've also seen this in production with django_compressor.
comment:9 by , 9 years ago
Cc: | added |
---|
Why is the:
if os.path.exists(name):
...line needed?
That line tells me that an exception should not be raised if the file does not exist and yet in the except
clause, an exception is raised if the file was deleted before this thread could do it.
Why the mixed messages and why can't we just have this:
try: os.remove(name) except OSError: pass
?
Looks good! Thank you. :)