Re: [Django] #13750: ImageField accessing height or width and then data results in "I/O operation on closed file"
Django
operation on closed file"
--------------------------------------+------------------------------------
Reporter: ROsborne | Owner: aethemba
Type: Bug | Status: assigned
Component: File uploads/storage | Version: 1.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Changes (by aethemba):
* owner: nobody => aethemba
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/13750#comment:7>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
operation on closed file"
--------------------------------------+------------------------------------
Reporter: ROsborne | Owner: aethemba
Type: Bug | Status: assigned
Component: File uploads/storage | Version: 1.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Changes (by aethemba):
* Attachment "regression_13750.diff" added.
Test to replicate the behaviour described in the ticket
--
Ticket URL: <https://code.djangoproject.com/ticket/13750>
Django
operation on closed file"
--------------------------------------+------------------------------------
Reporter: ROsborne | Owner: aethemba
Type: Bug | Status: assigned
Component: File uploads/storage | Version: 1.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Changes (by aethemba):
* Attachment "13750_simple_approach_solution.diff" added.
Simple approach to solve #13750
Django
operation on closed file"
--------------------------------------+------------------------------------
Reporter: ROsborne | Owner: aethemba
Type: Bug | Status: assigned
Component: File uploads/storage | Version: 1.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by aethemba):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/13750#comment:8>
Django
operation on closed file"
--------------------------------------+------------------------------------
Reporter: ROsborne | Owner: aethemba
Type: Bug | Status: assigned
Component: File uploads/storage | Version: 1.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
* Attachment "13750_tests.txt" added.
Results of tests with simple solution
--
Ticket URL: <https://code.djangoproject.com/ticket/13750>
Django
operation on closed file"
--------------------------------------+------------------------------------
Reporter: ROsborne | Owner: aethemba
Type: Bug | Status: assigned
Component: File uploads/storage | Version: 1.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Changes (by aethemba):
* cc: aethemba (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/13750#comment:9>
Django
operation on closed file"
--------------------------------------+------------------------------------
Reporter: ROsborne | Owner: aethemba
Type: Bug | Status: assigned
Component: File uploads/storage | Version: 1.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by timgraham):
How would the tests be adapted? Where is the file closed in the first
place?
--
Ticket URL: <https://code.djangoproject.com/ticket/13750#comment:10>
Django
operation on closed file"
--------------------------------------+------------------------------------
Reporter: ROsborne | Owner: aethemba
Type: Bug | Status: assigned
Component: File uploads/storage | Version: 1.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
* Attachment "13750_tests_2.txt" added.
Updated test results
--
Ticket URL: <https://code.djangoproject.com/ticket/13750>
Django
operation on closed file"
--------------------------------------+------------------------------------
Reporter: ROsborne | Owner: aethemba
Type: Bug | Status: assigned
Component: File uploads/storage | Version: 1.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Changes (by aethemba):
* Attachment "13750_test_simple_solution.diff" added.
Updated changed code
Django
operation on closed file"
--------------------------------------+------------------------------------
Reporter: ROsborne | Owner: aethemba
Type: Bug | Status: assigned
Component: File uploads/storage | Version: 1.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by aethemba):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/13750#comment:11>
Django
operation on closed file"
--------------------------------------+------------------------------------
Reporter: ROsborne | Owner: aethemba
Type: Bug | Status: assigned
Component: File uploads/storage | Version: 1.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by aethemba):
Replying to [comment:10 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/13750#comment:12>
Django
operation on closed file"
--------------------------------------+------------------------------------
Reporter: ROsborne | Owner: aethemba
Type: Bug | Status: assigned
Component: File uploads/storage | Version: 1.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/13750#comment:13>
Django
operation on closed file"
--------------------------------------+------------------------------------
Reporter: ROsborne | Owner: aethemba
Type: Bug | Status: assigned
Component: File uploads/storage | Version: 1.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
* needs_better_patch: 0 => 1
Comment:
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/13750#comment:14>
Django
operation on closed file"
--------------------------------------+------------------------------------
Reporter: ROsborne | Owner: aethemba
Type: Bug | Status: assigned
Component: File uploads/storage | Version: 1.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by aethemba):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/13750#comment:15>
Django
operation on closed file"
--------------------------------------+------------------------------------
Reporter: ROsborne | Owner: aethemba
Type: Bug | Status: assigned
Component: File uploads/storage | Version: 1.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
* Attachment "13750.diff" added.
Documentation for behavior and updated regression test
--
Ticket URL: <https://code.djangoproject.com/ticket/13750>
Django
operation on closed file"
--------------------------------------+------------------------------------
Reporter: ROsborne | Owner: aethemba
Type: Bug | Status: assigned
Component: File uploads/storage | Version: 1.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by aethemba):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/13750#comment:16>
Django
operation on closed file"
--------------------------------------+------------------------------------
Reporter: ROsborne | Owner: aethemba
Type: Bug | Status: assigned
Component: File uploads/storage | Version: 1.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
* needs_better_patch: 1 => 0
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/13750#comment:17>
Django
operation on closed file"
--------------------------------------+------------------------------------
Reporter: ROsborne | Owner: aethemba
Type: Bug | Status: assigned
Component: File uploads/storage | Version: 1.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Changes (by aethemba):
* Attachment "13750_new_patch.diff" added.
Latest patch
--
Ticket URL: <https://code.djangoproject.com/ticket/13750>
Django
operation on closed file"
--------------------------------------+------------------------------------
Reporter: ROsborne | Owner: aethemba
Type: Bug | Status: assigned
Component: File uploads/storage | Version: 1.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
* needs_better_patch: 0 => 1
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/13750#comment:18>
Django
operation on closed file"
--------------------------------------+------------------------------------
Reporter: ROsborne | Owner: aethemba
Type: Bug | Status: assigned
Component: File uploads/storage | Version: 1.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by aethemba):
Replying to [comment:18 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/13750#comment:19>
Django
operation on closed file"
--------------------------------------+------------------------------------
Type: Bug | Status: assigned
Component: File uploads/storage | Version: 1.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by Carlton Gibson):
[https://github.com/django/django/pull/11912 Docs PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/13750#comment:20>