Accessing ImageField's dimensions doesn't close file

[tripediac]

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

[Bob Thomas]

Open a new file handle to read image dimensions

[Bob Thomas]

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.

[Bob Thomas]

Add tests

[Bob Thomas]

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

[Harm Geerts]

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.

[Bob Thomas]

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.

[Bob Thomas]

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.

[Armin Ronacher]

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.

[Armin Ronacher]

proper fix for 8817.

[Jacob]

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

[Jacob]

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

[Karen Tracey]

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.

[Armin Ronacher]

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.

[Armin Ronacher]

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.

[Bob Thomas]

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

[Karen Tracey]

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.

[Jacob]

Milestone 1.1 deleted