Add TextIOWrapper fileobj to list of fds to close in get_readable_fileobj - #8675

I can confirm that this fixes the warning for me.

Good catch, @taldcroft ; need a change log as this is labeled a bug! Are the mpldev failures related? I don't think so but I wanna double check.

@bsipocz , I'll let you milestone it. Thinking either 3.2 or 3.2.1 but no point porting to 2.x series.

I milestoned this for 3.2.1. It's not really a critical bug and we do need to have someone that actually understands the code have a close look to be sure this fix does not have unintended consequences.

Looking at this more, I am not so sure. So, it turns out your use case was a nested get_readable_fileobj with the inner call depends on the fileobj of the outer call, correct?

Before this patch:

>>> with get_readable_fileobj(filename, encoding='binary') as fileobj:
...     with get_readable_fileobj(fileobj, encoding='UTF-8') as fileobj2:
....        out = fileobj2.seek(1)
...         print(fileobj.closed, fileobj2.closed)
...     print(fileobj.closed, fileobj2.closed)
False False
True True

With this patch:

>>> with get_readable_fileobj(filename, encoding='binary') as fileobj:
...     with get_readable_fileobj(fileobj, encoding='UTF-8') as fileobj2:
....        out = fileobj2.seek(1)
...         print(fileobj.closed, fileobj2.closed)
...     print(fileobj.closed, fileobj2.closed)
True True
True True

So, with this patch, the following is now broken:

>>> with get_readable_fileobj(filename, encoding='binary') as fileobj:
...     with get_readable_fileobj(fileobj, encoding='UTF-8') as fileobj2:
...        out = fileobj2.seek(1)
...     out = fileobj.seek(1)
ValueError: I/O operation on closed file

which seems wrong to me because technically fileobj should still be in scope within the outer context manager.

Thanks @pllim looks like you quickly found the unintended consequences! And as a by-product we have a new regression test that can be added to the utils/data.py unit tests.

So the problem may really be in io.ascii and guessing. I noted in the previous discussion (#8673 (comment)) that my "boiled-down" version of the I/O flow (basically what you did) didn't exactly match what seemed to be happening in the actual io.ascii flow. It looks like more investigation is needed, but I'm closing this PR as it appears to be wrong.

I think the cause of the ResourceWarning is the io.TextIOWrapper use, but indeed closing it may have unexpected consequences, because io.TextIOWrapper seems to always closes wrapped files: https://bugs.python.org/issue21363
There may be some workarounds like the on used by click (https://github.com/pallets/click/blob/master/click/_compat.py#L53) but it's not completely obvious...