Skip to content

array.fromfile not checking I/O errors #49645

Closed
Closed
array.fromfile not checking I/O errors#49645
Assignees
pitrou
Labels
easystdlibStandard Library Python modules in the Lib/ directorytype-bugAn unexpected behavior, bug, or error

Description

@aguiar
aguiar
mannequin
opened on 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']

    Activity

    1. aguiar commented on Feb 28, 2009

      @aguiar
      aguiarmannequin
      on Feb 28, 2009
      MannequinAuthor

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

    2. aguiar
      added
      stdlibStandard Library Python modules in the Lib/ directory
      type-bugAn unexpected behavior, bug, or error
      on Feb 28, 2009
    3. devdanzin
      added
      easy
      on Apr 22, 2009

    4. chuck commented on Sep 24, 2009

      @chuck
      chuckmannequin
      on Sep 24, 2009
      Mannequin

      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.

    5. chuck commented on Oct 4, 2009

      @chuck
      chuckmannequin
      on Oct 4, 2009
      Mannequin

      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.

    6. pitrou commented on Oct 4, 2009

      @pitrou
      pitrou
      on Oct 4, 2009
      Member

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

    7. pitrou commented on Oct 4, 2009

      @pitrou
      pitrou
      on Oct 4, 2009
      Member

      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

    8. ezio-melotti commented on Oct 4, 2009

      @ezio-melotti
      ezio-melotti
      on Oct 4, 2009
      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.

    9. chuck commented on Oct 5, 2009

      @chuck
      chuckmannequin
      on Oct 5, 2009
      Mannequin

      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.

    10. aguiar commented on Oct 5, 2009

      @aguiar
      aguiarmannequin
      on Oct 5, 2009
      MannequinAuthor

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

    11. chuck commented on Oct 6, 2009

      @chuck
      chuckmannequin
      on Oct 6, 2009
      Mannequin

      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.

    12. aguiar commented on Oct 6, 2009

      @aguiar
      aguiarmannequin
      on Oct 6, 2009
      MannequinAuthor

      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

    13. chuck commented on Oct 6, 2009

      @chuck
      chuckmannequin
      on Oct 6, 2009
      Mannequin

      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.

    14. BreamoreBoy commented on Jul 21, 2010

      @BreamoreBoy
      BreamoreBoymannequin
      on Jul 21, 2010
      Mannequin

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

    15. BreamoreBoy commented on Jul 21, 2010

      @BreamoreBoy
      BreamoreBoymannequin
      on Jul 21, 2010
      Mannequin

      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.

    16. pitrou commented on Jul 21, 2010

      @pitrou
      pitrou
      on Jul 21, 2010
      Member

      Will polish the patch and commit.

    17. pitrou
      self-assigned this
      on Jul 21, 2010

    18. pitrou commented on Jul 21, 2010

      @pitrou
      pitrou
      on Jul 21, 2010
      Member

      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!

    19. pitrou
      closed this as completedon Jul 21, 2010

    20. pitrou commented on Jul 21, 2010

      @pitrou
      pitrou
      on Jul 21, 2010
      Member

      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.

    21. ezio-melotti
      transferred this issue fromon Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Metadata

    Metadata

    Assignees

    Labels

    easystdlibStandard Library Python modules in the Lib/ directorytype-bugAn unexpected behavior, bug, or error

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions