Opened 14 years ago

Closed 4 years ago

Last modified 4 years ago

#13750 closed Bug (fixed)

ImageField accessing height or width and then data results in "I/O operation on closed file".

Reported by: ROsborne Owned by: Hasan Ramezani
Component: Documentation Version: dev
Severity: Normal Keywords:
Cc: Aksel 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 (last modified by Alex Gaynor)

If you have a simple model with an ImageField, the following code will fail with a "I/O operation on closed file":

instance = MyClass.objects.get(...)
w = instance.image.width
h = instance.image.height
original = Image.open(instance.image)

The work around is to reopen the file:

instance = MyClass.objects.get(...)
w = instance.image.width
h = instance.image.height
instance.image.open()
original = Image.open(instance.image)

Note this is different than the issue of the 2nd read() returning empty strings for two reasons:

  1. You can not seek(0), the file is closed.
  1. Needing to reopen in this case is unexpected.

Attachments (7)

regression_13750.diff (1.3 KB ) - added by Aksel 9 years ago.
Test to replicate the behaviour described in the ticket
13750_simple_approach_solution.diff (514 bytes ) - added by Aksel 9 years ago.
Simple approach to solve #13750
13750_tests.txt (34.1 KB ) - added by Aksel 9 years ago.
Results of tests with simple solution
13750_tests_2.txt (3.2 KB ) - added by Aksel 9 years ago.
Updated test results
13750_test_simple_solution.diff (2.1 KB ) - added by Aksel 9 years ago.
Updated changed code
13750.diff (1.8 KB ) - added by Aksel 9 years ago.
Documentation for behavior and updated regression test
13750_new_patch.diff (2.7 KB ) - added by Aksel 9 years ago.
Latest patch

Download all attachments as: .zip

Change History (31)

comment:1 by Alex Gaynor, 14 years ago

Description: modified (diff)

Reformatted description. Next time please use preview and consult WikiFormatting.

comment:2 by Russell Keith-Magee, 14 years ago

Triage Stage: UnreviewedAccepted

comment:3 by Russell Keith-Magee, 14 years ago

#13874 is a different presentation of the same problem.

comment:4 by Julien Phalip, 13 years ago

Severity: Normal
Type: Bug

comment:5 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:6 by Aymeric Augustin, 12 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:7 by Aksel, 9 years ago

Owner: changed from nobody to Aksel
Status: newassigned

by Aksel, 9 years ago

Attachment: regression_13750.diff added

Test to replicate the behaviour described in the ticket

by Aksel, 9 years ago

Simple approach to solve #13750

comment:8 by Aksel, 9 years ago

Would it be a problem to simply open image files (specifically ImageFieldFile objects) in the _get_ function of the FileDescriptor object? Please see the "simple approach" attachment with a potential diff.

Currently, it fixes the new regression test but approximately 31 other tests fail because they test for the "open-ness" of a file. However, these failing tests could be adapted if people agree to this solution. (See 13750_tests.txt)

I'm happy to hear people's suggestions or other feedback.

by Aksel, 9 years ago

Attachment: 13750_tests.txt added

Results of tests with simple solution

comment:9 by Aksel, 9 years ago

Cc: Aksel added

comment:10 by Tim Graham, 9 years ago

How would the tests be adapted? Where is the file closed in the first place?

by Aksel, 9 years ago

Attachment: 13750_tests_2.txt added

Updated test results

by Aksel, 9 years ago

Updated changed code

comment:11 by Aksel, 9 years ago

I've updated the patch code and I've added the updated test results.

Some tests are now failing because they expect the imagefile to be closed (e.g., tests/modelfields/test_imagefield.py line 165, after getting the size of an image).

I'm assuming that, given the description in this ticket, we want to retrieval of the image (and not an image property) to leave the file open. Is that a correct assumption? And if so, isn't that rather inconsistent behaviour?

in reply to:  10 comment:12 by Aksel, 9 years ago

Replying to timgraham:

How would the tests be adapted? Where is the file closed in the first place?

Hi Tim, I've updated my proposed solution code and test results as attachments. I also left some new questions. Lastly, the file get's closed during saving.

comment:13 by Collin Anderson, 9 years ago

Has patch: set

comment:14 by Tim Graham, 9 years ago

Patch needs improvement: set

I don't see a compelling reason to automatically open the file. That change would be backwards incompatible for anyone who is accessing other attributes but doesn't care about the image (they would now have to close it). Can we simply document the behavior in docs/topics/files.txt?

comment:15 by Aksel, 9 years ago

Yeah, while I understand the issue of the ticket reporter, keeping things as they are makes more sense to me because the behaviour would remain consistent. As a next step, I could adapt the regression test to test for an image to be closed and include an entry in docs/topics/files.txt. Unless, of course, someone else does have good reasons to open the file.

by Aksel, 9 years ago

Attachment: 13750.diff added

Documentation for behavior and updated regression test

comment:16 by Aksel, 9 years ago

I updated the regression test to check the behaviour that accessing an image field of a model returns a closed file-like reference. Furthermore, I updated the documentation in docs/topics/files.txt. Changes have an accompanying PR to master. Looking forward to the feedback.


comment:17 by Aksel, 9 years ago

Patch needs improvement: unset

I took another look at this issue and found some other interesting stuff. I noticed that, when not accessing the width or height property, the call to Image.open(model.image) does pass successfully. I may have been looking at the wrong thing.

I dug a little deeper and I found that in django/core/files/images.py the function get_dimensions() has a "close" keyword argument. This keyword argument comes from a call to self.closed in the ImageFile class (a few lines above). In the "get_image_dimensions" function, however, the "close" keyword argument determines whether a file should be closed at the end of the function. I think that was not correct, because if self.closed returns True, the keyword argument to the function is "close=True" and there is a call to file.close() at the end of the function get_image_dimensions.

As a solution, I removed the "close" keyword argument in "get_image_dimensions" and added a variable "close" that is either True or False depending on whether in the beginning of the function the file is explicitly opened.

I also re-added the regression test that caused the "I/O operation on closed file" error. With the new patch this test now passes and so do all the other tests. I updated my PR to the repo and removed the documentation bit of my previous patch.

by Aksel, 9 years ago

Attachment: 13750_new_patch.diff added

Latest patch

comment:18 by Tim Graham, 9 years ago

Patch needs improvement: set

Still not sure the patch is ideal as noted on the pull request. By the way, there's no need to attach the patch to the ticket if you send a pull request.

in reply to:  18 comment:19 by Aksel, 9 years ago

Replying to timgraham:

Still not sure the patch is ideal as noted on the pull request. By the way, there's no need to attach the patch to the ticket if you send a pull request.

Hey Tim,

thanks for the feedback, much appreciated. I'm a bit confused though:

"Can you make this work adding self.assertTrue(profile.image.closed) after this line? I wouldn't expect accessing the image dimensions to leave the file opened."

Isn't that contradicting with the original complaint of the ticket? If I close the file after accessing the height or image the regression test will fail because a closed file will throw the IO error when passed to an Image.open().

I'm not sure which direction to take to resolve this issue correctly.

comment:21 by Mariusz Felisiak, 4 years ago

Component: File uploads/storageDocumentation
Owner: changed from Aksel to Hasan Ramezani
Patch needs improvement: unset
Summary: ImageField accessing height or width and then data results in "I/O operation on closed file"ImageField accessing height or width and then data results in "I/O operation on closed file".
Version: 1.2master

comment:22 by Mariusz Felisiak, 4 years ago

Triage Stage: AcceptedReady for checkin

comment:23 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

Resolution: fixed
Status: assignedclosed

In f57e174f:

Fixed #13750 -- Clarified need to reopen models.ImageField.image file to access raw image data.

comment:24 by Carlton Gibson <carlton.gibson@…>, 4 years ago

In 4037b4c:

[3.0.x] Fixed #13750 -- Clarified need to reopen models.ImageField.image file to access raw image data.

Backport of f57e174fa61e4c31213f6d0033fb9d647b463626 from master

comment:25 by Carlton Gibson <carlton.gibson@…>, 4 years ago

In 4cc1549:

[2.2.x] Fixed #13750 -- Clarified need to reopen models.ImageField.image file to access raw image data.

Backport of f57e174fa61e4c31213f6d0033fb9d647b463626 from master

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