readline: clear the error flag if the error happens to be EAGAIN

[tonycoz]

(or the equivalent EWOULDBLOCK)

This allows questionable code that tries to combine select and the
readline flavour of buffered I/O to limp along. Such code is still
risky due to select() checking the underlying OS handle and not
the perl handle.

Fixes #22883

---

• This set of changes requires a perldelta entry, and it is included.

[toddr]

@tonycoz Thank you for this! Looks like there's a rebase needed to get this merged.

[Leont]

This would mean that suddenly readline returning undef doesn't always mean EOF. That could be quite confusing.

[jkeenan]

@tonycoz @toddr, Could you evaluate @Leont's comment from last month? It's not clear to me that this p.r. should be in 'Approved' status. Thanks.

[tonycoz]

@book, @ap , @haarg I request the guidance of the PSC.

In response to #20060, in #20103 I changed the error handling for readline(), this was applied for perl 5.38 and was documented in perl5380delta.

Previously it would clear the error flag on the stream on pretty much any error, so if there was any sort of error reading from the stream the error flag for the stream would simply be cleared. This was changed to clear the error state in much more limited cases.

This can include errors like trying to read from an output handle, reading from a handle opened to a directory and EWOULDBLOCK errors on sockets marked as non-blocking. This could also include hard errors like EIO

Note that readline() was the only read op that cleared the error state like this, using read() or getc() instead does not clear the stream error state.

This has caused some breakage downstream, including each of the soft error cases above (#22883 #20376 #21240)

In this particular PR after some discussion in #22883 I suggested only ignoring the non-blocking file handle errors, and this PR implements that, though mixing buffered I/O and select() is generally a mistake.

So what I'm looking for here is: should readline() revert to clearing the error state? (and inconsistency with read(), getc())

If not, should the change here be applied, clearing the error state only on EWOULDBLOCK/EAGAIN?

[ap]

We talked this through at the in-person PSC meeting at the PTS today.

So what I'm looking for here is: should readline() revert to clearing the error state? (and inconsistency with read(), getc())

No. That would just resurrect #20060. Fixing that by removing the error-clearing from the end of readline was correct. The previous situation was already conceptually wrong, but that was much less obvious before we addressed #20060.

Namely, the conceptual problem is that, in code which does a whole sequence of I/O operations without checking for errors, and then does an I/O operation after which it does check for errors, that error check can run into an error from any of the preceding I/O operations, even though it’s (99.999% of the time anyway) written to check for errors of only the last operation.

So what we think should be done about #22883 in conjunction with #20060 is that every I/O operation needs to be preceded by clearing the error state. Then readline won’t need to do anything special about EAGAIN and EWOULDBLOCK either, since in those cases it proceeds to do another read internally anyway, prior to which is going to clear the error state, which handles those cases implicitly. (This would automatically keep it correct, for free, even if handling of any new conditions ever were to be added in future – not that that’s likely, just as evidence of conceptual correctness.)

Does that sound right? Are we missing anything @tonycoz?

[tonycoz]

So what we think should be done about #22883 in conjunction with #20060 is that every I/O operation needs to be preceded by clearing the error state. Then readline won’t need to do anything special about EAGAIN and EWOULDBLOCK either, since in those cases it proceeds to do another read internally anyway, prior to which is going to clear the error state, which handles those cases implicitly. (This would automatically keep it correct, for free, even if handling of any new conditions ever were to be added in future – not that that’s likely, just as evidence of conceptual correctness.)

If I understand what you're suggesting, if the user does:

# assuming the output buffer is full or close to it here
print "Hello"; # disk is full, this fails, some output discarded
# some disk space freed here
print "Goodbye"; # error flag cleared, space available, so this succeeds

In particular, if this happened for a perl -pe '....' if the last print succeeded, we'd silently produce a corrupt output file, which is why the error flag is normally preserved.

[book]

In particular, if this happened for a perl -pe '....' if the last print succeeded, we'd silently produce a corrupt output file, which is why the error flag is normally preserved.

$ perl -MO=Deparse -p -e '...'
LINE: while (defined($_ = readline ARGV)) {
    die 'Unimplemented';
}
continue {
    die "-p destination: $!\n" unless print $_;
}
-e syntax OK

So, actually, -p does a print or die, and wouldn't cause corruption in combination with -i.

The issue remains for everyone not checking the return value of a print, though. At least Perl does the right thing.

[tonycoz]

For history, making the success of close depend on the error state was done just post 5.8.0, though I don't see anything in the 5.8.1 or 5.10.0 perldeltas.

It was originally proposed in #7637 (transferred from RT) and applied as part of a batch of patches originally made to debian.

From the original post the intent appears to be to ensure that write failures were preserved, so I don't think a general "clear errors every I/O" is the solution.

e199e3b - the patch to make close fail if the error flag is set
https://www.nntp.perl.org/group/perl.perl5.porters/2005/03/msg98839.html - applied
#7637 - original RT ticket
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=285435 - debian ticket

I spent some time looking through this and noticed in PerlIOUnix_read():

        if (len >= 0 || errno != EINTR) {
            if (len < 0) {
                if (errno != EAGAIN) {
                    PerlIOBase(f)->flags |= PERLIO_F_ERROR;
                    PerlIO_save_errno(f);
                }
            }
            else if (len == 0 && count != 0) {
                PerlIOBase(f)->flags |= PERLIO_F_EOF;
                SETERRNO(0,0);
            }
            return len;
        }

which prevents setting the error flag if the error was EAGAIN. This was a fix for #6062

This was added in ba85f2e but only modified PerlIOUnix_read() (and didn't include a test), not the higher layers, if I look at PerlIOBuf_fill():

    if (avail <= 0) {
        if (avail == 0)
            PerlIOBase(f)->flags |= PERLIO_F_EOF;
        else
        {
            PerlIOBase(f)->flags |= PERLIO_F_ERROR;
            PerlIO_save_errno(f);
        }
        return -1;
    }

which doesn't exclude EAGAIN errors from setting the stream error flag. So what if we change that:

diff --git a/perlio.c b/perlio.c
index 97415f8fa8..ade240de74 100644
--- a/perlio.c
+++ b/perlio.c
@@ -4466,8 +4466,7 @@ PerlIOBuf_fill(pTHX_ PerlIO *f)
     if (avail <= 0) {
         if (avail == 0)
             PerlIOBase(f)->flags |= PERLIO_F_EOF;
-        else
-        {
+        else if (errno != EAGAIN) {
             PerlIOBase(f)->flags |= PERLIO_F_ERROR;
             PerlIO_save_errno(f);
         }

All tests successful, what happens on a EAGAIN read?

tony@venus:.../git/perl6$ cat ../22883b.pl
use v5.40.0;
pipe(my $in, my $out);

$in->blocking(0) >= 0 or die "Blocking: $!";
$! = 0;
my $x = <$in>;
say defined $x ? "<$x>" : "undef";
say $in->error;
say $!;
tony@venus:.../git/perl6$ ~/perl/blead/bin/perl5.41.11 ../22883b.pl 
undef
1
Resource temporarily unavailable
tony@venus:.../git/perl6$ ./perl -Ilib ../22883b.pl 
undef
0
Resource temporarily unavailable

So readline still returns the error, the error flag isn't set and it matches the apparent intent of ba85f2e.

Still needs tests of some sort.

Neither this change nor this PR fixes #23026