Opened 7 years ago

Closed 7 years ago

Last modified 3 years ago

#8203 closed (fixed)

File Storage can't delete temporary uploaded files in Windows

Reported by: julien Owned by: ramikassab
Component: File uploads/storage Version: master
Severity: Keywords: aug22sprint
Cc: rajesh.dhawan@…, snaury@…, amiroff@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

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.

Attachments (6)

patch.txt (1.4 KB) - added by Jeethu Rao <jeethu@…> 7 years ago.
patch-#8203.diff (697 bytes) - added by rajeshdhawan 7 years ago.
Revised patch that closes the temporary file only on Windows leaving the behaviour on other OSes unchanged.
django-win32-8203-tests.patch (864 bytes) - added by snaury 7 years ago.
Regression test for patch django-win32-8203.patch
django-win32-8203.patch (5.1 KB) - added by snaury 7 years ago.
Revised patch to take cygwin into account
django-win32-8203.2.patch (4.9 KB) - added by snaury 7 years ago.
Turned out os.unlink emulates file deletion on cygwin, no special case needed
django-win32-8203+samefile+copystat+test.patch (6.5 KB) - added by snaury 7 years ago.
A more thorough implementation that has some necessary bits copied from shutil (plus removed forgotten import sys in django.core.files.temp)

Download all attachments as: .zip

Change History (35)

comment:1 Changed 7 years ago by julien

  • milestone set to 1.0 beta
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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.

comment:2 Changed 7 years ago by mtredinnick

  • milestone changed from 1.0 beta to 1.0

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

comment:3 Changed 7 years ago by julien

Sorry Malcolm, thanks for clearing that up :)

comment:4 Changed 7 years ago by Jeethu Rao <jeethu@…>

  • Has patch set

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.

Changed 7 years ago by Jeethu Rao <jeethu@…>

comment:5 Changed 7 years ago by jacob

  • Component changed from Uncategorized to File uploads/storage
  • Triage Stage changed from Unreviewed to Accepted

comment:6 Changed 7 years ago by julien

  • Needs tests set
  • Patch needs improvement set

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

comment:7 Changed 7 years ago by rajeshdhawan

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

comment:8 Changed 7 years ago by rajeshdhawan

  • Cc rajesh.dhawan@… added

comment:9 Changed 7 years ago by julien

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?

comment:10 Changed 7 years ago by tapautler

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

comment:11 Changed 7 years ago by kmtracey

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

comment:12 Changed 7 years ago by 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.

comment:13 Changed 7 years ago by rajeshdhawan

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.

comment:14 Changed 7 years ago by 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. :)

comment:15 Changed 7 years ago by snaury

  • Cc snaury@… added

Changed 7 years ago by rajeshdhawan

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

comment:16 Changed 7 years ago by rajeshdhawan

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.

Changed 7 years ago by snaury

Regression test for patch django-win32-8203.patch

Changed 7 years ago by snaury

Revised patch to take cygwin into account

Changed 7 years ago by snaury

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

comment:17 Changed 7 years ago by 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.

Changed 7 years ago by snaury

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

comment:18 Changed 7 years ago by 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.

comment:19 Changed 7 years ago by rajeshdhawan

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

comment:20 Changed 7 years ago by amiroff

  • Cc amiroff@… added

comment:21 Changed 7 years ago by 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?

comment:22 Changed 7 years ago by mtredinnick

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.

comment:23 follow-up: Changed 7 years ago by cgrady

  • Keywords aug22sprint added
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready for checkin

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

malcolm approved, changing state :)

comment:24 in reply to: ↑ 23 Changed 7 years ago by ramikassab

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.

comment:25 Changed 7 years ago by ramikassab

  • Owner changed from nobody to ramikassab
  • Status changed from new to assigned

comment:26 Changed 7 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from assigned to closed

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

comment:27 Changed 7 years ago by 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.

comment:28 Changed 7 years ago by 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?

comment:29 Changed 3 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

Note: See TracTickets for help on using tickets.
Back to Top