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

[PATCH 1/3] 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

[PATCH 2/3] 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

[PATCH 3/3] git-svn: drop redundant blocking of SIGPIPE

[Roman Kagan]

Now that SIGPIPE is ignored there's no point blocking it.

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

diff --git a/git-svn.perl b/git-svn.perl
index aa14564..f8e9ef0 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -4070,7 +4070,7 @@ sub rev_map_set {
 	if ($update_ref) {
 		$sigmask = POSIX::SigSet->new();
 		my $signew = POSIX::SigSet->new(SIGINT, SIGHUP, SIGTERM,
-			SIGALRM, SIGPIPE, SIGUSR1, SIGUSR2);
+			SIGALRM, SIGUSR1, SIGUSR2);
 		sigprocmask(SIG_BLOCK, $signew, $sigmask) or
 			croak "Can't block signals: $!";
 	}
-- 
1.7.7.6

[PATCH 0/3] 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

This is the second version of the series; it only differs from the first
submission in that it includes the third patch with a cosmetic cleanup.

Roman Kagan (3):
  git-svn: use POSIX::sigprocmask to block signals
  git-svn: ignore SIGPIPE
  git-svn: drop redundant blocking of SIGPIPE

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

-- 
1.7.7.6

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

[Junio C Hamano]

Roman Kagan <rkagan@mail.ru> writes:

> rev_map_set() tries to avoid being interrupted by signals.

The wording "tries to avoid" was unclear and I had to read the code
twice.  The code defers the signal processing but still wants to get the
signal after it is done what it is doing, which is different from simply
"ignoring", which is another way to "try to avoid".

> 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 may be the first degree consequence (another is what happens when
you received signals of different kinds while in the blocked section),
but how would that difference affect the overall program execution?

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

Everything you discussed above in the log message before "That said"
part made sense.  Instead of catching and setting a single $sig and
replaying that later, potentially losing accumulated signals that are of
different kinds, blocking before entering the part you do not want to
get interrupted and unblocking after you are done is better done using
sigprocmask.

If the problem to solve is to implement deferral and delayed signal
processing correctly, I think your patch did the right thing, but your
"necessary/sufficient" comment implies that the problem you were trying
to address is _different_ from that.  But it is not clear what it is.

Could you elaborate on it a bit more here, or if it will become clear in
the later patch, then please drop that parenthesized part out of the log
message.

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

[Roman Kagan]

23 апреля 2012 г. 21:33 пользователь Junio C Hamano <junio@pobox.com> написал:
> Roman Kagan <rkagan@mail.ru> writes:
>
>> rev_map_set() tries to avoid being interrupted by signals.
>
> The wording "tries to avoid" was unclear and I had to read the code
> twice.  The code defers the signal processing but still wants to get the
> signal after it is done what it is doing, which is different from simply
> "ignoring", which is another way to "try to avoid".

Sorry.  I'll try to avoid misunderstanding next time ;)

>> 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 may be the first degree consequence (another is what happens when
> you received signals of different kinds while in the blocked section),
> but how would that difference affect the overall program execution?

Unless the parent of this script decides to ignore those signals,
nothing will change: the signals will be delivered at the end of the
section and script will die.

>> [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.]
>
> Everything you discussed above in the log message before "That said"
> part made sense.  Instead of catching and setting a single $sig and
> replaying that later, potentially losing accumulated signals that are of
> different kinds, blocking before entering the part you do not want to
> get interrupted and unblocking after you are done is better done using
> sigprocmask.
>
> If the problem to solve is to implement deferral and delayed signal
> processing correctly, I think your patch did the right thing, but your
> "necessary/sufficient" comment implies that the problem you were trying
> to address is _different_ from that.  But it is not clear what it is.
>
> Could you elaborate on it a bit more here, or if it will become clear in
> the later patch, then please drop that parenthesized part out of the log
> message.

If read the code correctly (and I'm not a "native perl speaker"), all
this is about maintaining consistency in the ad-hoc database for
mapping svn revision numbers git commits.

However, blocking some signals is obviously not a good enough
protection measure: the program may die on SIGKILL or the power may go
off.  Even worse, due to the unfortunate record size of 24 bytes
there's essentially no way to ensure atomicity of appends.

So if the consistency of this database is critical for git-svn, then
it needs a stronger protection mechanism; if it isn't then it's
probably not worth the hassle at all.

However, as I said I'm not strong enough in perl to understand which
of the above applies and how to do it right.  (Yes, ~7kloc perl script
using multi-level callback-based svn binding scares me off).

So I just made a fairly simple change that basically preserved the
existing behavior but allowed me to set SIGPIPE to SIG_IGN for the
duration of the script run in the second patch of the series.

Do you think I should merge this into the commit log, or just drop
that appendix from my original log and be done with it?

Roman.

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

[Junio C Hamano]

Roman Kagan <rkagan@mail.ru> writes:

> [B]locking some signals is obviously not a good enough
> protection measure: the program may die on SIGKILL or the power may go
> off.

Ahh, that is what you meant.  I agree that it is of dubious value to
only try to catch signals, even though it may be better than nothing.

It should be sufficient to replace the "[...]" part with the above,
and conclude it with "But it will be a separate topic that this patch
does not address." or something.

Thanks.

[PATCH 2/3] 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