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

[Kevin Stone]

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.

[Alex Gaynor]

Agreed that atomic rename should be used.

[Tim Graham]

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

[bcail]

This should be easier to implement once the ​OS_OPEN_FLAGS are removed (they're currently deprecated).

[Jyry Suvilehto]

I added a patch ​/ 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.

[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.