[PATCH 0/2] git-svn: fixes for intermittent SIGPIPE

[PATCH 0/2] git-svn: fixes for intermittent SIGPIPE

[Roman Kagan]

In my work environment subversion is still being used as the main
revision control system.  Therefore many people who prefer to work with
git have to resort to git-svn.

However, in many configurations it used to suffer from the notorious
"git-svn died of signal 13" problem (see e.g.
http://thread.gmane.org/gmane.comp.version-control.git/134936 and the
links therein).

I believe to have tracked down the issue to the connection being closed
by the server when http keep-alive is in use, and the client dying on
SIGPIPE because its handler is left at SIG_DFL when a new request is
being made.

The patches have been tested on

- Linux Fedora 16 x86_64, git 1.7.7.6, perl v5.14.2, svn 1.6.17
- Windows 7 x64 + Cygwin, git 1.7.9, perl v5.10.1, svn 1.7.4,
- Windows 7 x64 + MsysGit, git 1.7.9.msysgit.0, perl v5.8.8, svn 1.4.6

Roman Kagan (2):
  git-svn: use POSIX::sigprocmask to block signals
  git-svn: ignore SIGPIPE

 git-svn.perl |   20 ++++++++++++++------
 1 files changed, 14 insertions(+), 6 deletions(-)

-- 
1.7.7.6

[PATCH 1/2] git-svn: use POSIX::sigprocmask to block signals

[Roman Kagan]

rev_map_set() tries to avoid being interrupted by signals.

The conventional way to achieve this is through sigprocmask(), which is
available in the standard POSIX module.

This is implemented by this patch.  One important consequence of it is
that the signal handlers won't be unconditionally set to SIG_DFL anymore
upon the first invocation of rev_map_set() as they used to.

[That said, I'm not convinced that messing with signals is necessary
(and sufficient) here at all, but my perl-foo is too weak for a more
intrusive change.]

Signed-off-by: Roman Kagan <rkagan@mail.ru>
---
 git-svn.perl |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 4334b95..570504c 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -2031,6 +2031,7 @@ use IPC::Open3;
 use Time::Local;
 use Memoize;  # core since 5.8.0, Jul 2002
 use Memoize::Storable;
+use POSIX qw(:signal_h);
 
 my ($_gc_nr, $_gc_period);
 
@@ -4059,11 +4060,14 @@ sub rev_map_set {
 	length $commit == 40 or die "arg3 must be a full SHA1 hexsum\n";
 	my $db = $self->map_path($uuid);
 	my $db_lock = "$db.lock";
-	my $sig;
+	my $sigmask;
 	$update_ref ||= 0;
 	if ($update_ref) {
-		$SIG{INT} = $SIG{HUP} = $SIG{TERM} = $SIG{ALRM} = $SIG{PIPE} =
-		            $SIG{USR1} = $SIG{USR2} = sub { $sig = $_[0] };
+		$sigmask = POSIX::SigSet->new();
+		my $signew = POSIX::SigSet->new(SIGINT, SIGHUP, SIGTERM,
+			SIGALRM, SIGPIPE, SIGUSR1, SIGUSR2);
+		sigprocmask(SIG_BLOCK, $signew, $sigmask) or
+			croak "Can't block signals: $!";
 	}
 	mkfile($db);
 
@@ -4102,9 +4106,8 @@ sub rev_map_set {
 	                            "$db_lock => $db ($!)\n";
 	delete $LOCKFILES{$db_lock};
 	if ($update_ref) {
-		$SIG{INT} = $SIG{HUP} = $SIG{TERM} = $SIG{ALRM} = $SIG{PIPE} =
-		            $SIG{USR1} = $SIG{USR2} = 'DEFAULT';
-		kill $sig, $$ if defined $sig;
+		sigprocmask(SIG_SETMASK, $sigmask) or
+			croak "Can't restore signal mask: $!";
 	}
 }
 
-- 
1.7.7.6

Re: [PATCH 1/2] git-svn: use POSIX::sigprocmask to block signals

[Eric Wong]

Roman Kagan <rkagan@mail.ru> wrote:
> +		my $signew = POSIX::SigSet->new(SIGINT, SIGHUP, SIGTERM,
> +			SIGALRM, SIGPIPE, SIGUSR1, SIGUSR2);

Considering your 2/2 patch, can we remove SIGPIPE here?
Otherwise, I think this series is good.  Thanks!

Re: [PATCH 1/2] git-svn: use POSIX::sigprocmask to block signals

[Roman Kagan]

11 апреля 2012 г. 1:11 пользователь Eric Wong <normalperson@yhbt.net> написал:
> Roman Kagan <rkagan@mail.ru> wrote:
>> +             my $signew = POSIX::SigSet->new(SIGINT, SIGHUP, SIGTERM,
>> +                     SIGALRM, SIGPIPE, SIGUSR1, SIGUSR2);
>
> Considering your 2/2 patch, can we remove SIGPIPE here?

Doing it in this patch (i.e. before SIGPIPE gets ignored by the second
patch) would be illogical.

I can submit another patch which removes SIGPIPE from the list of
blocked signals (the reason would be mostly aesthetic since blocking
an ignored signal is harmless anyway).

Roman.

[PATCH 2/2] git-svn: ignore SIGPIPE

[Roman Kagan]

In HTTP with keep-alive it's not uncommon for the client to notice that
the server decided to stop maintaining the current connection only when
sending a new request.  This naturally results in -EPIPE and possibly
SIGPIPE.

The subversion library itself makes no provision for SIGPIPE.  Some
combinations of the underlying libraries do (typically SIG_IGN-ing it),
some don't.

Presumably for that reason all subversion commands set SIGPIPE to
SIG_IGN early in their main()-s.

So should we.

This, together with the previous patch, fixes the notorious "git-svn
died of signal 13" problem (see e.g.
http://thread.gmane.org/gmane.comp.version-control.git/134936).

Signed-off-by: Roman Kagan <rkagan@mail.ru>
---
 git-svn.perl |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 570504c..aa14564 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -36,6 +36,11 @@ $ENV{TZ} = 'UTC';
 $| = 1; # unbuffer STDOUT
 
 sub fatal (@) { print STDERR "@_\n"; exit 1 }
+
+# All SVN commands do it.  Otherwise we may die on SIGPIPE when the remote
+# repository decides to close the connection which we expect to be kept alive.
+$SIG{PIPE} = 'IGNORE';
+
 sub _req_svn {
 	require SVN::Core; # use()-ing this causes segfaults for me... *shrug*
 	require SVN::Ra;
-- 
1.7.7.6

Re: [PATCH 0/2] git-svn: fixes for intermittent SIGPIPE

[Roman Kagan]

2 апреля 2012 г. 20:13 пользователь Roman Kagan <rkagan@mail.ru> написал:
> In my work environment subversion is still being used as the main
> revision control system.  Therefore many people who prefer to work with
> git have to resort to git-svn.
>
> However, in many configurations it used to suffer from the notorious
> "git-svn died of signal 13" problem (see e.g.
> http://thread.gmane.org/gmane.comp.version-control.git/134936 and the
> links therein).
>
> I believe to have tracked down the issue to the connection being closed
> by the server when http keep-alive is in use, and the client dying on
> SIGPIPE because its handler is left at SIG_DFL when a new request is
> being made.
>
> The patches have been tested on
>
> - Linux Fedora 16 x86_64, git 1.7.7.6, perl v5.14.2, svn 1.6.17
> - Windows 7 x64 + Cygwin, git 1.7.9, perl v5.10.1, svn 1.7.4,
> - Windows 7 x64 + MsysGit, git 1.7.9.msysgit.0, perl v5.8.8, svn 1.4.6
>
> Roman Kagan (2):
>  git-svn: use POSIX::sigprocmask to block signals
>  git-svn: ignore SIGPIPE
>
>  git-svn.perl |   20 ++++++++++++++------
>  1 files changed, 14 insertions(+), 6 deletions(-)

IIUC the series was approved by Eric.  What do I need to do now to
have it reviewed for accepting into the master tree?

Thanks,
Roman.

Re: [PATCH 0/2] git-svn: fixes for intermittent SIGPIPE

[Junio C Hamano]

Roman Kagan <rkagan@mail.ru> writes:

> IIUC the series was approved by Eric.  What do I need to do now to
> have it reviewed for accepting into the master tree?

I see this:

        Date: Tue, 10 Apr 2012 21:11:20 +0000
        From: Eric Wong <normalperson@yhbt.net>
        Message-ID: <20120410211120.GA27555@dcvr.yhbt.net>

        Roman Kagan <rkagan@mail.ru> wrote:
        > +		my $signew = POSIX::SigSet->new(SIGINT, SIGHUP, SIGTERM,
        > +			SIGALRM, SIGPIPE, SIGUSR1, SIGUSR2);

        Considering your 2/2 patch, can we remove SIGPIPE here?
        Otherwise, I think this series is good.  Thanks!

What usually happens after such an intial round of review is for you to
think about the comments like this one given during the review, and
either submit a patch updated accordingly, or discuss why your original
is better than the suggested update, and then the reviewer responds to
it, and repeat the process until everybody involved in the discussion
accepts the outcome.

Then the patch will hit my 'next' branch (or my 'master' branch, for
subsystems like git-svn where the area expert, i.e. Eric in this case,
knows much better than myself) after that.

In short, as far as I can see, the ball is still in your court.

Thanks for a reminder, though.

Re: [PATCH 0/2] git-svn: fixes for intermittent SIGPIPE

[Roman Kagan]

23 апреля 2012 г. 18:44 пользователь Junio C Hamano <gitster@pobox.com> написал:
> Roman Kagan <rkagan@mail.ru> writes:
>
>> IIUC the series was approved by Eric.  What do I need to do now to
>> have it reviewed for accepting into the master tree?
>
> I see this:
>
>        Date: Tue, 10 Apr 2012 21:11:20 +0000
>        From: Eric Wong <normalperson@yhbt.net>
>        Message-ID: <20120410211120.GA27555@dcvr.yhbt.net>
>
>        Roman Kagan <rkagan@mail.ru> wrote:
>        > +             my $signew = POSIX::SigSet->new(SIGINT, SIGHUP, SIGTERM,
>        > +                     SIGALRM, SIGPIPE, SIGUSR1, SIGUSR2);
>
>        Considering your 2/2 patch, can we remove SIGPIPE here?
>        Otherwise, I think this series is good.  Thanks!
>
> What usually happens after such an intial round of review is for you to
> think about the comments like this one given during the review, and
> either submit a patch updated accordingly, or discuss why your original
> is better than the suggested update, and then the reviewer responds to
> it, and repeat the process until everybody involved in the discussion
> accepts the outcome.

I replied on the very same day that I thought that Eric's comment
would better be addressed in a followup patch, and that patch would be
purely cosmetic anyway.
So I felt like I could wait until these two are merged or commented by
more people.  My bad; will resubmit the series with the third patch
included to hopefully get Eric's full approval.

Thanks,
Roman.