File Storage can't delete temporary uploaded files in Windows

Marking it beta1.0 as it's a quite annoying problem.

Also, I forgot to mention that the file is properly moved to the designated location. It's only the temporary file that is not deleted, generating the exception mentioned in the ticket description.

@julien: "annoying problem" is not a determination of 1.0-beta status. Bugs are 1.0 milestone.

Sorry Malcolm, thanks for clearing that up :)

Here's a patch which ignores WindowsError on Windows. Since the file we're trying to delete is a NamedTemporaryFile, which has an open handle, it'll be deleted anyways when it goes out of scope.

@Jeethu Rao I've tested the patch, but it doesn't work for me...

Explicitly closing the TemporaryUploadedFile? before calling file_move_safe seems to work. Attaching a patch. I've tested successfully on Windows XP Pro SP2. May be others can help test further.

The below potentially infinite loop in django.core.file.storage.FileSystemStorage._save() could use more attention. It assumes that all OSErrors within the block are because of a pre-existing file with the same name as the destination file being created. But there could be other errors like: permission errors in creating the destination file, temporary file move errors, and such. It looks like the except OSError block needs to do some explicit checking of the returned error code but I'm not sure exactly what it should test.

        while True:
            try:
                # This file has a file path that we can move.
                if hasattr(content, 'temporary_file_path'):
                    # On some OSes (Windows, in particular), shutil.move
                    # will choke when unlinking the source file if it's open. 
                    # So close the *possibly* open temporary file. Calling 
                    # close() on an already closed TemporaryUploadedFile file 
                    # is OK too.
                    content.close()
                    file_move_safe(content.temporary_file_path(), full_path)
                    content.close() # Just in case, file_move_safe left it open.

                # This is a normal uploadedfile that we can stream.
                else:
                    # This fun binary flag incantation makes os.open throw an
                    # OSError if the file already exists before we open it.
                    fd = os.open(full_path, os.O_WRONLY | os.O_CREAT | os.O_EXCL | getattr(os, 'O_BINARY', 0))
                    try:
                        locks.lock(fd, locks.LOCK_EX)
                        for chunk in content.chunks():
                            os.write(fd, chunk)
                    finally:
                        locks.unlock(fd)
                        os.close(fd)
            except OSError:
                # Ooops, we need a new file name.
                name = self.get_available_name(name)
                full_path = self.path(name)
            else:
                # OK, the file save worked. Break out of the loop.
                break

Thanks rajeshdhawan for the patch. It seems to work with me on Windows XP too. Do you think you could also write some tests for it, so we make sure it works on other platforms as well?

Tested on Vista. Fixed my looping problem from #8333.

#8450 is another report of upload looping caused by this, I think.

What I find strange is that NamedTemporaryFile? is supposed to delete the file upon closing. :-/ So if you close it before you move it there should be nothing to move. :-/ Yet the patch works. *some time later* Ah! So, you redefine NamedTemporaryFile? on nt in django.core.files.temp, no surprise it works so strangely. What I *must* stress then is that on non-nt systems tempfile.NamedTemporaryFile? will destroy the file upon closing and there would be nothing to move: this patch effectively moves this bug to all the other platforms.

My solution would be to rework django.core.files.move to never rely on shutil.move: it is impossible to know where exactly it failed, it could have race condition with allow_overwrite=False (if after the check the file suddenly materialized then shutil.move will overwrite it). And more importantly, if shutil.move failed then all the code below doesn't make sense: it will fail as well.

Can't prepare the patch right now, but it would be pretty simple: use os.rename only, don't use os.O_CREAT when allow_overwrite and catch OSError around os.remove.

snaury, you're right that this patch screws things up on all other platforms. Your suggested os.rename based code would still not solve the problem because:

1. os.rename breaks when moving across filesystems (as opposed to shutils.move) and 2. the following fragment in storage.FileSystemStorage?._save is still trying to move a file which is open and whose contents may not yet have been flushed to disk. So, I think that os.rename won't be able to move this file while it's in use.

{{

if hasattr(content, 'temporary_file_path'):

file_move_safe(content.temporary_file_path(), full_path) content.close()

}}

I too am looking into this further. I think we need a short term solution as well as a long term one. The short term solution is needed for Windows users because without it a lot of file upload related stuff breaks. Don't know what exactly the long term solution would be.

rajeshdhawan, see the patch I have just attached. shutil.move does not solve anything either, it just uses shutil.copy2, where the only useful part is shutil.copystat. But since file_move_safe is only used by file uploads, stat info doesn't matter, and my patch should make things safer than shutil.move (shutil.move would also try to recursively move directories). Additionally I have patched django.core.files.temp.TemporaryFile? to be more compatible with tempfile._TemporaryFileWrapper implementation. This way .close() will (hopefully) delete the file on all platforms. There is a question about cygwin though, should django.core.files.temp check for os.platform == 'cygwin' as well?

And I hope my solution is the long term one. :)

Revised patch that closes the temporary file only on Windows leaving the behaviour on other OSes unchanged.

snaury, your patch looks comprehensive but I am not an expert in this area to review it. I will try and test it on the Unix and Windows platforms at my disposal. The patch's adoption will probably be expedited if you are able to provide some regression Unit tests.

Meanwhile, I have revised my undoubtedly short-term patch to only do its thing on Windows so it doesn't cause unexpected results on other OSes that you pointed out earlier.

Regression test for patch django-win32-8203.patch

Revised patch to take cygwin into account

Turned out os.unlink emulates file deletion on cygwin, no special case needed

rajeshdhawan, in my humble opinion closing a file before moving it is wrong and unstable, besides it relies on the incorrect behaviour of django.core.files.temp.NamedTemporaryFile? (which my patch fixes as well). shutil.move is just totally unreliable: it has no concept of allow_overwrite, and it might accidentally destroy files, so django should not use it like that. If you worry my implementation is still lacking some bits of shutil I'm attaching a new patch that has _samefile and copyfile copied from shutil.

A more thorough implementation that has some necessary bits copied from shutil (plus removed forgotten import sys in django.core.files.temp)

Additional clarification: Unfortunately I couldn't make a test that will fail on large file upload, so in case of a regression it will just hang until filename grows very large or you run out of disk space.

snaury, your patch is working well for me on Windows.

So (24 hours passed and) what happens now to this bug report?

Do we need to remove flags 'needs tests' and 'patch needs improvement'? Do we have to set owner? Are the right people aware of this bug and patches to review them?

Please exhibit some patience. This is just one of many open bugs on the 1.0 track. As it turns out, some people are looking at this at the moment, but it may still take some days before it is committed -- we don't know yet.

verified issue on windows, patch fixed issue, no harm on linux and osx

malcolm approved, changing state :)

Replying to cgrady:

verified issue on windows, patch fixed issue, no harm on linux and osx malcolm approved, changing state :)

I too can verify this. Tested it on windows with cgrady. Fresh checkout of Django produced the looping problem; however, django-win32-8203+samefile+copystat+test.patch fixed it. Also tested on OS X and Ubuntu both with and without the patch. No adverse effects noticed.

(In [8493]) Fixed #8203 -- Fixed temporary file deleation on Windows and a couple of edge cases on Unix-like systems. Patch from snaury. Testing and verification on Windows, Mac and Linux from cgrady and ramikassab.

I have the newest revision 8620 and still the missbehaviour that the uzploaded file is copied in an endless loop. I am running windowsXP. I dont have any idea what I am doing wrong.

Could you please try running file_uploads test (python runtests.py -v 2 --settings=... file_uploads) and look if it hangs up for you? Also, if the problem is still in file_move_safe, can you try adding some print statements in django/core/files/move.py (before removing old file, after doing it, inside except OSError, etc, in various places) to see where and why exactly is this happening?