Image.close() should not close an externally provided file pointer (when exclusive_fp is False)

[andreymal]

When using context manager, Image does not close file pointer, as expected. It checks getattr(self, "_exclusive_fp", False) and closes the file pointer only if it's True.

with open("file.png", "rb") as fp:
    assert fp.closed is False
    with Image.open(fp) as im:
        ...
    assert fp.closed is False  # OK

But the Image.close() method misses _exclusive_fp check and closes the file pointer unexpectedly:

with open("test.png", "rb") as fp:
    assert fp.closed is False
    im = Image.open(fp)
    ...
    im.close()
    assert fp.closed is False  # AssertionError

[radarhere]

I would say this is the documented behaviour. https://pillow.readthedocs.io/en/stable/reference/Image.html#PIL.Image.Image.close

Closes the file pointer, if possible.

Or are you trying to say that you think the behaviour should be changed, even so? Do you think there should be a method at all to close the file pointer that Pillow has been passed?

[andreymal]

Or are you trying to say that you think the behaviour should be changed, even so?

Yeah, I think. From my point of view, "if possible" means if self._exclusive_fp is True :)

It will be consistent with many other libraries that work with file-like objects (zipfile, olefile, lxml etc.)

[andreymal]

Do you think there should be a method at all to close the file pointer that Pillow has been passed?

As documentation of some libraries say, "it is the caller's responsibility to close the file object", and I agree with them, Pillow shouldn't care about it.

On the other hand, it may be a backward incompatible change if there are some users who rely on the current behavior of Image.close() (But I hope there are none...)

[radarhere]

Pillow does value backwards compatibility. For context, close() has been part of Pillow since 2.5.0, and the idea of exclusive file pointers was only added afterwards, in 4.1.0.

In 6.1.0 we deprecated the idea of implicitly closing file pointers, and in 7.0.0, we removed support for it, suggesting to users that they either use context managers or the close() method.

[radarhere]

It will be consistent with many other libraries that work with file-like objects (zipfile, olefile, lxml etc.)

I can't find how this applies to lxml.

Yes, zipfile works as you describe. "it is the caller's responsibility to close the file object" comes from the wave documentation, which also doesn't close the file pointer.

import zipfile
with open("demo.zip", "rb") as fp:
    myzip = zipfile.ZipFile(fp)
    myzip.close()
    assert fp.closed is False

However, olefile does not - it works the same as Pillow does.

import olefile
with open("input_bw_five_bands.fpx", "rb") as fp:
    ole = olefile.OleFileIO(fp)
    ole.close()
    assert fp.closed is True

[andreymal]

However, olefile does not - it works the same as Pillow does.

@radarhere Install olefile from github https://github.com/decalage2/olefile

See https://github.com/decalage2/olefile/tree/cc0bdc07/olefile/olefile.py#L1406-L1412

[radarhere]

Ah, there hasn't been a release since https://github.com/decalage2/olefile/pull/121. Interesting

[aclark4life]

@andreymal @radarhere @delta1513 So we have a request for fix, a PR to implement, and some confusion on our end about whether or not we should support this change. In such cases, @wiredfool typically has strong opinions about whether or not something is a good idea. If he says "this is a good idea and we should do it" then we'd probably move forward. If he says "no" then we probably won't do it. If he says anything but yes or no, then it'll probably remain in limbo for lack of any additional forces to push it through.

Another approach would be to rely on testing. If we can make the claim "this doesn't break our tests" and "We're pretty sure it won't break other peoples' code" then we can move forward in the interest of supporting community-driven changes however big or small when we are confident about the implications. Many-a-small-improvements are done this way.

Lastly, there is the "My way, 2010-2013" approach. When this project was small and someone told me something was a good idea and I had no reason not to believe them, I would merge now and ask questions later. This looks like a potential candidate for that approach. 🤔

[radarhere]

@jdufresne as the author of #3577, which introduced the 6.1.0 change, did you have any thoughts on this?

[jdufresne]

Thanks for asking, but I no longer work on a project that uses Pillow.

[radarhere]

Another approach would be to rely on testing. If we can make the claim "this doesn't break our tests" and "We're pretty sure it won't break other peoples' code" then we can move forward in the interest of supporting community-driven changes

Implementing this could mean that file pointers are not closed at the point that our users have come to expect, so I don't think we can make this claim.

[radarhere]

In the absence of other opinions, I'm going to close this in favour of backwards compatibility.

[andreymal]

^{(no one has even upvoted this issue, am I the only one in the world for whom the current behavior is causing troubles?)}