Opened 7 years ago

Closed 6 years ago

Last modified 4 years ago

#8817 closed (fixed)

Accessing ImageField's dimensions doesn't close file

Reported by: tripediac Owned by: mitsuhiko
Component: File uploads/storage Version: master
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: UI/UX:

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 bthomas 6 years ago.
Open a new file handle to read image dimensions
8817_1.diff (1.8 KB) - added by bthomas 6 years ago.
Add tests
8817_2.diff (2.3 KB) - added by Harm Geerts 6 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 mitsuhiko 6 years ago.
proper fix for 8817.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 6 years ago by jacob

  • milestone set to 1.1
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Changed 6 years ago by bthomas

Open a new file handle to read image dimensions

comment:2 Changed 6 years ago by bthomas

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

Changed 6 years ago by bthomas

Add tests

comment:3 Changed 6 years ago by bthomas

  • Cc bthomas@… added
  • Needs tests unset

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

Changed 6 years ago by 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.

comment:4 Changed 6 years ago by bthomas

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 Changed 6 years ago by bthomas

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 Changed 6 years ago by simon29

  • Cc simon@… added

comment:7 Changed 6 years ago by mitsuhiko

  • Triage Stage changed from Accepted to Ready 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.

Changed 6 years ago by mitsuhiko

proper fix for 8817.

comment:8 Changed 6 years ago by mitsuhiko

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

comment:9 Changed 6 years ago by jacob

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

(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 Changed 6 years ago by 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.

comment:11 Changed 6 years ago by kmtracey

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 Changed 6 years ago by mitsuhiko

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 Changed 6 years ago by mitsuhiko

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 follow-up: Changed 6 years ago by 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.

comment:15 in reply to: ↑ 14 Changed 6 years ago by kmtracey

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 Changed 4 years ago by jacob

  • milestone 1.1 deleted

Milestone 1.1 deleted

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