threads / patch / 30319

patch, 3 partsgit-svn: use POSIX::sigprocmask to block signals

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

## tl;dr

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

replies: 5people: 3as markdown or json

Roman Kagan· Apr 24, 2012, 06:53 UTC · lore

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

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 third version of the series; compared to the previous submission the log message of the first patch was reworded to make it more palatable.

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

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

In order to maintain consistency of the database mapping svn revision numbers to git commit ids, rev_map_set() defers signal processing until it's finished with an append transaction.[*]

The conventional way to achieve this is through sigprocmask(), which is available in perl 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. As a result, the signals ignored by git-svn parent will remain ignored; otherwise the behavior remains the same.

This patch paves the way to ignoring SIGPIPE throughout git-svn which will be done in the followup patch.

[*] Deferring signals is not enough to ensure the database consistency: the program may die on SIGKILL or power loss, run out of disk space, etc. However that's a separate issue that this patch doesn't address.

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

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

[PATCH 2/3] 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, 16:26 UTC · re: Roman Kagan · lore

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

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

Show changes to git-svn.perl +1 −1

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

Eric Wong· Apr 24, 2012, 09:45 UTC · re: Roman Kagan · lore

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

Roman Kagan <rkagan@mail.ru> wrote:

Show 7 quoted lines

> This is the third version of the series; compared to the previous submission
> the log message of the first patch was reworded to make it more palatable.
> 
> Roman Kagan (3):
>   git-svn: use POSIX::sigprocmask to block signals
>   git-svn: ignore SIGPIPE
>   git-svn: drop redundant blocking of SIGPIPE

Thanks for this series, acked and pushed to git://bogomips.org/git-svn

Junio C Hamano· Apr 24, 2012, 19:09 UTC · re: Eric Wong · lore

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

Eric Wong <normalperson@yhbt.net> writes:

Show 10 quoted lines

> Roman Kagan <rkagan@mail.ru> wrote:
>> This is the third version of the series; compared to the previous submission
>> the log message of the first patch was reworded to make it more palatable.
>> 
>> Roman Kagan (3):
>>   git-svn: use POSIX::sigprocmask to block signals
>>   git-svn: ignore SIGPIPE
>>   git-svn: drop redundant blocking of SIGPIPE
>
> Thanks for this series, acked and pushed to git://bogomips.org/git-svn

Thanks, both. I agree that the first one is much more nicely explained than the previous one.