Implemented NoDuplicatesStorageProxy, for use in "compress" command

[cuu508]

This is my second attempt to fix #1099.

I added a NoDuplicatesStorageProxy, which delegates all method calls to DefaultStorage, except it filters out duplicate save() calls.

I updated the compress management command to use NoDuplicatesStorageProxy by monkey-patching compressor.storage.default_storage.

The PR includes a test case which exercises the changes.

[diox]

Apologies for not getting back to you sooner on this, I have a shitty month and not much motivation for looking at code outside of work.

This PR works for offline compression, and we could go that route, but maybe we should try to see if we can address the broader issue first. I've written about it in I've commented about the broader issue in #1103 (comment) : the heart of the problem is that compressor really cares about files being named in a predictable way, and to achieve that, it circumvents Django's storage implementation that doesn't let you do that by deleting the file first if it exists.

There are multiple problematic real-world scenarios with this:

• The one we're having in offline compression, where multiple threads are trying to write to the same file at the same time
• "Online" compression where 2 compressor threads try to write to the same file at the same time
• Tests running with multiple processes where 2 processes try to write to the same file at the same time
• "Online" compression where compressor tries to write to an existing file - therefore temporarily deleting it - while the file is being served to the browser by a completely different process (this one is almost impossible to solve for as long as we're deleting the file)

Maybe we could expand the lock idea to compressor core (adding it to the base storage class), to circumvent some of these scenarios ? It's a little scary to add locks to online compression, but maybe that should be ok with a short timeout just in case?

[cuu508]

Maybe we could expand the lock idea to compressor core (adding it to the base storage class), to circumvent some of these scenarios ?

I see django.core.files.storage uses helper functions from django.core.files.locks. It looks like these functions lock access to files at OS level, and so would probably work even with multiple processes. So that's perhaps a piece of the puzzle.

Another idea:

• drop the deletion logic in CompressorFileStorage.get_available_name()
• this means that get_available_name will sometimes return filenames with an underscore and a random string appended at the end to ensure uniqueness
• override the save() method to add one additional last step: if the filename contains an underscore then move it into the correct location.

So, instead of deleting a file and then writing new data in the same filename, compressor would write the data in a temporary file, then move it to the correct place. Python docs say os.replace is an atomic operation, so this would avoid the brief moment of file not existing between delete and write.

[diox]

I like that idea of using os.replace. It seems a lot better than deleting the file if it exists! Compared to your original approaches, it might be a bit wasteful as we're writing the same file multiple times, but that seems fine. And we don't have to care about the potential issues regarding replacing across different filesystems, so it should never fail. In our situation, it carries a lot less risk than locks IMHO.

[diox]

#1107 addressed this differently.