classification
Title: array.fromfile not checking I/O errors
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 2.7, Python 2.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: pitrou Nosy List: BreamoreBoy, aguiar, chuck, ezio.melotti, pitrou
Priority: normal Keywords: easy, patch

Created on 2009-02-28 18:30 by aguiar, last changed 2010-07-21 16:54 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
array_ioerror.patch chuck, 2009-09-24 10:58
array_ioerror.patch chuck, 2009-10-04 11:03
array_ioerror.patch chuck, 2009-10-05 07:13
array_ioerror.patch chuck, 2009-10-06 15:09
Messages (16)
msg82936 - (view) Author: Eduardo Aguiar (aguiar) Date: 2009-02-28 18:30
At arraymodule.c (line 1258):

		nread = fread(item + (Py_SIZE(self) - n) * itemsize,
			      itemsize, n, fp);
		if (nread < (size_t)n) {
		  Py_SIZE(self) -= (n - nread);
			PyMem_RESIZE(item, char, Py_SIZE(self)*itemsize);
			self->ob_item = item;
			self->allocated = Py_SIZE(self);
			PyErr_SetString(PyExc_EOFError,
				         "not enough items in file");
			return NULL;
		}

When fread returns 0, ferror should be called to check if it was an EOF
or an error condition. It is not handling OSErrors, such as I/O errors,
raising always "not enough items in file".
msg93065 - (view) Author: Jan (chuck) * Date: 2009-09-24 10:58
I attached a path for raising IOErrors in fromfile. I also added a 
testcase which failed before.

The test opens a file and closes the file with os.close(fd) without 
telling the file object, so fromfile doesn't notice it's reading from a 
file that is actually closed.
msg93532 - (view) Author: Jan (chuck) * Date: 2009-10-04 11:03
Ezio, I moved the test to a separate method. Also I couldn't find 
something to close the file if I don't care about errors. I thought an 
assertRises would be wrong, as I am not debugging files here, so I added a 
function to call a callable I expect to throw an exception (you said you 
don't like the try/close/except/pass). I don't know if it is okay to add 
functions to test_support, though.
msg93534 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-10-04 11:17
There doesn't seem to be any reason to introduce the
"expect_exception()" helper, rather than to use a with statement. Am I
mistaken?
msg93536 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-10-04 11:21
Ok, I get it, you want f.close() to always succeed, even if the
underlying file descriptor has already been closed.
Well, I so no reason to introduce a helper anyway, the following four
lines are much more readable and explicit:

    try:
        f.close()
    except IOError:
        pass
msg93544 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2009-10-04 14:24
I tried to apply both the patches on the trunk but the tests don't pass.
With the latest patch I get an EOFError instead of IOError in the
assertRaises.

The function I was talking about was test_support.unlink(), but that
just removes the file, it doesn't close it, so the try/except is fine
when you close it (I also see that you already added
test_support.unlink() in the new patch).
I agree with Antoine that the helper function is not necessary.


A few more comments about the latest patch:
1) the try/except that imports the os module seems unnecessary to me;
2) if there are really cases where os is not available, the test should
be skipped with a message that says that the os module is not available
and that the test cannot be executed (with return the test is marked as
'passed' even if nothing is actually tested);
3) if os.close(f.fileno()) is required instead of a simpler f.close()
please write a short comment to clarify why;
4) if there are cases where EOFError is raised and they are not tested,
it would be nice to add another test that checks if/when EOFError is raised.
msg93577 - (view) Author: Jan (chuck) * Date: 2009-10-05 07:13
1&2) I removed the try/except around the import. I have no clue if os 
might be unavailable. Maybe leave out handling that until we see that 
breaking.

I added the try/except because I saw that in other tests in the same 
file when importing gc.

3) Done.
4) The EOFError exceptions are tested in test_tofromfile.

> I tried to apply both the patches on the trunk but the tests don't
> pass. With the latest patch I get an EOFError instead of IOError in
> the assertRaises.
That is the also behaviour before the patch, so the ferror(fp) does not 
fire. Either the test is inappropriate or your system doesn't regard 
reading a closed filehandle an error. I'm not able to investigate this 
as it works fine on my os x 10.6. What system did you test it on?

Reliable ways of producing IOErrors are harder to find than I thought. 
Deleting the file between to reads makes it just look truncated. Another 
method I tried was crashing a process which holds the other end of a 
pipe, but that's messy and complicated.
msg93626 - (view) Author: Eduardo Aguiar (aguiar) Date: 2009-10-05 21:40
Maybe you could create a file without read permission (000) and try
to read from it.
msg93632 - (view) Author: Jan (chuck) * Date: 2009-10-06 06:24
> Maybe you could create a file without read permission (000) and try
> to read from it.

I just checked. If I don't have read permissions, I am not able to open 
the file. When I open a file and change permissions afterwards, I can read 
the complete file alright. (Checked with a 4mb file, I hope that's beyond 
buffering.) So I guess permissions are only checked when you open the 
file.
msg93649 - (view) Author: Eduardo Aguiar (aguiar) Date: 2009-10-06 13:50
Another try. I have opened a file for writing, and have tried to read
from it:

>>> fp = open ('xxx', 'w')
>>> fp.read ()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
IOError: [Errno 9] Bad file descriptor
msg93652 - (view) Author: Jan (chuck) * Date: 2009-10-06 15:09
Thanks Aduardo! (I could have sworn I tried that.) I changed the test to 
reading from a file in 'wb' mode, which raised a EOFError before and now 
raises IOErrors.
msg111080 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2010-07-21 16:00
With the latest patch on Windows Vista against 2.7 I got 12 EOFError errors instead of IOError.
msg111084 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2010-07-21 16:30
Sorry for the noise, forgot to rebuild the code.  The tests run fine 543 tests ok, except I note that all the output is repeated 6 times, I don't understand this at all.
msg111086 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-07-21 16:39
Will polish the patch and commit.
msg111088 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-07-21 16:54
I've committed the new test in py3k (r83030) and 3.1 (r83033), and the full patch in 2.7 (r83031) and 2.6 (r83032). Thank you!
msg111090 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-07-21 16:54
Regarding:
> The tests run fine 543 tests ok, except I note that all the output is
> repeated 6 times, I don't understand this at all.

This is normal, all array tests are run once per array type, and 6 different array types are being tested.
History
Date User Action Args
2010-07-21 16:54:57pitrousetmessages: + msg111090
2010-07-21 16:54:15pitrousetstatus: open -> closed
versions: + Python 2.6, - Python 3.1, Python 3.2
messages: + msg111088

resolution: accepted -> fixed
stage: commit review -> resolved
2010-07-21 16:39:37pitrousetassignee: pitrou
resolution: accepted
messages: + msg111086
stage: patch review -> commit review
2010-07-21 16:30:09BreamoreBoysetmessages: + msg111084
2010-07-21 16:00:56BreamoreBoysetnosy: + BreamoreBoy

messages: + msg111080
versions: + Python 3.1
2009-10-06 15:09:20chucksetfiles: + array_ioerror.patch

messages: + msg93652
2009-10-06 13:50:35aguiarsetmessages: + msg93649
2009-10-06 06:24:34chucksetmessages: + msg93632
2009-10-05 21:40:59aguiarsetmessages: + msg93626
2009-10-05 07:13:41chucksetfiles: + array_ioerror.patch

messages: + msg93577
2009-10-04 14:24:43ezio.melottisetmessages: + msg93544
2009-10-04 11:21:28pitrousetmessages: + msg93536
2009-10-04 11:17:43pitrousetversions: + Python 2.7, Python 3.2, - Python 2.6
nosy: + pitrou

messages: + msg93534

stage: test needed -> patch review
2009-10-04 11:03:48chucksetfiles: + array_ioerror.patch

messages: + msg93532
2009-10-03 22:08:11ezio.melottisetnosy: + ezio.melotti
2009-09-24 10:58:42chucksetfiles: + array_ioerror.patch

nosy: + chuck
messages: + msg93065

keywords: + patch
2009-04-22 14:39:28ajaksu2setpriority: normal
keywords: + easy
stage: test needed
2009-02-28 18:30:22aguiarcreate