This patch is to fix bug 1523853.
Now file.read() works as written in the doc, which says
"An empty string is returned when EOF is encountered immediately."
Before this fix, An empty string was returned even when the last
read() encountered EOF.

I was not able to reproduce the problem.  I tried to read through the end of the file, append to the file from an external process and read again.  It worked as expected: more data was read.

The proposed patch seems to be redundant: once ferror(f->f_fp) is known to be 0, clearerr will do nothing and in the alternative branch clearerr is already there. 

I can easily reproduce this problem on OS X 10.4.9 using Python 2.4.3 and Python 2.5.

Here's the code
#################
import os, sys
filename = "spam"

f = open(filename, "w+")
f.seek(0, 2)

line = f.read() # EOF

# Writing the same file using another fd
f2 = open(filename, "a+")
f2.write("Spam")
f2.flush()
f2.close()

statinfo = os.stat(filename)
print "file size: %d" % statinfo.st_size
print "position : %d" % f.tell()
line = f.read() # read() returns emtpy!! readlines?() works ok
print "line     : [%s]" % line
#################

On my machine, it outputs the following
###
file size: 4
position : 0
line     : []
###

And just now  I found that on ubuntu on the same machine (vmware),
it works collect.
###
file size: 4
position : 0
line     : [Spam]
###

My patched version python outputs the same result as above on OS X.

We need clearerr() there, too because clearerr()'s job is not only only clearing error indicators
but EOF indicators also.

Hmm. It looks like glibc's fread will attempt reading even if EOF flag is already set. 

I've reproduced the problem on OS X 10.4.9 and Python 2.6a0 (trunk:54926M).

This patch fixes it.

I confirmed that fread can read data from fp in EOF state on Linux. On OS X, freading from fp in EOF state doesn't work,
just return 0.

I tested this behavior using the following code.
----------------------------

#include <stdio.h>
#include <stdlib.h>

main(void)
{
  FILE *fp1, *fp2;
  int n;
  char buf[128];

  fp1 = fopen("spam.txt", "w+");
  fputs("foo", fp1);

  fseek(fp1, 0L, SEEK_END);
  if (fread(buf, 1, sizeof(buf), fp1) == 0)
    fprintf(stderr, "first fread failed\n");

  fp2 = fopen("spam.txt", "a+");
  fputs("bar", fp2);
  fclose(fp2);

  if (feof(fp1))
    fprintf(stderr, "after appending some text, fp1 is still eof\n");

  if (fread(buf, 1, sizeof(buf), fp1) == 0)
    fprintf(stderr, "second fread failed\n");
  printf("buf: %s\n", buf);

  fclose(fp1);

  return 0;
}
----------------------------
=============
On Linux
=============
first fread failed
after appending some text, fp1 is still eof
buf: bar

=============
On OS X
=============
first fread failed
after appending some text, fp1 is still eof
second fread failed
buf: 

Anyway, I think it's safe and preferable to clear EOF indicator in this case.

I have just verified that this issue affects readlines and readinto methods on Mac OS X as well.  This tells me that rather than patching all the individual methods, maybe Py_UniversalNewlineFread should be fixed instead.  I believe that the current situation where a function that looks like Python API reports errors by setting flags on a FILE object instead of settin the an exception is against python's design.  I suggest changing Py_UniversalNewlineFgets so that if fread returns 0, it checks whether that was due to EOF or an error and sets an exception in a case of an error and always calls clearerr.

New patch Attached.
I believe this one includes all belopolsky's suggestions.

Now all dirty jobs are done in Py_UniversalNewlineFread:
1. calls clearerr if ferror or feof
2. set IOErrorr exception and return

The caller just checks whether IOError is occurred or not
and does appropriate jobs.

The attachment also contains unittest for this problem.

File Added: eof.diff

The latest patch works on Mac OS 10.4 . I would suggest a few cosmetic changes:

1. Put the flag checking logic in a separate function and call it instead of fread.  This will eliminate code duplication.
2. "else" is redundant in

+                       if (ferror(stream)) {
+                               clearerr(stream);
+                               PyErr_SetFromErrno(PyExc_IOError);
+                               return 0;
+                       } else if (feof(stream)) {
+                               clearerr(stream);
+                       }

The flow will be more obvious if you write

+                       if (ferror(stream)) {
+                               clearerr(stream);
+                               PyErr_SetFromErrno(PyExc_IOError);
+                               return 0;
+                       } 
+                       if (feof(stream)) {
+                               clearerr(stream);
+                       }

3. Please mention bug 1523853 in a comment near the test.  

otherwise, the patch looks fine.

The flag checking is revised.
I removed feof() so the checking logic become shorter.
I didn't factor out that logic 
because I think the logic is simple enough

Added a comment that mentions bug 1523853.


File Added: eof2.diff

Could someone please review my patch and give me some feedback?

Any chance to get this fix commmited in?

I get an error when I try to read 1523853. Is this not moved to the new
tracker or others are able to access it?

Yes, a number of items were not moved, as SF failed to provide them on
export.

jos, can you please provide a real name in "Your Details" of this
tracker? We cannot accept anonymous/pseudonymous patches.

What's in a name? :p
Done, anyway.

Hi, when will this patch be checkedin?

Thanks for the patch. Committed into the 2.5 branch as r67740. This
still needs to be applied to the other branches.

Unfortunately, the patch is broken. The program

fname='test123'
f=open(fname,'w')
f.read()

crashes with the patch applied.

I think I will revert the patch in 2.5.3, release 2.5.4, and reject the
patch.

It isn't being careful when calling PyErr_SetFromErrno inside the
Py_UniversalNewlineFread function since this function is being called
all over fileobject after releasing the GIL.. so, isn't this just a
matter of adding pairs of PyGILState_Ensure/PyGILState_Release around
these calls to PyErr_SetFromErrno in this specific function ?

> It isn't being careful when calling PyErr_SetFromErrno inside the
> Py_UniversalNewlineFread function since this function is being called
> all over fileobject after releasing the GIL.. so, isn't this just a
> matter of adding pairs of PyGILState_Ensure/PyGILState_Release around
> these calls to PyErr_SetFromErrno in this specific function ?

Perhaps that could fix this problem (or perhaps not - is
PyGILState_Ensure guaranteed to do the right thing, even in the
presence of multiple interpreters?)

My concern is that this patch may contain other bugs. I am apparently
incapable of reviewing it correctly, and nobody else has offered a
careful review.

On Sun, Dec 21, 2008 at 9:52 PM, Martin v. Löwis <report@bugs.python.org> wrote:
>
> Martin v. Löwis <martin@v.loewis.de> added the comment:
>
>> It isn't being careful when calling PyErr_SetFromErrno inside the
>> Py_UniversalNewlineFread function since this function is being called
>> all over fileobject after releasing the GIL.. so, isn't this just a
>> matter of adding pairs of PyGILState_Ensure/PyGILState_Release around
>> these calls to PyErr_SetFromErrno in this specific function ?
>
> Perhaps that could fix this problem (or perhaps not - is
> PyGILState_Ensure guaranteed to do the right thing, even in the
> presence of multiple interpreters?)

It is said to be unsupported in that case, but I guess you knew that.

I believe the original patch is overreaching. I believe the changes to
fileobject.c should've been much simpler, just as the title describes
(adding clearerr() just before performing reads on the FILE handles).

I've attached a much simpler patch that I believe corrects the issue,
but I don't have an OS X platform to try it on.

I have now reverted the patch in r67914. I won't reject the patch
because of Scott's alternative, but leave it open for review. Since
Scott's patch is not approved yet, this is not a release blocker anymore.

I've combined Scott's patch with John's (?) test case in issue1706039.diff.  Confirmed that the patch fixes the issue on Mac OS X 
10.5.6 (Intel).

Maybe not a problem, but an inconsistency: in Py_UniversalNewlineFgets 
clearerr is added inside univ_newline conditional and then again before 
the loop over characters, but in Py_UniversalNewlineFread it is added only 
once before "if (!f->f_univ_newline)".  I am not sure which approach is 
right, but I don't think there is a reason for the two functions to be 
different.

Sorry for inconvenience caused with the patch.
I should've got more reviews before asking it checked in.

I verified eof2.diff-applied-python gets "Bus error"
and Scott Dial's fileobject.diff fixes this.
(Test was on 9.6.0 Darwin Kernel Version 9.6.0)

They differ because in Py_UniversalNewlineFgets() there is a call to
FLOCKFILE(), and it seemed appropriate to ensure that clearerr() was
called after locking the FILE stream. I certainly pondered over whether
it made a difference to do it before or after, and it would seem to me
that if we are bothering to lock the FILE stream then we would care to
ensure that the first GETC() after locking reaps the benefit of having
called clearerr(). In the cases that we fall into fread and fgets, we
may actually need to be doing:

    FLOCKFILE(stream)
    clearerr(stream);
    result = fgets(buf, n, stream);
    FUNLOCKFILE(stream);
    return result;

One nitpick is that the testcase was appended to the StdoutTests class
instead of OtherFileTests class, which would seem more appropriate.

Does it apply to 3.1?