In _fileio.c, there is the following comment: "Returns 0 on success,
errno (which is < 0) on failure." The problem here is the claim that
errno ever was less than zero, which is simply wrong.

You can see this being a problem with the following few lines:

import os, io
fd = os.open( "some existing file", os.O_RDONLY)
s1 = os.fdopen(fd)
s2 = io.open(fd)
os.close(fd)
s1.close()
s2.close()

The call to close() pulls the file from under the feet of the two stream
objects, but only the one opened with os.fdopen() actually detects that.
For the second one, errno is set, but to a positive value which isn't
detected correctly.

This adds a test that the close() correctly fails with an IOError.

Strangely enough, it works in py3k.

In Python 3.x, internal_close() returns -1 on error (if close() 
returns a negative value) and 0 on success.

In Python 2.x, internal_closes() returns the errno value of close() on 
error, or 0 on success. fileio_close() tests if the error value is 
negative instead of if it's not null or at least it's strict positive.

Python 3.x code looks better. I tried to backport the internal_close() 
changes but I get a segfault on PyErr_WriteUnraisable() :-p (close() 
error in fileio_dealloc)

I have a patch here which passes the proposed change to the tests in
trunk. A few notes up front though, since the patch isn't good yet:

1. The handling of the error in internal_close() isn't really good. What
I'm also not sure about is whether it is correct to set the error there,
the old code sometimes did set an error as reaction to internal_close()
failing and sometimes not.
2. There is one place in dircheck() where I think that the error should
be ignored, but I'm not sure about that.
3. This patch fixes another issue, and that is that internal_close()
would call close() even if 'closefd' was not set. This e.g. happens when
__init__ is called explicitly. This case is missing a test though.
4. I wonder if internal_close() should reset only the 'fd' field or if
it should also reset 'readable', 'writable', 'seekable', too, i.e. apart
from 'weakreflist' do the same as fileio_new() does.

> 1. The handling of the error in internal_close() isn't really good. What
> I'm also not sure about is whether it is correct to set the error there,
> the old code sometimes did set an error as reaction to internal_close()
> failing and sometimes not.

internal_close() is used in:
 - fileio_close(): raise an IOError(errno) on internal_close() failure
 - dircheck(): ignore internal_close() failure, whereas Python 3.x raise 
   a IOError(errno) on internal_close() failure
 - file_init(): catch internal_close() failure but don't set an exception,
   whereas Python 3.x set a IOError(errno)
 - fileio_dealloc(): PySys_WriteStderr(...) in Python 2.x,
   PyErr_WriteUnraisable(self) in Python 3.x

But Python 2.x all errors are ignored because the test (errno < 0) is wrong...

I always prefer Python 3.x behaviour: internal_close() is responsible to raise 
an exception.

> 2. There is one place in dircheck() where I think that the error should
> be ignored, but I'm not sure about that.

I rarely a good idea to ignore errors, I prefer to raise a different error if 
the file can not be closed.

> 3. This patch fixes another issue, and that is that internal_close()
> would call close() even if 'closefd' was not set. This e.g. happens when
> __init__ is called explicitly. This case is missing a test though.

Python 3.x is also affected by this issue: file_init() and dircheck() ignore 
self->closefd.

passwd = open("/etc/passwd")
f = _FileIO(fd, closefd=False)
f.__init__(fd, closefd=False) => close passwd file!

> 4. I wonder if internal_close() should reset only the 'fd' field or if
> it should also reset 'readable', 'writable', 'seekable', too, i.e. apart
> from 'weakreflist' do the same as fileio_new() does.

seekable(), readable() and writable() raise an error if the file is closed. I 
think it's fine to leave the internal attributes unchanged.

>> 4. I wonder if internal_close() should reset only the 'fd' field
>> or if it should also reset 'readable', 'writable', 'seekable'
>
> seekable(), readable() and writable() raise an error if the file
> is closed. I think it's fine to leave the internal attributes
> unchanged.

That's not what I'm concerned about. What I'm concerned about is this:

f = _FileIO(open("some text file"))
print f.seekable()
f.__init__(open("some pipe"))
print f.seekable()

The cached value whether the file was seekable survives the __init__ 
call but is actually wrong then. Of course, one could also reset the 
value in __init__. Whether the file is readable or writable is 
different, since those values are supplied via the mode string and the 
code by the user and not determined automatically.


I think that I just found another buglet, too: when supplying a file 
descriptor, dircheck() isn't actually called. os.open() under 2.5 on 
Debian/Linux at least allows opening '.', trunk under MS Windows fails 
with "permission denied".

Please note the comment from Victor:-
"Python 3.x is also affected by this issue: file_init() and dircheck() ignore self->closefd."

Might as well close as nobody appears interested.

fileio_close.patch (for Python 3.3): Fix FileIO.__init__() to not close the file if closefd=False and the constructor is called twice (or more).

fileio_close.patch should maybe use os.open() (to create the fd) and os.fstat() (to check that the fd is not closed).

Status of this issue:
 - io.FileIO.close() raises IOError with Python 2.7, 3.1, 3.2 and 3.3 (e.g. if the underlying file descriptor has been closed), it doesn't with Python 2.6
 - If FileIO constructor is called twice, the file is closed at the second call even if closefd was False at the first call: bug in Python 2.6-3.3: fileio_close.patch fixes this bug

So except fixing FileIO constructor (using fileio_close.patch) in Python 2.7, 3.2 and 3.3, there is nothing more to do.

The reasoning is reasonable, the patch trivial & LGTM, the test suite passes. Anything I’m missing, why this is still open?

Hynek, no obvious reason, it probably slipped through :)

New changeset 83d500908ffc by Hynek Schlawack in branch '2.7':
#4841: Fix FileIO constructor to honor closefd when called repeatedly
http://hg.python.org/cpython/rev/83d500908ffc

New changeset 8a58670048c9 by Hynek Schlawack in branch '3.2':
#4841: Fix FileIO constructor to honor closefd when called repeatedly
http://hg.python.org/cpython/rev/8a58670048c9

New changeset d2b511592cc5 by Hynek Schlawack in branch 'default':
#4841: Fix FileIO constructor to honor closefd when called repeatedly
http://hg.python.org/cpython/rev/d2b511592cc5

I’ve committed due to haypo's wish on IRC. ;)