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
io close() swallowing exceptions #52113
Comments
The current semantic of io streams is to swallow IOErrors on close(), eg. in _pyio from the trunk, we have each time constructs like: and in C files :
/* If flush() fails, just give up */
if (PyErr_ExceptionMatches(PyExc_IOError))
PyErr_Clear(); I'd rather advocate letting exceptions flow, as users have the right to know if their io operations lost bytes. PS : issues like http://bugs.python.org/issue7640 are actually much more crucial, so we shall let them higher priority |
Wouldn't this break code that currently works? |
Well, it would break code which currently ignores that it fails, so it's more a benefit than a loss for programmers imo. I doubt the impact will be important though, because the io module is still quite recent, and furthermore errors on the last flush are quite unlikely to happen if previous ones succeeded (except "disk full", I don't see any reason for this to happen). |
You're right that silencing IO errors is bad. Patch welcome. |
Patch and test to stop swallowing exceptions on stream close(), for python SVN trunk. |
The tests should probably check all three types of I/O (raw, buffered, text). |
SHould be better this way then B-) |
I just tried the patch. One problem is that you are supposed to be able to call close() several times without having it fail: >>> f = open("LICENSE")
>>> f.close()
>>> f.close()
>>> f = io.open("LICENSE")
>>> f.close()
>>> f.close()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ValueError: I/O operation on closed file. This means your patch should be a little smarter. |
I'm quite surprised it wasn't already covered by the test suite :S Anyway I'm quite confused about the semantic which is expected from IO operations... Should a flush on a closed stream fail (at the moment sometimes it does, sometimes doesn't) ? Why is sometimes ValueError used when I'd rather expect an IOError ? |
Probably an oversight. Do you want to add some tests?
It probably should, yes.
Because it's not an IO error at all. No I/O occurs. You are just using |
That's WIP
Then when you try to wrap a non-readable stream into a readable buffered stream (like BufferedRWPair), it should raise a value error as well, but currently it's rather: |
For what it's worth, I documented the possibility to call close() several times in r80592.
Good point. Unfortunately, it's now a bit late to change this. |
Here is a code/test patch which *should* fix the multiple close() case, and ensure flush() raise an error on closed file. All IO test suites pass on my win32 (except test_largefile that I skip due to lack of hdd space). I've noticed that the detach() semantic was quite different between _pyio and _io, by the way: C code raises valueerror when we try to access the stream after detaching it, whereas python lets attribute errors be raised. But maybe this is not so important, as these are programming errors anyway. One thing I'm still wondering : why couldn't we obtain these C extension by cythonizing _pyio ? Are there features that cython lacks, or optimization considerations I'm not aware of ? Cython-generated extensions seem soooo easier to maintain... |
Agreed :)
Several reasons:
|
Thanks for the patch! It was committed in r80720 (trunk), r80721 (2.6), r80722 (py3k), r80723 (3.1). |
Cool, thanks a lot B-) |
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: