classification
Title: Don’t close fd when FileIO.__init__ fails
Type: behavior Stage: resolved
Components: IO Versions: Python 3.2, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: amaury.forgeotdarc, eric.araujo, haypo, hynek, ocean-city, pitrou, python-dev
Priority: normal Keywords: patch

Created on 2010-10-09 08:08 by ocean-city, last changed 2012-06-25 11:19 by amaury.forgeotdarc. This issue is now closed.

Files
File name Uploaded Description Edit
py3k_ensure_fd_not_closed_after_fileio_init_failed.patch ocean-city, 2010-10-09 08:08 review
test_ensure_fd_not_closed_after_fileio_init_failed.py ocean-city, 2010-10-09 08:10 single test script
fileio-do-not-close-on-fail.diff hynek, 2012-05-29 13:41 review
Messages (12)
msg118249 - (view) Author: Hirokazu Yamamoto (ocean-city) * (Python committer) Date: 2010-10-09 08:08
I suppose when _io.FileIO(fd) failed, passed fd
should not be closed. Otherwise, we cannot write the
code like this.

     fd = os.open(path, os.O_RDONLY)
     try:
         buf = io.open(fd, "wb")
     except:
         os.close(fd)
         raise

I found some corner case (rarely happens) in FileIO#__init__
Is this patch correct? Thank you.
msg118250 - (view) Author: Hirokazu Yamamoto (ocean-city) * (Python committer) Date: 2010-10-09 08:10
Here is the test script also run on py2.7.
msg120515 - (view) Author: Hirokazu Yamamoto (ocean-city) * (Python committer) Date: 2010-11-05 17:33
Recently, the patch to close fd when FileIO#__init__
failed and closefd = True was checked in. Is this mean
this issue is invalid?
msg161880 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-05-29 13:41
I think this issue is still valid. However, I’d favor the attached (simpler) patch that keeps track whether self->fd is our own (i.e. a result of an open() operation) or from outside.

A couple of notes:

 - I'm not sure why your test skipped if the Exception wasn't raised, I'd say that's a valid test fail.
 - The fd has been closed explicitly when seeking for append mode failed, so I removed it.
 - I'm not convinced it's worth/sensible to be back ported as it's a change in behavior.

Tests succeed on OS X & Linux.

Opinions? Win32 tests results?
msg162557 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-06-09 11:10
Anyone?
msg163274 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-06-20 12:39
In the test, assertNone is superfluous. Just call os.close().
However, you should probably use assertRaises to check that MyException is raised.
msg163360 - (view) Author: Roundup Robot (python-dev) Date: 2012-06-21 19:00
New changeset 981ad5254d07 by Hynek Schlawack in branch '2.7':
#10053: Don't close FDs when FileIO.__init__ fails
http://hg.python.org/cpython/rev/981ad5254d07

New changeset d042bd8625f3 by Hynek Schlawack in branch '3.2':
#10053: Don't close FDs when FileIO.__init__ fails
http://hg.python.org/cpython/rev/d042bd8625f3

New changeset 464cf523485e by Hynek Schlawack in branch 'default':
#10053: Don't close FDs when FileIO.__init__ fails
http://hg.python.org/cpython/rev/464cf523485e
msg163367 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-06-21 21:05
Should be fixed now.
msg163375 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-06-21 22:16
Shouldn’t the fix be ported to _pyio?
msg163384 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-06-22 05:53
Hmmm, I thought Lib/_pyio.py actually uses Lib/_io/_fileio.c? At least I can't find the logic inside. There's no error handling at all.

It just uses the FileIO object in Lib/_pyio.py:189 which it seems to get from Lib/_pyio.py:585 which I presumed was from Lib/_io/_fileio.c.

What am I missing and how would I be supposed to port that?
msg163728 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-06-24 06:35
I don’t know; Antoine is the io expert.  I just remember that io used to be in C and was moved to _pyio for documentation and experimentation purposes, so I was sure it was a fully standalone lib but I could be mistaken.
msg163938 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2012-06-25 11:19
Indeed, there is no pure-Python FileIO: _pyio.py imports it from C:
"from _io import FileIO"
History
Date User Action Args
2012-06-25 11:19:46amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg163938
2012-06-24 06:35:13eric.araujosetmessages: + msg163728
2012-06-22 05:53:16hyneksetmessages: + msg163384
2012-06-21 22:16:49eric.araujosetnosy: + eric.araujo
messages: + msg163375
2012-06-21 21:05:06hyneksetstatus: open -> closed
resolution: fixed
messages: + msg163367

stage: patch review -> resolved
2012-06-21 19:00:30python-devsetnosy: + python-dev
messages: + msg163360
2012-06-20 12:39:21pitrousetmessages: + msg163274
2012-06-09 11:10:53hyneksetmessages: + msg162557
2012-05-30 20:03:01hayposetnosy: + haypo
2012-05-29 13:41:19hyneksetfiles: + fileio-do-not-close-on-fail.diff
type: behavior
messages: + msg161880
2012-05-26 19:47:34pitrousetnosy: + hynek

versions: + Python 3.3, - Python 3.1
2010-11-05 17:33:37ocean-citysetmessages: + msg120515
2010-10-09 09:15:44eric.araujosetnosy: + pitrou

title: Probably fd should not be closed when FileIO#__init__ failed -> Don’t close fd when FileIO.__init__ fails
2010-10-09 08:10:12ocean-citysetfiles: + test_ensure_fd_not_closed_after_fileio_init_failed.py

messages: + msg118250
2010-10-09 08:08:18ocean-citycreate