Opened 16 years ago

Closed 15 years ago

Last modified 12 years ago

#8817 closed (fixed)

Accessing ImageField's dimensions doesn't close file

Reported by: tripediac Owned by: Armin Ronacher
Component: File uploads/storage Version: dev
Severity: Keywords:
Cc: bthomas@…, simon@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

If you access an ImageField's width or height property, the corresponding image file doesn't seem to be closed after retrieving its dimensions: After retrieving a lot of image dimensions (of different images), Django produces strange errors ("cannot open template file ..." etc.) which seem to result from too many files being open by the process. Manually calling the close() method of the ImageFieldFile object (after accessing the width and height properties) resolves these errors, so it seems it isn't not called automatically...

Attachments (4)

8817.diff (1.2 KB ) - added by Bob Thomas 15 years ago.
Open a new file handle to read image dimensions
8817_1.diff (1.8 KB ) - added by Bob Thomas 15 years ago.
Add tests
8817_2.diff (2.3 KB ) - added by Harm Geerts 15 years ago.
changed patch to reset the file position before and after reading the dimensions. Without these changes the Model.save() would fail if the file position was non zero, for example by validating the image dimensions in a forms clean method.
8817-final.patch (3.4 KB ) - added by Armin Ronacher 15 years ago.
proper fix for 8817.

Download all attachments as: .zip

Change History (20)

comment:1 by Jacob, 15 years ago

milestone: 1.1
Triage Stage: UnreviewedAccepted

by Bob Thomas, 15 years ago

Attachment: 8817.diff added

Open a new file handle to read image dimensions

comment:2 by Bob Thomas, 15 years ago

Has patch: set
Needs tests: set

I have a possible fix for this. There are several problems here that make a clean solution difficult. The patch I attached solves the problem by opening a new file handle to read the image dimensions, instead of reading from the open file-like object passed to it. This avoids the problem of the FileField.file attribute transparently opening the file, which can't be easily detected inside of get_image_dimensions. A method like get_image_dimensions shouldn't have side effects, but it will alter the file pointer's position by reading from it. This patch prevents any side effects in most cases, but it will still alter a file object that is not based on django.core.files.base.File (this should be fixed, but it's a slightly separate issue).

No unit tests yet, but it passes the existing file_storage tests.

Calling file.close() at the end may not be necessary (the interpreter should clean up when it drops out of scope), but explicit is probably better than implicit here.

by Bob Thomas, 15 years ago

Attachment: 8817_1.diff added

Add tests

comment:3 by Bob Thomas, 15 years ago

Cc: bthomas@… added
Needs tests: unset

Added unit tests. Easier than I thought, thanks to the existing tests for #8534

by Harm Geerts, 15 years ago

Attachment: 8817_2.diff added

changed patch to reset the file position before and after reading the dimensions. Without these changes the Model.save() would fail if the file position was non zero, for example by validating the image dimensions in a forms clean method.

comment:4 by Bob Thomas, 15 years ago

This bug is also causing the file storage regression tests to fail on Windows, because an open file handle will cause the shutil.rmtree(temp_storage_dir) at the end to fail.

comment:5 by Bob Thomas, 15 years ago

I don't think that doing a seek(0) at the end is really the correct behavior. It would still modify the position on the stream if it was nonzero before. Getting image dimensions should have *no* side effects. However, this is really a separate bug from leaving the file handle open, and I think there is already a separate bug for doing a seek(0) before attempting to save.

comment:6 by Simon Litchfield, 15 years ago

Cc: simon@… added

comment:7 by Armin Ronacher, 15 years ago

Triage Stage: AcceptedReady for checkin

Here an updated version with a proper fix, no seek (which is against the common interface of functions accepting both strings and files) and a testcase.

by Armin Ronacher, 15 years ago

Attachment: 8817-final.patch added

proper fix for 8817.

comment:8 by Armin Ronacher, 15 years ago

Owner: changed from nobody to Armin Ronacher
Status: newassigned

comment:9 by Jacob, 15 years ago

Resolution: fixed
Status: assignedclosed

(In [10707]) Fixed #8817: get_image_dimensions correctly closes the files it opens, and leaves open the ones it doesn't. Thanks, mitsuhiko.

While I was at it, I converted the file_storage doctests to unit tests.

comment:10 by Jacob, 15 years ago

(In [10708]) [1.0.X] Fixed #8817: get_image_dimensions correctly closes the files it opens, and leaves open the ones it doesn't. Thanks, mitsuhiko.

While I was at it, I converted the file_storage doctests to unit tests.

Backport of [10707] from trunk.

comment:11 by Karen Tracey, 15 years ago

The fix for this should have fixed #11032 but for some reason it hasn't. Using trunk r10707 on Windows with PIL installed, running the file_storage tests fails due to mug being open, therefore the attempt to delete the temp directory fails. Removing lines 36-43 of regressiontests/file_storage/models.py (the lines that access the dimensions) makes the test pass. Apparently the dimensions are read from an already-open file but the access of them is causing the file to not be subsequently closed? I don't understand what's going on here, and unfortunately I've run out of time to look at this for several hours, so I'll settle for leaving this pesky comment.

comment:12 by Armin Ronacher, 15 years ago

I'm looking into the test failure on windows right now. I have a semi-finished patch for the temporary file that could fix that problem.

comment:13 by Armin Ronacher, 15 years ago

I personally think the problem should better be solved by replacing the custom NamedTemporaryFile with the standard one from the tempfile module. I can't really see what the one in django is used for. The docstring says this:

The temp module provides a NamedTemporaryFile that can be re-opened on any
platform. Most platforms use the standard Python tempfile.TemporaryFile class,
but MS Windows users are given a custom class.

This is needed because in Windows NT, the default implementation of
NamedTemporaryFile uses the O_TEMPORARY flag, and thus cannot be reopened [1].

1: http://mail.python.org/pipermail/python-list/2005-December/359474.html

The point of the temporary file is that it disappears if close() is called. Why is there code doing something like open(f.name, 'r') on a temporary file in django? Maybe that could be refactored that it does not open the file a second time. If not it would probably make sense to open the file using the windows API and by passing all FILE_SHARE_ flags to it and the FILE_ATTRIBUTE_TEMPORARY and FILE_FLAG_DELETE_ON_CLOSE flags. That should give a posix like behavior.

comment:14 by Bob Thomas, 15 years ago

This no longer seems to be an issue after r10737. #11032 is still broken, but for a different reason, and unrelated to this bug.

in reply to:  14 comment:15 by Karen Tracey, 15 years ago

Replying to bthomas:

This no longer seems to be an issue after r10737. #11032 is still broken, but for a different reason, and unrelated to this bug.

Yes, I confirmed r10737 fixed 'mug' being kept open. I've also checked in the fix for #11032 now.

comment:16 by Jacob, 12 years ago

milestone: 1.1

Milestone 1.1 deleted

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