Why does IOBase.__del__ call .close?

[Nikolaus Rath]

Hello,

I recently noticed (after some rather protacted debugging) that the
io.IOBase class comes with a destructor that calls self.close():

[0] ***@vostro:~/tmp$ cat test.py
import io
class Foo(io.IOBase):
def close(self):
print('close called')
r = Foo()
del r
[0] ***@vostro:~/tmp$ python3 test.py
close called

To me, this came as quite a surprise, and the best "documentation" of
this feature seems to be the following note (from the io library
reference):

"The abstract base classes also provide default implementations of some
methods in order to help implementation of concrete stream classes. For
example, BufferedIOBase provides unoptimized implementations of
readinto() and readline()."

For me, having __del__ call close() does not qualify as a reasonable
default implementation unless close() is required to be idempotent
(which one could deduce from the documentation if one tries to, but it's
far from clear).

Is this behavior an accident, or was that a deliberate decision?


Best,
-Nikolaus

[MRAB]

Post by Nikolaus Rath
Hello,
I recently noticed (after some rather protacted debugging) that the
import io
print('close called')
r = Foo()
del r
close called
To me, this came as quite a surprise, and the best "documentation" of
this feature seems to be the following note (from the io library
"The abstract base classes also provide default implementations of some
methods in order to help implementation of concrete stream classes. For
example, BufferedIOBase provides unoptimized implementations of
readinto() and readline()."
For me, having __del__ call close() does not qualify as a reasonable
default implementation unless close() is required to be idempotent
(which one could deduce from the documentation if one tries to, but it's
far from clear).
Is this behavior an accident, or was that a deliberate decision?

To me, it makes sense. You want to make sure that it's closed, releasing
any resources it might be holding, even if you haven't done so
explicitly.

[Nikolaus Rath]

Post by MRAB

Post by Nikolaus Rath
Hello,
I recently noticed (after some rather protacted debugging) that the
import io
print('close called')
r = Foo()
del r
close called
To me, this came as quite a surprise, and the best "documentation" of
this feature seems to be the following note (from the io library
"The abstract base classes also provide default implementations of some
methods in order to help implementation of concrete stream classes. For
example, BufferedIOBase provides unoptimized implementations of
readinto() and readline()."
For me, having __del__ call close() does not qualify as a reasonable
default implementation unless close() is required to be idempotent
(which one could deduce from the documentation if one tries to, but it's
far from clear).
Is this behavior an accident, or was that a deliberate decision?

To me, it makes sense. You want to make sure that it's closed, releasing
any resources it might be holding, even if you haven't done so
explicitly.

I agree with your intentions, but I come to the opposite conclusion:
automatically calling close() in the destructor will hide that there's a
problem in the code. Without that automatic cleanup, there's at least a
good chance that a ResourceWarning will be emitted so the problem gets
noticed. "Silently work around bugs in caller's code" doesn't seem like
a very useful default to me...


Best,
-Nikolaus

[Benjamin Peterson]

Post by Nikolaus Rath

Post by MRAB

Post by Nikolaus Rath
Hello,
I recently noticed (after some rather protacted debugging) that the
import io
print('close called')
r = Foo()
del r
close called
To me, this came as quite a surprise, and the best "documentation" of
this feature seems to be the following note (from the io library
"The abstract base classes also provide default implementations of some
methods in order to help implementation of concrete stream classes. For
example, BufferedIOBase provides unoptimized implementations of
readinto() and readline()."
For me, having __del__ call close() does not qualify as a reasonable
default implementation unless close() is required to be idempotent
(which one could deduce from the documentation if one tries to, but it's
far from clear).
Is this behavior an accident, or was that a deliberate decision?

To me, it makes sense. You want to make sure that it's closed, releasing
any resources it might be holding, even if you haven't done so
explicitly.

automatically calling close() in the destructor will hide that there's a
problem in the code. Without that automatic cleanup, there's at least a
good chance that a ResourceWarning will be emitted so the problem gets
noticed. "Silently work around bugs in caller's code" doesn't seem like
a very useful default to me...

Things which actually hold system resources (like FileIO) give
ResourceWarning if they close in __del__, so I don't understand your
point.

[Nikolaus Rath]

Post by Benjamin Peterson

Post by Nikolaus Rath

Post by MRAB

Post by Nikolaus Rath
Hello,
I recently noticed (after some rather protacted debugging) that the
import io
print('close called')
r = Foo()
del r
close called
To me, this came as quite a surprise, and the best "documentation" of
this feature seems to be the following note (from the io library
"The abstract base classes also provide default implementations of some
methods in order to help implementation of concrete stream classes. For
example, BufferedIOBase provides unoptimized implementations of
readinto() and readline()."
For me, having __del__ call close() does not qualify as a reasonable
default implementation unless close() is required to be idempotent
(which one could deduce from the documentation if one tries to, but it's
far from clear).
Is this behavior an accident, or was that a deliberate decision?

To me, it makes sense. You want to make sure that it's closed, releasing
any resources it might be holding, even if you haven't done so
explicitly.

automatically calling close() in the destructor will hide that there's a
problem in the code. Without that automatic cleanup, there's at least a
good chance that a ResourceWarning will be emitted so the problem gets
noticed. "Silently work around bugs in caller's code" doesn't seem like
a very useful default to me...

Things which actually hold system resources (like FileIO) give
ResourceWarning if they close in __del__, so I don't understand your
point.

Consider this simple example:

$ cat test.py
import io
import warnings

class StridedStream(io.IOBase):
def __init__(self, name, stride=2):
super().__init__()
self.fh = open(name, 'rb')
self.stride = stride

def read(self, len_):
return self.fh.read(self.stride*len_)[::self.stride]

def close(self):
self.fh.close()

class FixedStridedStream(StridedStream):
def __del__(self):
# Prevent IOBase.__del__ frombeing called.
pass

warnings.resetwarnings()
warnings.simplefilter('error')

print('Creating & loosing StridedStream..')
r = StridedStream('/dev/zero')
del r

print('Creating & loosing FixedStridedStream..')
r = FixedStridedStream('/dev/zero')
del r

$ python3 test.py
Creating & loosing StridedStream..
Creating & loosing FixedStridedStream..
Exception ignored in: <_io.FileIO name='/dev/zero' mode='rb'>
ResourceWarning: unclosed file <_io.BufferedReader name='/dev/zero'>

In the first case, the destructor inherited from IOBase actually
prevents the ResourceWarning from being emitted.


Best,
-Nikolaus

[Benjamin Peterson]

Post by Nikolaus Rath
$ cat test.py
import io
import warnings
super().__init__()
self.fh = open(name, 'rb')
self.stride = stride
return self.fh.read(self.stride*len_)[::self.stride]
self.fh.close()
# Prevent IOBase.__del__ frombeing called.
pass
warnings.resetwarnings()
warnings.simplefilter('error')
print('Creating & loosing StridedStream..')
r = StridedStream('/dev/zero')
del r
print('Creating & loosing FixedStridedStream..')
r = FixedStridedStream('/dev/zero')
del r
$ python3 test.py
Creating & loosing StridedStream..
Creating & loosing FixedStridedStream..
Exception ignored in: <_io.FileIO name='/dev/zero' mode='rb'>
ResourceWarning: unclosed file <_io.BufferedReader name='/dev/zero'>
In the first case, the destructor inherited from IOBase actually
prevents the ResourceWarning from being emitted.

Ah, I see. I don't see any good ways to fix it, though, besides setting
some flag if close() is called from __del__.

[Nikolaus Rath]

Post by Benjamin Peterson

Post by Nikolaus Rath
$ cat test.py
import io
import warnings
super().__init__()
self.fh = open(name, 'rb')
self.stride = stride
return self.fh.read(self.stride*len_)[::self.stride]
self.fh.close()
# Prevent IOBase.__del__ frombeing called.
pass
warnings.resetwarnings()
warnings.simplefilter('error')
print('Creating & loosing StridedStream..')
r = StridedStream('/dev/zero')
del r
print('Creating & loosing FixedStridedStream..')
r = FixedStridedStream('/dev/zero')
del r
$ python3 test.py
Creating & loosing StridedStream..
Creating & loosing FixedStridedStream..
Exception ignored in: <_io.FileIO name='/dev/zero' mode='rb'>
ResourceWarning: unclosed file <_io.BufferedReader name='/dev/zero'>
In the first case, the destructor inherited from IOBase actually
prevents the ResourceWarning from being emitted.

Ah, I see. I don't see any good ways to fix it, though, besides setting
some flag if close() is called from __del__.

How about not having IOBase.__del__ call self.close()? Any resources
acquired by the derived class would still clean up after themselves when
they are garbage collected.


Best,
-Nikolaus

[Benjamin Peterson]

Post by Nikolaus Rath

Post by Benjamin Peterson

Post by Nikolaus Rath
$ cat test.py
import io
import warnings
super().__init__()
self.fh = open(name, 'rb')
self.stride = stride
return self.fh.read(self.stride*len_)[::self.stride]
self.fh.close()
# Prevent IOBase.__del__ frombeing called.
pass
warnings.resetwarnings()
warnings.simplefilter('error')
print('Creating & loosing StridedStream..')
r = StridedStream('/dev/zero')
del r
print('Creating & loosing FixedStridedStream..')
r = FixedStridedStream('/dev/zero')
del r
$ python3 test.py
Creating & loosing StridedStream..
Creating & loosing FixedStridedStream..
Exception ignored in: <_io.FileIO name='/dev/zero' mode='rb'>
ResourceWarning: unclosed file <_io.BufferedReader name='/dev/zero'>
In the first case, the destructor inherited from IOBase actually
prevents the ResourceWarning from being emitted.

Ah, I see. I don't see any good ways to fix it, though, besides setting
some flag if close() is called from __del__.

How about not having IOBase.__del__ call self.close()? Any resources
acquired by the derived class would still clean up after themselves when
they are garbage collected.

Well, yes, but that's probably a backwards compat problem.

[Antoine Pitrou]

Post by Nikolaus Rath
For me, having __del__ call close() does not qualify as a reasonable
default implementation unless close() is required to be idempotent
(which one could deduce from the documentation if one tries to, but it's
far from clear).

close() should indeed be idempotent on all bundled IO class
implementations (otherwise it's a bug), and so should it preferably on
third-party IO class implementations.

If you want to improve the documentation on this, you're welcome to
provide a patch!

Regards

Antoine.

[Nick Coghlan]

Post by Antoine Pitrou

Post by Nikolaus Rath
For me, having __del__ call close() does not qualify as a reasonable
default implementation unless close() is required to be idempotent
(which one could deduce from the documentation if one tries to, but it's
far from clear).

close() should indeed be idempotent on all bundled IO class

implementations (otherwise it's a bug), and so should it preferably on
third-party IO class implementations.

Post by Antoine Pitrou
If you want to improve the documentation on this, you're welcome to

provide a patch!

We certainly assume idempotent close() behaviour in various places, so if
that expectation isn't currently clear in the docs, suggestions for
improved wording would definitely be appreciated!

Cheers,
Nick.

[Serhiy Storchaka]

Post by Antoine Pitrou
close() should indeed be idempotent on all bundled IO class
implementations (otherwise it's a bug), and so should it preferably on
third-party IO class implementations.

There are some questions about close().

1. If object owns several resources, should close() try to clean up all
them if error is happened during cleaning up some resource. E.g. should
BufferedRWPair.close() close reader if closing writer failed?

2. If close() raises an exception, should repeated call of close() raise
an exception or do nothing? E.g. if GzipFile.close() fails during
writing gzip tail (CRC and size), should repeated call of it try to
write this tail again?

3. If close() raises an exception, should the closed attribute (if
exists) be True or False?