threads / patch / 30127

patch, 2 partsgit-svn: fixes for intermittent SIGPIPE

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

## tl;dr

8 messages between Apr 2, 2012 and Apr 23, 2012. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Roman Kagan· Apr 2, 2012, 16:13 UTC · lore

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

Roman Kagan· Apr 2, 2012, 16:13 UTC · re: Roman Kagan · lore

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

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(-)

Show changes to git-svn.perl +9 −6

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

Eric Wong· Apr 10, 2012, 21:11 UTC · re: Roman Kagan · lore

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

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!

Roman Kagan· Apr 11, 2012, 11:22 UTC · re: Eric Wong · lore

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

11 апреля 2012 г. 1:11 пользователь Eric Wong <normalperson@yhbt.net> написал:

Show 5 quoted lines

> 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.

Roman Kagan· Apr 2, 2012, 16:13 UTC · re: Roman Kagan · lore

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

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(-)

Show changes to git-svn.perl +5 −0

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

Roman Kagan· Apr 23, 2012, 07:05 UTC · re: Roman Kagan · lore

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

2 апреля 2012 г. 20:13 пользователь Roman Kagan <rkagan@mail.ru> написал:

Show 26 quoted lines

> 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.

Junio C Hamano· Apr 23, 2012, 14:44 UTC · re: Roman Kagan · lore

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

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.

Roman Kagan· Apr 23, 2012, 15:10 UTC · re: Junio C Hamano · lore

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

23 апреля 2012 г. 18:44 пользователь Junio C Hamano <gitster@pobox.com> написал:

Show 24 quoted lines

> 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.