classification
Title: io.FileIO calls flush() after file closed
Type: behavior Stage: resolved
Components: IO, Library (Lib) Versions: Python 3.5, Python 3.4, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: bquinlan, martin.panter, pitrou, python-dev, serhiy.storchaka, terry.reedy
Priority: normal Keywords: patch

Created on 2009-04-05 15:35 by bquinlan, last changed 2015-02-22 22:33 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
remove_flush.diff bquinlan, 2009-04-05 15:35 Patch to remove the call to flush by _io._IOBase review
cooperation.diff bquinlan, 2009-04-12 10:25 Use cooperative inheritance review
fileio_flush_closed.patch serhiy.storchaka, 2014-10-12 13:55 review
fileio_flush_closed_2.patch serhiy.storchaka, 2015-02-15 14:12 review
fileio_flush_closed-2.7.patch serhiy.storchaka, 2015-02-15 14:16 review
Messages (22)
msg85521 - (view) Author: Brian Quinlan (bquinlan) (Python committer) Date: 2009-04-05 15:35
>>> import io
>>> class MyIO(io.FileIO):
...     def flush(self):
...             print('closed:', self.closed)
...
>>> f = MyIO('test.out', 'wb')
>>> f.close()
closed: True

IMHO, calling flush() after the file has already been closed is
incorrect behaviour. Search for "Possible py3k io wierdness" on
python-dev for discussion.
msg85616 - (view) Author: Brian Quinlan (bquinlan) (Python committer) Date: 2009-04-06 06:58
Discussion about this bug is ongoing in python-dev.
msg85890 - (view) Author: Brian Quinlan (bquinlan) (Python committer) Date: 2009-04-12 10:33
cooperation.diff:
- change the close method to call .flush() and then ._close()
- only IOBase implements close() (though a subclass can override close
  without causing problems - so long as it calls super().close())
- .flush() invokes super().flush()
- ._close() invokes super()._close()
- FileIO is implemented in Python in _pyio.py so that it can have the
  same base class as the other Python-implemented files classes
- tests verify that .flush() is not called after the file is closed
- tests verify that ._close()/.flush() calls move correctly are 
  propagated correctly
msg85894 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-04-12 11:34
> - FileIO is implemented in Python in _pyio.py so that it can have the
>    same base class as the other Python-implemented files classes

Is it really necessary (e.g. to pass the tests)?
msg85895 - (view) Author: Brian Quinlan (bquinlan) (Python committer) Date: 2009-04-12 11:50
>> - FileIO is implemented in Python in _pyio.py so that it can have the
>>    same base class as the other Python-implemented files classes

> Is it really necessary (e.g. to pass the tests)?

It is necessary to make MI work. With out it the inheritance graph looks
like this (using _pyio):


io.IOBase    _pyio.IOBase
    |              |
io.FileIO       MyMixin
    |              |
     \            /
      \          /
       \        /
         MyClass

If you call MyClass.flush() with this hierarchy then it will propagate
to io.IOBase. Since io.IOBase doesn't call super().flush() in its flush
implementation (because it has no super class), flush propagation would
stop and MyMixin.flush wouldn't be called.

There are other ways you could solve this, of course, like getting
io.IOBase to call super().flush and ignore any errors that it gets.

But it seems like this is the right way to fix this problem anyway - as
a user, I would expect isinstance(FileIO(...), IOBase) but that is not
currently the case with _pyio.
msg85900 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-04-12 12:54
> It is necessary to make MI work. With out it the inheritance graph looks
> like this (using _pyio):
> 
> 
> io.IOBase    _pyio.IOBase
>     |              |
> io.FileIO       MyMixin
>     |              |
>      \            /
>       \          /
>        \        /
>          MyClass

MyMixin doesn't need to inherit from IOBase, since precisely it is a
mixin. It's just a piece of functionality that you drop into an already
existing inheritance tree.

> But it seems like this is the right way to fix this problem anyway - as
> a user, I would expect isinstance(FileIO(...), IOBase) but that is not
> currently the case with _pyio.

That's because the reference ABCs are defined in the io module, not in
_pyio:

True
>>> issubclass(_pyio.FileIO, io.IOBase)
True
>>> issubclass(io.TextIOWrapper, io.IOBase)
True
>>> issubclass(_pyio.TextIOWrapper, io.IOBase)
True

_pyio.IOBase and friends are just concrete implementations of those
ABCs:

True
>>> issubclass(_pyio.TextIOBase, io.TextIOBase)
True

I know it looks a bit non-intuitive at first, but the point is that the
ABCs are unified, even there are two different implementations. I think
it's better than having two different sets of ABCs depending on whether
you import the pure-Python version or the C version.
msg85901 - (view) Author: Brian Quinlan (bquinlan) (Python committer) Date: 2009-04-12 13:44
Antoine Pitrou wrote:
> Antoine Pitrou <pitrou@free.fr> added the comment:
> 
>> It is necessary to make MI work. With out it the inheritance graph looks
>> like this (using _pyio):
>>
>>
>> io.IOBase    _pyio.IOBase
>>     |              |
>> io.FileIO       MyMixin
>>     |              |
>>      \            /
>>       \          /
>>        \        /
>>          MyClass
> 
> MyMixin doesn't need to inherit from IOBase, since precisely it is a
> mixin. It's just a piece of functionality that you drop into an already
> existing inheritance tree.

In my implementation, it does. Otherwise the linearization of the class 
hierarchy becomes:

MyClass -> io.FileIO -> io.IOBase -> MyMixin (-> _pyio.IOBase)

But io.IOBase doesn't make super calls so MyMixin method won't be called.

I think that this linearization is probably more useful:

MyClass -> io.FileIO -> MyMixin -> IOBase

And you get that when io.FileIO and MyMixin share the same IOBase as a 
base class.

I don't mind changing io.IOBase to make super calls in ._close() and 
.flush() and suppressing the attributes errors that will be generated if 
a mixin is not present - but it feels a bit untidy to me.

[snipped]

> I know it looks a bit non-intuitive at first, but the point is that the
> ABCs are unified, even there are two different implementations. I think
> it's better than having two different sets of ABCs depending on whether
> you import the pure-Python version or the C version.

I'm not trying to change the ABC unification at all - and if I did then 
there is a bug in my code. I just think that the immediate parent class 
of _pyio.FileIO should be _pyio.IOBase (just like _io.FileIO has 
_io.IOBase as an immediate parent). That will make the Python and C 
class hierarchies completely consistent within themselves.

Cheers,
Brian
msg85902 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-04-12 13:48
> I think that this linearization is probably more useful:
> 
> MyClass -> io.FileIO -> MyMixin -> IOBase

But why not simply:

MyClass -> MyMixin -> io.FileIO -> IOBase

?
Is there something I'm missing that prevents you from doing this?

> I'm not trying to change the ABC unification at all - and if I did then 
> there is a bug in my code. I just think that the immediate parent class 
> of _pyio.FileIO should be _pyio.IOBase (just like _io.FileIO has 
> _io.IOBase as an immediate parent). That will make the Python and C 
> class hierarchies completely consistent within themselves.

I understand, but that's at the price of an otherwise useless
indirection layer, which will also make _pyio even slower that it
already is :-)
(I admit, however, that _pyio shouldn't be used in normal circumstances,
so this is not a showstopper argument)
msg85913 - (view) Author: Brian Quinlan (bquinlan) (Python committer) Date: 2009-04-12 17:44
Antoine Pitrou wrote:
> Antoine Pitrou <pitrou@free.fr> added the comment:
> 
>> I think that this linearization is probably more useful:
>>
>> MyClass -> io.FileIO -> MyMixin -> IOBase
> 
> But why not simply:
> 
> MyClass -> MyMixin -> io.FileIO -> IOBase
> 
> ?
> Is there something I'm missing that prevents you from doing this?

No, doing this is trivial. But shouldn't it be up to the implementor of 
MyClass to decide whether MyMixin or io.FileIO methods are evaluated first?

>> I'm not trying to change the ABC unification at all - and if I did then 
>> there is a bug in my code. I just think that the immediate parent class 
>> of _pyio.FileIO should be _pyio.IOBase (just like _io.FileIO has 
>> _io.IOBase as an immediate parent). That will make the Python and C 
>> class hierarchies completely consistent within themselves.
> 
> I understand, but that's at the price of an otherwise useless
> indirection layer, which will also make _pyio even slower that it
> already is :-)

Maybe I misunderstand the purpose of _pyio. The Python 3.1 says that its 
purpose is for experimentation. For experimentation, having a Python 
implementation where you can add methods and change behavior (though 
perhaps not in as deep as way is if this class were completely written 
in Python) is useful. It is also useful for the behavior of the Python 
implementation to match that of the C implementation as closely as 
possible - this patch makes the inheritance graph for _pyio.FileIO more 
consistent.

I, for example, want to add a sync() method to FileIO that will call 
fsync() on the file's file descriptor. With this change, I have a place 
to plug in that change in Python and I can write the C implementation 
when I have the Python implementation right.

Cheers,
Brian
msg85914 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-04-12 17:52
> No, doing this is trivial. But shouldn't it be up to the implementor of 
> MyClass to decide whether MyMixin or io.FileIO methods are evaluated first?

Is there a concrete use case, though?

By the way, what if _pyio.FileIO inherited from io.FileIO instead of
delegating all methods?
msg85915 - (view) Author: Brian Quinlan (bquinlan) (Python committer) Date: 2009-04-12 18:19
Antoine Pitrou wrote:
> Antoine Pitrou <pitrou@free.fr> added the comment:
> 
>> No, doing this is trivial. But shouldn't it be up to the implementor of 
>> MyClass to decide whether MyMixin or io.FileIO methods are evaluated first?
> 
> Is there a concrete use case, though?

I don't have a good one in mind - though

> By the way, what if _pyio.FileIO inherited from io.FileIO instead of
> delegating all methods?

I don't think that would work - but reasoning about MRO hurts my brain ;-)

You'd have to declare the class like this:

class FileIO(io.FileIO, IOBase):
   pass

to get the io.File methods to resolve first. But that means that the 
method invocation chain would be broken by io.IOBase, which doesn't make 
super calls.

Cheers,
Brian
msg85917 - (view) Author: Brian Quinlan (bquinlan) (Python committer) Date: 2009-04-12 20:01
Oops, I didn't finish my thought:

>> No, doing this is trivial. But shouldn't it be up to the implementor of 
>> MyClass to decide whether MyMixin or io.FileIO methods are evaluated
first?
> 
> Is there a concrete use case, though?

I don't have a good one in mind - though writing the unit test
implementation required that the mixin class methods be called last.
msg139205 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2011-06-26 19:54
The original snippet works the same on 3.2.0. Was their any conclusion as to whether or not a change should be made?
msg228234 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-10-02 16:00
There is complete implementation of FileIO in pure Python in issue21859. It is free from this bug.

>>> import _pyio as io
>>> class MyIO(io.FileIO):
...     def flush(self):
...             print('closed:', self.closed)
... 
>>> f = MyIO('test.out', 'wb')
>>> f.close()
closed: False
msg229143 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-10-12 13:55
Yet one related bug is that flush() isn't called at all if the file was opened with closefd=False.

>>> import io, os
>>> class MyIO(io.FileIO):
...     def flush(self):
...         print('closed:', self.closed)
... 
>>> fd = os.open('test.out', os.O_WRONLY|os.O_CREAT)
>>> f = MyIO(fd, 'wb', closefd=False)
>>> f.close()

The proposed simple patch fixes both bugs.
msg234880 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-01-28 09:27
Ping.
msg236039 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-02-15 14:12
Added comments as Antoine suggested.
msg236040 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-02-15 14:16
And here is a patch for 2.7.
msg236074 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-02-15 22:38
For the record, the python-dev thread referenced in the original post is <https://mail.python.org/pipermail/python-dev/2009-April/088212.html>.

The obvious fix to me is to have FileIO.close() call IOBase.close() to invoke the flush() before continuing with its own closing. This is what Serhiy’s patches appear to do, so I agree with them in general.

I do wonder what the point of duplicating test code in test_io and test_fileio is. The only difference seems to be calling open() versus calling FileIO() directly. Wouldn’t it be better to merge the code together, or maybe just assert open() returns a FileIO instance?
msg236337 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-02-20 22:14
Agree, the test in test_fileio is redundant.
msg236339 - (view) Author: Roundup Robot (python-dev) Date: 2015-02-20 22:37
New changeset 7052206ad381 by Serhiy Storchaka in branch '2.7':
Issue #5700: io.FileIO() called flush() after closing the file.
https://hg.python.org/cpython/rev/7052206ad381

New changeset 36f5c36b7704 by Serhiy Storchaka in branch '3.4':
Issue #5700: io.FileIO() called flush() after closing the file.
https://hg.python.org/cpython/rev/36f5c36b7704

New changeset e1f08f5b6b62 by Serhiy Storchaka in branch 'default':
Issue #5700: io.FileIO() called flush() after closing the file.
https://hg.python.org/cpython/rev/e1f08f5b6b62
msg236417 - (view) Author: Roundup Robot (python-dev) Date: 2015-02-22 22:33
New changeset bf52f69d6948 by Serhiy Storchaka in branch '2.7':
Broke reference loops in tests added in issue #5700.
https://hg.python.org/cpython/rev/bf52f69d6948

New changeset 00bde0743690 by Serhiy Storchaka in branch '3.4':
Broke reference loops in tests added in issue #5700.
https://hg.python.org/cpython/rev/00bde0743690

New changeset a8df1ca60452 by Serhiy Storchaka in branch 'default':
Broke reference loops in tests added in issue #5700.
https://hg.python.org/cpython/rev/a8df1ca60452
History
Date User Action Args
2015-02-22 22:33:59python-devsetmessages: + msg236417
2015-02-20 22:37:46serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2015-02-20 22:37:12python-devsetnosy: + python-dev
messages: + msg236339
2015-02-20 22:14:48serhiy.storchakasetmessages: + msg236337
2015-02-15 22:38:27martin.pantersetnosy: + martin.panter
messages: + msg236074
2015-02-15 14:16:11serhiy.storchakasetfiles: + fileio_flush_closed-2.7.patch

messages: + msg236040
2015-02-15 14:12:07serhiy.storchakasetfiles: + fileio_flush_closed_2.patch

messages: + msg236039
versions: + Python 2.7
2015-01-28 09:27:41serhiy.storchakasetmessages: + msg234880
2014-10-12 13:55:15serhiy.storchakasetfiles: + fileio_flush_closed.patch

assignee: serhiy.storchaka
components: + IO
versions: + Python 3.4, Python 3.5, - Python 3.2, Python 3.3
type: behavior
messages: + msg229143
stage: patch review
2014-10-02 16:00:50serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg228234
2011-06-26 19:54:48terry.reedysetnosy: + terry.reedy

messages: + msg139205
versions: + Python 3.2, Python 3.3, - Python 3.1
2009-04-12 20:01:26bquinlansetmessages: + msg85917
2009-04-12 18:19:42bquinlansetmessages: + msg85915
2009-04-12 17:52:35pitrousetmessages: + msg85914
2009-04-12 17:44:09bquinlansetmessages: + msg85913
2009-04-12 13:48:19pitrousetmessages: + msg85902
2009-04-12 13:44:22bquinlansetmessages: + msg85901
2009-04-12 12:54:28pitrousetmessages: + msg85900
2009-04-12 11:50:40bquinlansetmessages: + msg85895
2009-04-12 11:34:38pitrousetnosy: + pitrou
messages: + msg85894
2009-04-12 10:33:08bquinlansetmessages: + msg85890
2009-04-12 10:25:29bquinlansetfiles: + cooperation.diff
2009-04-06 06:58:45bquinlansetmessages: + msg85616
2009-04-05 15:35:55bquinlancreate