Skip to content
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

array.fromfile not checking I/O errors #49645

Closed
aguiar mannequin opened this issue Feb 28, 2009 · 16 comments
Closed

array.fromfile not checking I/O errors #49645

aguiar mannequin opened this issue Feb 28, 2009 · 16 comments
Assignees
Labels
easy stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@aguiar
Copy link
Mannequin

aguiar mannequin commented Feb 28, 2009

BPO 5395
Nosy @pitrou, @ezio-melotti
Files
  • array_ioerror.patch
  • array_ioerror.patch
  • array_ioerror.patch
  • array_ioerror.patch
  • 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:

    assignee = 'https://github.com/pitrou'
    closed_at = <Date 2010-07-21.16:54:15.051>
    created_at = <Date 2009-02-28.18:30:22.822>
    labels = ['easy', 'type-bug', 'library']
    title = 'array.fromfile not checking I/O errors'
    updated_at = <Date 2010-07-21.16:54:57.978>
    user = 'https://bugs.python.org/aguiar'

    bugs.python.org fields:

    activity = <Date 2010-07-21.16:54:57.978>
    actor = 'pitrou'
    assignee = 'pitrou'
    closed = True
    closed_date = <Date 2010-07-21.16:54:15.051>
    closer = 'pitrou'
    components = ['Library (Lib)']
    creation = <Date 2009-02-28.18:30:22.822>
    creator = 'aguiar'
    dependencies = []
    files = ['14963', '15037', '15046', '15055']
    hgrepos = []
    issue_num = 5395
    keywords = ['patch', 'easy']
    message_count = 16.0
    messages = ['82936', '93065', '93532', '93534', '93536', '93544', '93577', '93626', '93632', '93649', '93652', '111080', '111084', '111086', '111088', '111090']
    nosy_count = 5.0
    nosy_names = ['pitrou', 'aguiar', 'ezio.melotti', 'chuck', 'BreamoreBoy']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue5395'
    versions = ['Python 2.6', 'Python 2.7']

    @aguiar
    Copy link
    Mannequin Author

    aguiar mannequin commented Feb 28, 2009

    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".

    @aguiar aguiar mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Feb 28, 2009
    @devdanzin devdanzin mannequin added the easy label Apr 22, 2009
    @chuck
    Copy link
    Mannequin

    chuck mannequin commented Sep 24, 2009

    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.

    @chuck
    Copy link
    Mannequin

    chuck mannequin commented Oct 4, 2009

    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.

    @pitrou
    Copy link
    Member

    pitrou commented Oct 4, 2009

    There doesn't seem to be any reason to introduce the
    "expect_exception()" helper, rather than to use a with statement. Am I
    mistaken?

    @pitrou
    Copy link
    Member

    pitrou commented Oct 4, 2009

    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
    

    @ezio-melotti
    Copy link
    Member

    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.

    @chuck
    Copy link
    Mannequin

    chuck mannequin commented Oct 5, 2009

    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.

    1. Done.
    2. 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.

    @aguiar
    Copy link
    Mannequin Author

    aguiar mannequin commented Oct 5, 2009

    Maybe you could create a file without read permission (000) and try
    to read from it.

    @chuck
    Copy link
    Mannequin

    chuck mannequin commented Oct 6, 2009

    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.

    @aguiar
    Copy link
    Mannequin Author

    aguiar mannequin commented Oct 6, 2009

    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

    @chuck
    Copy link
    Mannequin

    chuck mannequin commented Oct 6, 2009

    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.

    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Jul 21, 2010

    With the latest patch on Windows Vista against 2.7 I got 12 EOFError errors instead of IOError.

    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Jul 21, 2010

    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.

    @pitrou
    Copy link
    Member

    pitrou commented Jul 21, 2010

    Will polish the patch and commit.

    @pitrou pitrou self-assigned this Jul 21, 2010
    @pitrou
    Copy link
    Member

    pitrou commented Jul 21, 2010

    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!

    @pitrou pitrou closed this as completed Jul 21, 2010
    @pitrou
    Copy link
    Member

    pitrou commented Jul 21, 2010

    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.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    easy stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants