Code

Opened 6 years ago

Closed 6 years ago

#7658 closed (fixed)

Streaming file uploads fail on Windows XP

Reported by: justin.driscoll@… Owned by: axiak
Component: Core (Other) Version: master
Severity: Keywords: 2070-fix
Cc: prigun@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: UI/UX:

Description

When you attempt to upload a file larger than the memory cutoff it returns a WindowsError when it attempts to delete the temporary file which still has a handle from the python process because it is not being closed.

Attachments (4)

file_upload_patch.patch (372 bytes) - added by justin.driscoll@… 6 years ago.
Adds a "close" method to TemporaryUploadedFile.
tempfile.diff (2.9 KB) - added by axiak 6 years ago.
New temp module for Windows support
tempfile.2.diff (1.0 KB) - added by axiak 6 years ago.
Updated tempfile diff to fix AttributeError problem.
tempfile.3.diff (3.0 KB) - added by axiak 6 years ago.
Updated tempfile diff to fix AttributeError problem.

Download all attachments as: .zip

Change History (21)

Changed 6 years ago by justin.driscoll@…

Adds a "close" method to TemporaryUploadedFile.

comment:1 Changed 6 years ago by justin.driscoll@…

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary Streaming file uploads fail on Windows XP deleted

I would also ask the relevant devs to consider adding some exception handling to the file_move_safe function so that even if os.remove(old_file_name) fails the application can continue.

try:

os.remove(old_file_name)

except:

# log the error and continue execution

comment:2 Changed 6 years ago by justin.driscoll@…

  • Needs tests set
  • Summary set to Streaming file uploads fail on Windows XP

Last comment reset the summary, etc. Sorry.

comment:3 Changed 6 years ago by axiak

  • Keywords 2070-fix added
  • Owner changed from nobody to axiak
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

comment:4 Changed 6 years ago by axiak

This should be fixed as of [7859], can you please confirm this?

comment:5 Changed 6 years ago by justin.driscoll@…

Just updated to the latest revs for both trunk and newforms-admin (7870). Still get the error above in newforms admin but in trunk I now get a IOError "No such file or directory". Here's a snippet of the traceback where it fails:

File "C:\home\Projects\Django\libs\django-trunk\django\core\files\move.py" in file_move_safe

  1. file_move(old_file_name, new_file_name)

File "C:\Python25\lib\shutil.py" in move

  1. copy2(src,dst)

File "C:\Python25\lib\shutil.py" in copy2

  1. copyfile(src, dst)

File "C:\Python25\lib\shutil.py" in copyfile

  1. fsrc = open(src, 'rb')

comment:6 Changed 6 years ago by axiak

So it appears there's a reason not to use tempfile.NamedTemporaryFile() on windows. I'm going to create my own NamedTemporaryFile() function for windows.

Changed 6 years ago by axiak

New temp module for Windows support

comment:7 Changed 6 years ago by axiak

  • Triage Stage changed from Accepted to Ready for checkin

Someone else should probably look at this, but I think this fixes the problems with Windows.

comment:8 follow-up: Changed 6 years ago by justin.driscoll@…

Still getting the IOError on streaming uploads after applying the new patch.

comment:9 in reply to: ↑ 8 Changed 6 years ago by axiak

Replying to justin.driscoll@gmail.com:

Still getting the IOError on streaming uploads after applying the new patch.

Yeah. That's bug #7675.

Changed 6 years ago by axiak

Updated tempfile diff to fix AttributeError problem.

Changed 6 years ago by axiak

Updated tempfile diff to fix AttributeError problem.

comment:10 Changed 6 years ago by justin.driscoll@…

Just updated to newforms-admin rev 7888 which from what I can see is inline with trunk and now there appears to be an issue with ImageField and streaming uploads as the PIL check for a valid image fails on streamed file. Should I file this as a new issue?

comment:11 Changed 6 years ago by Alex

Justin, there is already a ticket open for that problem, #7673.

comment:12 Changed 6 years ago by anonymous

Thanks, I ran a search but didn't find anything. Wanted to ask before I filed a dupe.

comment:13 Changed 6 years ago by andrewsk

  • Cc prigun@… added

It looks like close method for the TemporaryUploadFile deletes file on Linux and Mac systems
(didn't have a chance to test it on FreeBSD yet), see #7683

comment:14 Changed 6 years ago by mtredinnick

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

(In [8096]) Fixed #7658 -- Added some Windows-specific tempfile handling. The standard
stuff doesn't work with the way Django's file uploading code wants to operate.
Patch from Mike Axiak.

comment:15 Changed 6 years ago by justin.driscoll@…

  • Resolution fixed deleted
  • Status changed from closed to reopened

As of Django r8301 I am still getting a WindowsError when streaming uploads are used:

[Error 32] The process cannot access the file because it is being used by another process: 'c:
docume~1
jdrisc~1
locals~1
temp
eu5ium.upload'

comment:16 Changed 6 years ago by julien

Same issue is reported in #8203.

comment:17 Changed 6 years ago by julien

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

Re-resolving as 'fixed', since the newly reported issue is a *new* one and most likely the same as in #8203. I consider it's a new issue since the problem is now not with the streaming upload, namely, but within the storage phase.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.