[Django] #21602: FileSystemStorage._save() Should Save to a Temporary Filename and Rename to Attempt to be Atomic

[Django]

#21602: FileSystemStorage._save() Should Save to a Temporary Filename and Rename to
Attempt to be Atomic
--------------------------------------+--------------------
Reporter: kevinastone | Owner: nobody
Type: Uncategorized | Status: new
Component: File uploads/storage | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
When using FileSystemStorage based backend with say, staticfiles, creates
a race condition where it's writing the intended destination file which
can lead to race conditions where your web server can serve incomplete
assets while they're being written.

Another case happens with post-processing since CachedStaticFilesStorage
actually deletes the hash-named file before saving over it.

These issue should be circumventing by directing the writing to a
temporary file name and then atomically renaming it to the intended
destination. Even better, FileSystemStorage._save() has to site in a
While-loop because of race conditions in get_available_name(). Instead,
it could defer calling get_available_name until it's prepared to rename
the file to its destination.

I'm working on implementing these changes in my own sub-class of
FileSystemStorage (and CachedStaticFilesStorage to avoid the delete()
call) and would be apply to submit them as a patch if there's interest.

--
Ticket URL: <https://code.djangoproject.com/ticket/21602>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#21602: FileSystemStorage._save() Should Save to a Temporary Filename and Rename to
Attempt to be Atomic

--------------------------------------+------------------------------------

Reporter: kevinastone | Owner: nobody
Type: Uncategorized | Status: new
Component: File uploads/storage | Version: master

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

--------------------------------------+------------------------------------
Changes (by Alex):

* needs_better_patch: => 0
* needs_docs: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted

Comment:

Agreed that atomic rename should be used.

--
Ticket URL: <https://code.djangoproject.com/ticket/21602#comment:1>

[Django]

#21602: FileSystemStorage._save() Should Save to a Temporary Filename and Rename to
Attempt to be Atomic

--------------------------------------+------------------------------------
Reporter: kevinastone | Owner: nobody
Type: Cleanup/optimization | Status: new

Component: File uploads/storage | Version: master

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

--------------------------------------+------------------------------------
Changes (by timo):

* type: Uncategorized => Cleanup/optimization

--
Ticket URL: <https://code.djangoproject.com/ticket/21602#comment:2>

[Django]

#21602: FileSystemStorage._save() Should Save to a Temporary Filename and Rename to
Attempt to be Atomic

--------------------------------------+------------------------------------
Reporter: Kevin Stone | Owner: nobody
Type: Cleanup/optimization | Status: new

Component: File uploads/storage | Version: master

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

--------------------------------------+------------------------------------

Comment (by Tim Graham):

This is also suggested in #27334 (closed as duplicate).

--
Ticket URL: <https://code.djangoproject.com/ticket/21602#comment:3>

[Django]

#21602: FileSystemStorage._save() Should Save to a Temporary Filename and Rename to
Attempt to be Atomic

--------------------------------------+------------------------------------
Reporter: Kevin Stone | Owner: nobody

Type: Cleanup/optimization | Status: new
Component: File uploads/storage | Version: dev

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

--------------------------------------+------------------------------------
Changes (by bcail):

* cc: bcail (added)

Comment:

This should be easier to implement once the
[https://github.com/django/django/blob/main/django/core/files/storage/filesystem.py#L122
OS_OPEN_FLAGS] are removed (they're currently deprecated).
--
Ticket URL: <https://code.djangoproject.com/ticket/21602#comment:4>

[Django]

#21602: FileSystemStorage._save() Should Save to a Temporary Filename and Rename to
Attempt to be Atomic

-------------------------------------+-------------------------------------
Reporter: Kevin Stone | Owner: Jyry
Type: | Suvilehto
Cleanup/optimization | Status: assigned
Component: File | Version: dev
uploads/storage |

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Jyry Suvilehto):

* cc: Jyry Suvilehto (added)
* owner: nobody => Jyry Suvilehto
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/21602#comment:5>

[Django]

#21602: FileSystemStorage._save() Should Save to a Temporary Filename and Rename to
Attempt to be Atomic

-------------------------------------+-------------------------------------
Reporter: Kevin Stone | Owner: Jyry
Type: | Suvilehto
Cleanup/optimization | Status: assigned
Component: File | Version: dev
uploads/storage |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Jyry Suvilehto):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/21602#comment:6>

[Django]

#21602: FileSystemStorage._save() Should Save to a Temporary Filename and Rename to
Attempt to be Atomic

-------------------------------------+-------------------------------------
Reporter: Kevin Stone | Owner: Jyry
Type: | Suvilehto
Cleanup/optimization | Status: assigned
Component: File | Version: dev
uploads/storage |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Comment (by Jyry Suvilehto):

I added a patch [https://github.com/django/django/pull/21308 / here].

Reviewer comments are especially welcome on whether or not this needs new
tests? in theory this change could result in ghost files in a local temp
directory if there are unhandled exceptions.
--
Ticket URL: <https://code.djangoproject.com/ticket/21602#comment:7>

[Django]

#21602: FileSystemStorage._save() Should Save to a Temporary Filename and Rename to
Attempt to be Atomic

-------------------------------------+-------------------------------------
Reporter: Kevin Stone | Owner: Jyry
Type: | Suvilehto
Cleanup/optimization | Status: assigned
Component: File | Version: dev
uploads/storage |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Comment (by Jyry Suvilehto):

Now the system uses atomic `os.rename()` where possible. This lead to
another issue: if two uploads to the same file name `os.rename()` calls
are made almost simultaneously with the same target file name, only the
latter file will exist at the end. The current implementation does not
behave like this. Will it be surprising to the users if in rare cases two
uploads can result in only one file?

We could avoid this by copying the files ourselves and locking, but that
just moves a hacky locking operation from the upload to the copy. Copy
will be a faster operation since it's on the same machine, but still a bit
clunky IMO.
--
Ticket URL: <https://code.djangoproject.com/ticket/21602#comment:8>

[Django]

#21602: FileSystemStorage._save() Should Save to a Temporary Filename and Rename to
Attempt to be Atomic

-------------------------------------+-------------------------------------
Reporter: Kevin Stone | Owner: Jyry
Type: | Suvilehto
Cleanup/optimization | Status: assigned
Component: File | Version: dev
uploads/storage |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by JaeHyuckSa):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/21602#comment:9>