File Storage can't delete temporary uploaded files in Windows

[Julien Phalip]

In windows (at least in XP, AFAICT), the file storage system can't delete temporary uploaded files as an exception is raised:

Traceback (most recent call last):

  File "E:\Software\workspace\django\django\core\handlers\base.py", line 86, in get_response
    response = callback(request, *callback_args, **callback_kwargs)

  File "E:/Software/workspace/myproject/trunk/site/apps\projects\ajax_views\file_views.py", line 33, in upload_file
    return files_ajax_views.upload_file(request, project.get_file_gallery())

  File "E:/Software/workspace/myproject/trunk/site/apps\files\ajax_views.py", line 41, in upload_file
    file.get_behaviour().save(uploaded_file)

  File "E:/Software/workspace/myproject/trunk/site/apps\files\behaviours.py", line 90, in save
    self.__class__.storage.save(self.get_filename(), uploaded_file)

  File "E:\Software\workspace\django\django\core\files\storage.py", line 57, in save
    self._save(name, content)

  File "E:\Software\workspace\django\django\core\files\storage.py", line 154, in _save
    file_move_safe(content.temporary_file_path(), full_path)

  File "E:\Software\workspace\django\django\core\files\move.py", line 59, in file_move_safe
    os.remove(old_file_name)

WindowsError: [Error 32] The process cannot access the file because it is being used by another process: 'E:\\temp_django_files\\zqix8a.upload'

In the traceback above, if you wonder what self.__class__.storage is, it actually refers to a FileSystemStorage instance:

storage = FileSystemStorage(location='/bla')

E:\\temp_django_files is the custom folder defined with FILE_UPLOAD_TEMP_DIR

It works fine, though, for small files that go under the FILE_UPLOAD_MAX_MEMORY_SIZE limit.

[Julien Phalip]

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.

[Malcolm Tredinnick]

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

[Julien Phalip]

Sorry Malcolm, thanks for clearing that up :)

[Jeethu Rao <jeethu@…>]

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.

[Julien Phalip]

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

[Rajesh Dhawan]

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

[Julien Phalip]

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?

[tapautler]

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

[Karen Tracey]

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

[snaury]

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.

[Rajesh Dhawan]

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.

[snaury]

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. :)

[Rajesh Dhawan]

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

[Rajesh Dhawan]

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.

[snaury]

Regression test for patch django-win32-8203.patch

[snaury]

Revised patch to take cygwin into account

[snaury]

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

[snaury]

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.

[snaury]

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

[snaury]

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.

[Rajesh Dhawan]

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

[snaury]

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?

[Malcolm Tredinnick]

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.

[Collin Grady]

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

malcolm approved, changing state :)

[Rami Kassab]

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.

[Malcolm Tredinnick]

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

[Toni]

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.

[snaury]

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?

[Jacob]

Milestone 1.0 deleted