Description
After upgrade from perl 5.36 to 5.40, we began encountering issues with code that used Git::Repository. This module uses System::Command to interface with the git command line binary. We implemented a modified $git->run, which does the following:
my $select = IO::Select->new;
foreach my $fh ( $stdout, $stderr ) {
$fh->blocking(0);
$select->add($fh);
}
while ( $select->count ) {
my @ready = $select->can_read;
foreach my $fh (@ready) {
if ( eof $fh ) {
$select->remove($fh);
}
else {
${ $data->{$fh} } .= $_ while (<$fh>);
}
}
}
This code is pretty much straight out of the documentation example in IO::Select.
During cleanup, System::Command does the following to its handles for $stdin
/$stdout
/$stderr
and this is causing a lot of noise right now for us:
sub close {
my ($self) = @_;
# close all pipes
my ( $in, $out, $err ) = @{$self}{qw( stdin stdout stderr )};
$in and $in->opened and $in->close || carp "error closing stdin: $!";
$out and $out->opened and $out->close || carp "error closing stdout: $!";
$err and $err->opened and $err->close || carp "error closing stderr: $!";
# and wait for the child (if any)
$self->_reap();
return $self;
}
It's not uncommon to see this on CPAN, and it's even worse: We see close $fh or die...
, which means all of this code now unexpectedly blows up.
Digging deeper into our problem, we determined that the IO::Select
method can_read
is causing EAGAIN
(or is it EWOULDBLOCK
? They're the same error) to be put as an error on the file handle, which now causes close to exit non-zero.
In the change introduced by #20103, which addressed issues raised by #20060, they were concerned with losing errors on the file handle during close. While I can see some value in EISDIR being retained, I struggle to understand why we would retain ALL of the errors, especially if there is a successful read afterwards.
I would prefer #20103 to be reverted, but I think we're past that, so I want to suggest an alternative to @tonycoz, who worked on this originally: Possibly, we could restore the PerlIO_clearerr
unless the error set are specific ones we care about. In my specific case, I would argue that EAGAIN should be cleared if there is later a successful read, right?