New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
_pyio.BufferedReader and _pyio.TextIOWrapper destructor don't emit ResourceWarning if the file is not closed #64028
Comments
$ ./python
Python 3.4.0b1 (default:acabd3f035fe, Nov 28 2013, 15:04:09)
[GCC 4.8.2 20131017 (Red Hat 4.8.2-1)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import _pyio
>>> f=_pyio.open("/etc/issue"); f=None
>>> f=_pyio.open("/etc/issue", "rb"); f=None
>>> f=_pyio.open("/etc/issue", "rb", 0); f=None
__main__:1: ResourceWarning: unclosed file <_io.FileIO name='/etc/issue' mode='rb'>
>>> import io
>>> f=io.open("/etc/issue"); f=None
__main__:1: ResourceWarning: unclosed file <_io.TextIOWrapper name='/etc/issue' mode='r' encoding='UTF-8'>
>>> f=io.open("/etc/issue", "rb"); f=None
__main__:1: ResourceWarning: unclosed file <_io.BufferedReader name='/etc/issue'>
>>> f=io.open("/etc/issue", "rb", 0); f=None
__main__:1: ResourceWarning: unclosed file <_io.FileIO name='/etc/issue' mode='rb'> I expect the same behaviour when I use _pyio or io module. |
I think it will be good. |
Related bpo-21859 "Add Python implementation of FileIO" |
It looks like the C _io close() implementations delegate to the wrapped object’s undocumented _dealloc_warn() method to emit the warning if “self->finalizing” is set. For wrapped objects like BytesIO that do not have it, I guess the error due to the missing method is ignored. There is no Python implementation of FileIO yet (see bpo-21859), so the Python file wrapper classes still use the C FileIO implementation, which emits a resource warning. However for the wrapper classes, _pyio.IOBase.__del__() explicitly invokes the wrapper’s close() method, which will invoke FileIO.close() method, which bypasses the warning. Similarly, any object inheriting from the C implmentation’s _io.IOBase will also have close() explicitly invoked when being finalized, bypassing any warnings of the underlying wrapped object. The SocketIO instances returned by socket.makefile() are an example of this. |
One option would be for any wrapper-type class (e.g. BufferedReader, SocketIO) to override __del__(). Instead of calling close(), it should call detach() or equivalent, and delete the returned reference to the underlying wrapped object without explicitly closing it, to allow a warning to be generated. Maybe a WrappedIO mixin class could help with this. I am posting del-detach.patch which roughly implements the above option. It only seems to break the test_override_destructor() tests for the TextIOWrapperTest and CommonBufferedTests classes, which expect close() to be called. Another more radical option might be to make IOBase.__del__() only call flush(), not close(), except for the actual implementations that really should emit a warning, like FileIO. I will have a go at doing this option in another patch. Both these options would change the behaviour in some cases that rely on the garbage collector to close objects. They would defer the automatic closing until all references of the underlying object are garbage collected, instead of just when the wrapped object is garbage collected. A third option would be to add explicit __del__() methods emitting a warning in each class, along with any exception supression needed due to Python interpreter finalization quirks. |
Posting del-flush.patch, which only calls flush() instead of close() from __del__(), except for the FileIO class. Quick analysis of resulting test failures: These tests fail because they are obviously written to test that __del__ calls close():
CBufferedReader/Writer/RandomTest.test_garbage_collection() fail because they do not free the reference to the wrapped raw file object. Similarly, CTextIOWrapperTest.test_garbage_collection() holds references to both levels of wrapped object. TextIOWrapperTest.test_destructor() fails because it assumes the wrapped close() will be called. The data is still written to the wrapped file though. MiscIOTest.test_warn_on_dealloc/_fd() fail because the warning message only refers to the FileIO object rather than the wrapper object. |
Attached patch modifies _pyio to mimick better the reference io module:
I added hasattr(.., '_dealloc_warn') sanity checks because _dealloc_warn() are added to concrete classes, not to base classes. |
(oops, there was a typo in my first patch) |
Oh, the io module is more complex than what I expected :-) test_io fails with pyio_res_warn-2.patch. test_io now pass with pyio_res_warn-3.patch. I fixed FileIO.close() to always call super().close(), and set self._fd to -1 even if logging the ResourceWarning raised an exception. |
Try attached res_warn.py script to test manually all ResourceWarning warnings. Ouput of python3.6 -Wd res_warn.py: buffered raw fileio -------------------------- Ouput of python3.6 -X tracemalloc=2 -Wd res_warn.py: buffered raw fileio -------------------------- |
del-detach.patch: I don't understand the change in _pyio.py. I think that the change on socket.py is no more need since the change 46329eec5515 (issue bpo-26590). del-flush.patch:
Well, I guess that these two patches were written before Serhiy reimplemented the FileIO in pure Python, and this change outdated your two patches. |
I don't like _dealloc_warn() at all. It looks as a dirty hack and doesn't work with custom file-like objects. I think we should use one of Martin's option. If there are any issues with garbage collecting, we should fix the garbage collector. |
I tried to support emitting a ResourceWarning when the close() method is overriden in a subclass, but then I saw that io doesn't support this use case neither: import io
import os
class MyFile(io.FileIO):
def close(self):
os.close(self.fileno())
f = MyFile('/etc/issue')
f = None => no warning Ok, now I have to read again Martin's patches since I didn't understand them at the first read :-) |
Ok, here is a new simpler proposition: remove all _dealloc_warn() method, and simply emit the ResourceWarning in IOBase finalizer. With this change, all objects which inherit from io.IOBase (or _pyio.IOBase) now emits a ResourceWarning if they are not closed explicitly. You have to override the __del__() method to prevent this warning. A lot of new objects start to log ResourceWarning: io.SocketIO, _pyio.TextIOWrapper, io.BytesIO, http.client.HTTPResponse, etc. For io.BytesIO, I fixed the code to inherit correctly IOBase finalizer. -- A lot of tests start to fail because they emit a lot of ResourceWarning warnings. I don't know yet if it's a feature or a bug :-) -- With the patch, Python starts to logs warnings like: sys:1: ResourceWarning: unclosed file <_io.TextIOWrapper name='<stdout>' mode='w' encoding='UTF-8'> You can use my patch of the issue bpo-26642 to fix these warnings. ResourceWarning is emited even if FileIO.closefd is false. Note: It looks like the ResourceWarning is not always displayed. Yet another bug? |
Oh, I forgot to include it in my patch. But it's maybe better to have it in a different patch: see attached bytesio_stringio.patch which changes also io.StringIO to inherit the IOBase finalizer. Emitting a ResourceWarning in io.BytesIO and io.StringIO adds a *lot* of warnings when running the Python test suite. Since these objects only contain memory, no very limited resource like file descriptors, it's probably better to *not* log ResourceWarning for them. |
Ok, I now understand the problem better. They are two kinds of io objects: (1) object which directly or indirectly owns a limited resource like file descriptor => must emit a ResourceWarning Examples of (1): FileIO, BufferedReader(FileIO), TextIOWrapper(BufferedReader(FileIO)) Examples of (2): BytesIO, BuffereadReader(BytesIO), TextIOWrapper(BuffereadReader(BytesIO)) The tricky part is to decide if an object owns a limited resource or not. Currently, the io module uses the _dealloc_warn() trick. A close() method tries to call _dealloc_warn(), but it ignores any exception when calling _dealloc_warn(). BufferedReader calls raw._dealloc_warn(). TextIOWrapper calls buffer._dealloc_warn(). For case (1), BufferedReader(FileIO).close() calls FileIO._dealloc_warn() => ResourceWarning is logged For case (2), BufferedReader(BytesIO).close() calls BytesIO._dealloc_warn() raises an AttributeError => no warning is logged Well, we can call this a hack, but it works :-) pyio_res_warn-3.patch implements the same hack in _pyio. |
|
In the long term, I prefer not calling close() in __del__(), like del-flush.patch, although I admit it is an API change and should probably be limited to 3.6+. If people want to improve things in 3.5, privately implementing _dealloc_warn() like Victor’s pyio_res_warn-3.patch seems the best option. Serhiy: why did you add 2.7 to this bug? For 2.7, I don’t think anything should be done. There is no ResourceWarning in 2.7. In 3.5, _dealloc_warn() could also be implemented in SocketIO (possibly also HTTPResponse, GzipFile, etc). But it should not become a public API, and I don’t think it is important to make this sort of change to 3.5 anyway. In 3.6, if we stopped __del__() from calling close() like del-flush.patch, we would be changing the documented behaviour: <https://docs.python.org/3.6/library/io.html#io.IOBase.\_\_del__\> says “IOBase . . . calls the instance’s close() method.” But the end result seems cleaner to me. I think changing or adding an API (del, _dealloc_warn, etc) is necessary for a general solution to the problem. The effect of del-flush.patch will be that a wrapped FileIO or similar object will not be closed until all references are deleted. >>> file = open(os.devnull, "wb", 0)
>>> print(file)
<_io.FileIO name='/dev/null' mode='wb' closefd=True>
>>> wrapper = BufferedWriter(file) In 3.5, deleting the wrapper produces a warning and closes the underlying file: >>> del wrapper
__main__:1: ResourceWarning: unclosed file <_io.BufferedWriter name='/dev/null'>
>>> print(file)
<_io.FileIO [closed]>
>>> file.write(b"more data")
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ValueError: I/O operation on closed file I propose that in 3.6, deleting a wrapper should not automatically close or warn about the wrapped object until all references are deleted: >>> del wrapper # No problem; we still have a reference to "file"
>>> file.write(b"more data") # File is still open
9
>>> del file # Finally closes the file and triggers the warning
ResourceWarning: unclosed file <FileIO> |
Just for the case. Not calling close() in __del__() is one option, and it looks attractive. But there are possible hidden catches. There is no guarantee that flush() writes all buffered data, some stateful encoder or compressor can left some data in the buffer unless explicitly closed. close() can write required footer or close connection. If not call close() in __del__(), GzipFile will produce incorrect file. May be this is appropriate. This is why explicit close() should be called or context manager should be used. Other option is to use some API through all closable objects. _dealloc_warn() is not the only option.
|
While it would be great to also emit ResourceWarning, in the last 8 years, nobody managed to implement the feature. Also, this issue has no activity. I prefer to close the issue. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: