On Oct 30, 2014, at 15:08, Eric Wong wrote:
For a (currently) unknown reason, Git::SVN::Fetcher::close_file
sometimes triggers "Bad file descriptor" errors on syswrite when
writing symlink contents on the "svn_hash" tempfile.
The current IO::Handle::opened call in Git.pm is not a
sufficient check, as the underlying file descriptor is closed
without the PerlIO layer knowing about it. This is likely a bug
inside libsvn (1.6.17), as none of the Git.pm or Git::SVN
modules close IOs without the knowledge of the PerlIO layer.
Cc: Kyle J. McKay <mackyle@xxxxxxxxx>
Signed-off-by: Eric Wong <normalperson@xxxxxxxx>
---
Kyle/Junio: thoughts? I'm running out of time to track this down
so it might be necessary for 2.2-rc0. What's even stranger is
I cannot always reproduce the problem even without this patch,
so it may be only triggered by network latency...
Thanks.
With this patch added, do you then see the warning:
carp "Temp file '", $name,
"' was closed. Opening replacement.";
From _temp_cache? It would seem odd if you didn't unless there was
only one symlink involved.
I suspect that symlinks were rare in the repositories I was testing
against. I wonder if having a test svn repo that adds several new
symlinks for several revisions in a row might trigger this problem
more robustly.
The _repository->temp_acquire('svn_hash') call is assigned to a "my"
variable and then has Git::temp_release called on it later in the same
function and the only calls made on it in between are Git::temp_path,
so I don't see how the libsvn library could be responsible for closing
it since it looks to me like it never sees it. But I'm looking at
v2.1.2 of Fetcher.pm, so if some other calls have been inserted there
since then.
perl/Git.pm | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/perl/Git.pm b/perl/Git.pm
index b5905ee..f1f210b 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -99,7 +99,7 @@ increase notwithstanding).
=cut
-use Carp qw(carp croak); # but croak is bad - throw instead
+use Carp qw(carp croak cluck); # but croak is bad - throw instead
use Error qw(:try);
use Cwd qw(abs_path cwd);
use IPC::Open2 qw(open2);
@@ -1206,6 +1206,16 @@ sub temp_acquire {
$temp_fd;
}
+sub _temp_really_open {
+ my ($fh) = @_;
+
+ if (defined($fh) && $fh->opened) {
+ return 1 if stat($fh);
+ cluck("$fh closed independently of PerlIO\n");
+ }
+ return 0;
+}
+
=item temp_is_locked ( NAME )
Returns true if the internal lock created by a previous
C<temp_acquire()>
@@ -1232,7 +1242,7 @@ sub temp_is_locked {
my ($self, $name) = _maybe_self(@_);
my $temp_fd = \$TEMP_FILEMAP{$name};
- defined $$temp_fd && $$temp_fd->opened && $TEMP_FILES{$$temp_fd}
{locked};
+ _temp_really_open($$temp_fd) && $TEMP_FILES{$$temp_fd}{locked};
}
=item temp_release ( NAME )
@@ -1264,7 +1274,7 @@ sub temp_release {
carp "Attempt to release temp file '",
$temp_fd, "' that has not been locked";
}
- temp_reset($temp_fd) if $trunc and $temp_fd->opened;
+ temp_reset($temp_fd) if $trunc && _temp_really_open($temp_fd);
$TEMP_FILES{$temp_fd}{locked} = 0;
undef;
@@ -1276,7 +1286,7 @@ sub _temp_cache {
_verify_require();
my $temp_fd = \$TEMP_FILEMAP{$name};
- if (defined $$temp_fd and $$temp_fd->opened) {
+ if (_temp_really_open($$temp_fd)) {
if ($TEMP_FILES{$$temp_fd}{locked}) {
throw Error::Simple("Temp file with moniker '" .
$name . "' already in use");
--
EW
The changes themselves look okay, but eeewwww. I don't see how libsvn
can see the svn_hash fd to close it unless it's randomly closing fds.
--Kyle
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html