{"thread":{"id":"30127","subject":"[PATCH 0/2] git-svn: fixes for intermittent SIGPIPE","startedAt":"2012-04-02T16:13:36Z","lastAt":"2012-04-23T15:10:43Z","messageCount":8,"participants":["Roman Kagan","Eric Wong","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"188316","messageId":"cover.1333381684.git.rkagan@mail.ru","threadId":"30127","inReplyTo":null,"subject":"[PATCH 0/2] git-svn: fixes for intermittent SIGPIPE","fromName":"Roman Kagan","fromEmail":"rkagan@mail.ru","sentAt":"2012-04-02T16:13:36Z","receivedAt":"2012-04-02T16:13:36Z","isPatch":true,"sender":{"key":"rkagan@mail.ru","avatar":"https://avatars.githubusercontent.com/u/10214432?v=4"},"body":"In my work environment subversion is still being used as the main\nrevision control system. Therefore many people who prefer to work with\ngit have to resort to git-svn.\n\nHowever, in many configurations it used to suffer from the notorious\n\"git-svn died of signal 13\" problem (see e.g.\nhttp://thread.gmane.org/gmane.comp.version-control.git/134936 and the\nlinks therein).\n\nI believe to have tracked down the issue to the connection being closed\nby the server when http keep-alive is in use, and the client dying on\nSIGPIPE because its handler is left at SIG_DFL when a new request is\nbeing made.\n\nThe patches have been tested on\n\n- Linux Fedora 16 x86_64, git 1.7.7.6, perl v5.14.2, svn 1.6.17\n- Windows 7 x64 + Cygwin, git 1.7.9, perl v5.10.1, svn 1.7.4,\n- Windows 7 x64 + MsysGit, git 1.7.9.msysgit.0, perl v5.8.8, svn 1.4.6\n\nRoman Kagan (2):\n git-svn: use POSIX::sigprocmask to block signals\n git-svn: ignore SIGPIPE\n\n git-svn.perl | 20 ++++++++++++++------\n 1 files changed, 14 insertions(+), 6 deletions(-)\n\n-- \n1.7.7.6\n"},{"id":"188318","messageId":"9eaaebac91dc2b1a45a4dec77142be0b0b338056.1333381684.git.rkagan@mail.ru","threadId":"30127","inReplyTo":"cover.1333381684.git.rkagan@mail.ru","subject":"[PATCH 1/2] git-svn: use POSIX::sigprocmask to block signals","fromName":"Roman Kagan","fromEmail":"rkagan@mail.ru","sentAt":"2012-04-02T16:13:37Z","receivedAt":"2012-04-02T16:13:37Z","isPatch":true,"sender":{"key":"rkagan@mail.ru","avatar":"https://avatars.githubusercontent.com/u/10214432?v=4"},"body":"rev_map_set() tries to avoid being interrupted by signals.\n\nThe conventional way to achieve this is through sigprocmask(), which is\navailable in the standard POSIX module.\n\nThis is implemented by this patch. One important consequence of it is\nthat the signal handlers won't be unconditionally set to SIG_DFL anymore\nupon the first invocation of rev_map_set() as they used to.\n\n[That said, I'm not convinced that messing with signals is necessary\n(and sufficient) here at all, but my perl-foo is too weak for a more\nintrusive change.]\n\nSigned-off-by: Roman Kagan \n---\n git-svn.perl | 15 +++++++++------\n 1 files changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex 4334b95..570504c 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -2031,6 +2031,7 @@ use IPC::Open3;\n use Time::Local;\n use Memoize; # core since 5.8.0, Jul 2002\n use Memoize::Storable;\n+use POSIX qw(:signal_h);\n \n my ($_gc_nr, $_gc_period);\n \n@@ -4059,11 +4060,14 @@ sub rev_map_set {\n \tlength $commit == 40 or die \"arg3 must be a full SHA1 hexsum\\n\";\n \tmy $db = $self->map_path($uuid);\n \tmy $db_lock = \"$db.lock\";\n-\tmy $sig;\n+\tmy $sigmask;\n \t$update_ref ||= 0;\n \tif ($update_ref) {\n-\t\t$SIG{INT} = $SIG{HUP} = $SIG{TERM} = $SIG{ALRM} = $SIG{PIPE} =\n-\t\t $SIG{USR1} = $SIG{USR2} = sub { $sig = $_[0] };\n+\t\t$sigmask = POSIX::SigSet->new();\n+\t\tmy $signew = POSIX::SigSet->new(SIGINT, SIGHUP, SIGTERM,\n+\t\t\tSIGALRM, SIGPIPE, SIGUSR1, SIGUSR2);\n+\t\tsigprocmask(SIG_BLOCK, $signew, $sigmask) or\n+\t\t\tcroak \"Can't block signals: $!\";\n \t}\n \tmkfile($db);\n \n@@ -4102,9 +4106,8 @@ sub rev_map_set {\n \t \"$db_lock => $db ($!)\\n\";\n \tdelete $LOCKFILES{$db_lock};\n \tif ($update_ref) {\n-\t\t$SIG{INT} = $SIG{HUP} = $SIG{TERM} = $SIG{ALRM} = $SIG{PIPE} =\n-\t\t $SIG{USR1} = $SIG{USR2} = 'DEFAULT';\n-\t\tkill $sig, $$ if defined $sig;\n+\t\tsigprocmask(SIG_SETMASK, $sigmask) or\n+\t\t\tcroak \"Can't restore signal mask: $!\";\n \t}\n }\n \n-- \n1.7.7.6\n"},{"id":"188317","messageId":"af5d9c78d04ebc78ebd3636f7912b675b3c1f19d.1333381684.git.rkagan@mail.ru","threadId":"30127","inReplyTo":"cover.1333381684.git.rkagan@mail.ru","subject":"[PATCH 2/2] git-svn: ignore SIGPIPE","fromName":"Roman Kagan","fromEmail":"rkagan@mail.ru","sentAt":"2012-04-02T16:13:39Z","receivedAt":"2012-04-02T16:13:39Z","isPatch":true,"sender":{"key":"rkagan@mail.ru","avatar":"https://avatars.githubusercontent.com/u/10214432?v=4"},"body":"In HTTP with keep-alive it's not uncommon for the client to notice that\nthe server decided to stop maintaining the current connection only when\nsending a new request. This naturally results in -EPIPE and possibly\nSIGPIPE.\n\nThe subversion library itself makes no provision for SIGPIPE. Some\ncombinations of the underlying libraries do (typically SIG_IGN-ing it),\nsome don't.\n\nPresumably for that reason all subversion commands set SIGPIPE to\nSIG_IGN early in their main()-s.\n\nSo should we.\n\nThis, together with the previous patch, fixes the notorious \"git-svn\ndied of signal 13\" problem (see e.g.\nhttp://thread.gmane.org/gmane.comp.version-control.git/134936).\n\nSigned-off-by: Roman Kagan \n---\n git-svn.perl | 5 +++++\n 1 files changed, 5 insertions(+), 0 deletions(-)\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex 570504c..aa14564 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -36,6 +36,11 @@ $ENV{TZ} = 'UTC';\n $| = 1; # unbuffer STDOUT\n \n sub fatal (@) { print STDERR \"@_\\n\"; exit 1 }\n+\n+# All SVN commands do it. Otherwise we may die on SIGPIPE when the remote\n+# repository decides to close the connection which we expect to be kept alive.\n+$SIG{PIPE} = 'IGNORE';\n+\n sub _req_svn {\n \trequire SVN::Core; # use()-ing this causes segfaults for me... *shrug*\n \trequire SVN::Ra;\n-- \n1.7.7.6\n"},{"id":"188909","messageId":"20120410211120.GA27555@dcvr.yhbt.net","threadId":"30127","inReplyTo":"9eaaebac91dc2b1a45a4dec77142be0b0b338056.1333381684.git.rkagan@mail.ru","subject":"Re: [PATCH 1/2] git-svn: use POSIX::sigprocmask to block signals","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2012-04-10T21:11:20Z","receivedAt":"2012-04-10T21:11:20Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Roman Kagan wrote:\n> +\t\tmy $signew = POSIX::SigSet->new(SIGINT, SIGHUP, SIGTERM,\n> +\t\t\tSIGALRM, SIGPIPE, SIGUSR1, SIGUSR2);\n\nConsidering your 2/2 patch, can we remove SIGPIPE here?\nOtherwise, I think this series is good. Thanks!\n"},{"id":"188946","messageId":"CANiYKX56EnCJu6qTEJC=K7HVquObDXYCxRCPxn2YuJk+MZDmBQ@mail.gmail.com","threadId":"30127","inReplyTo":"20120410211120.GA27555@dcvr.yhbt.net","subject":"Re: [PATCH 1/2] git-svn: use POSIX::sigprocmask to block signals","fromName":"Roman Kagan","fromEmail":"rkagan@mail.ru","sentAt":"2012-04-11T11:22:56Z","receivedAt":"2012-04-11T11:22:56Z","isPatch":true,"sender":{"key":"rkagan@mail.ru","avatar":"https://avatars.githubusercontent.com/u/10214432?v=4"},"body":"11 апреля 2012 г. 1:11 пользователь Eric Wong написал:\n> Roman Kagan wrote:\n>> +             my $signew = POSIX::SigSet->new(SIGINT, SIGHUP, SIGTERM,\n>> +                     SIGALRM, SIGPIPE, SIGUSR1, SIGUSR2);\n>\n> Considering your 2/2 patch, can we remove SIGPIPE here?\n\nDoing it in this patch (i.e. before SIGPIPE gets ignored by the second\npatch) would be illogical.\n\nI can submit another patch which removes SIGPIPE from the list of\nblocked signals (the reason would be mostly aesthetic since blocking\nan ignored signal is harmless anyway).\n\nRoman.\n"},{"id":"189848","messageId":"CANiYKX5RNb3YXhGWXzpfDz+XK1PM6zyN=zrDKk3_4StCu2ukzg@mail.gmail.com","threadId":"30127","inReplyTo":"cover.1333381684.git.rkagan@mail.ru","subject":"Re: [PATCH 0/2] git-svn: fixes for intermittent SIGPIPE","fromName":"Roman Kagan","fromEmail":"rkagan@mail.ru","sentAt":"2012-04-23T07:05:37Z","receivedAt":"2012-04-23T07:05:37Z","isPatch":true,"sender":{"key":"rkagan@mail.ru","avatar":"https://avatars.githubusercontent.com/u/10214432?v=4"},"body":"2 апреля 2012 г. 20:13 пользователь Roman Kagan написал:\n> In my work environment subversion is still being used as the main\n> revision control system.  Therefore many people who prefer to work with\n> git have to resort to git-svn.\n>\n> However, in many configurations it used to suffer from the notorious\n> \"git-svn died of signal 13\" problem (see e.g.\n> http://thread.gmane.org/gmane.comp.version-control.git/134936 and the\n> links therein).\n>\n> I believe to have tracked down the issue to the connection being closed\n> by the server when http keep-alive is in use, and the client dying on\n> SIGPIPE because its handler is left at SIG_DFL when a new request is\n> being made.\n>\n> The patches have been tested on\n>\n> - Linux Fedora 16 x86_64, git 1.7.7.6, perl v5.14.2, svn 1.6.17\n> - Windows 7 x64 + Cygwin, git 1.7.9, perl v5.10.1, svn 1.7.4,\n> - Windows 7 x64 + MsysGit, git 1.7.9.msysgit.0, perl v5.8.8, svn 1.4.6\n>\n> Roman Kagan (2):\n>  git-svn: use POSIX::sigprocmask to block signals\n>  git-svn: ignore SIGPIPE\n>\n>  git-svn.perl |   20 ++++++++++++++------\n>  1 files changed, 14 insertions(+), 6 deletions(-)\n\nIIUC the series was approved by Eric. What do I need to do now to\nhave it reviewed for accepting into the master tree?\n\nThanks,\nRoman.\n"},{"id":"189877","messageId":"xmqqvckqld7q.fsf@junio.mtv.corp.google.com","threadId":"30127","inReplyTo":"CANiYKX5RNb3YXhGWXzpfDz+XK1PM6zyN=zrDKk3_4StCu2ukzg@mail.gmail.com","subject":"Re: [PATCH 0/2] git-svn: fixes for intermittent SIGPIPE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-23T14:44:41Z","receivedAt":"2012-04-23T14:44:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Roman Kagan writes:\n\n> IIUC the series was approved by Eric. What do I need to do now to\n> have it reviewed for accepting into the master tree?\n\nI see this:\n\n Date: Tue, 10 Apr 2012 21:11:20 +0000\n From: Eric Wong \n Message-ID: <20120410211120.GA27555@dcvr.yhbt.net>\n\n Roman Kagan wrote:\n > +\t\tmy $signew = POSIX::SigSet->new(SIGINT, SIGHUP, SIGTERM,\n > +\t\t\tSIGALRM, SIGPIPE, SIGUSR1, SIGUSR2);\n\n Considering your 2/2 patch, can we remove SIGPIPE here?\n Otherwise, I think this series is good. Thanks!\n\nWhat usually happens after such an intial round of review is for you to\nthink about the comments like this one given during the review, and\neither submit a patch updated accordingly, or discuss why your original\nis better than the suggested update, and then the reviewer responds to\nit, and repeat the process until everybody involved in the discussion\naccepts the outcome.\n\nThen the patch will hit my 'next' branch (or my 'master' branch, for\nsubsystems like git-svn where the area expert, i.e. Eric in this case,\nknows much better than myself) after that.\n\nIn short, as far as I can see, the ball is still in your court.\n\nThanks for a reminder, though.\n"},{"id":"189878","messageId":"CANiYKX7=og814EYvXkQ4jeOWkr7e9GZAXu1SKUYSDza6BCGtGw@mail.gmail.com","threadId":"30127","inReplyTo":"xmqqvckqld7q.fsf@junio.mtv.corp.google.com","subject":"Re: [PATCH 0/2] git-svn: fixes for intermittent SIGPIPE","fromName":"Roman Kagan","fromEmail":"rkagan@mail.ru","sentAt":"2012-04-23T15:10:43Z","receivedAt":"2012-04-23T15:10:43Z","isPatch":true,"sender":{"key":"rkagan@mail.ru","avatar":"https://avatars.githubusercontent.com/u/10214432?v=4"},"body":"23 апреля 2012 г. 18:44 пользователь Junio C Hamano написал:\n> Roman Kagan writes:\n>\n>> IIUC the series was approved by Eric.  What do I need to do now to\n>> have it reviewed for accepting into the master tree?\n>\n> I see this:\n>\n>        Date: Tue, 10 Apr 2012 21:11:20 +0000\n>        From: Eric Wong \n>        Message-ID: <20120410211120.GA27555@dcvr.yhbt.net>\n>\n>        Roman Kagan wrote:\n>        > +             my $signew = POSIX::SigSet->new(SIGINT, SIGHUP, SIGTERM,\n>        > +                     SIGALRM, SIGPIPE, SIGUSR1, SIGUSR2);\n>\n>        Considering your 2/2 patch, can we remove SIGPIPE here?\n>        Otherwise, I think this series is good.  Thanks!\n>\n> What usually happens after such an intial round of review is for you to\n> think about the comments like this one given during the review, and\n> either submit a patch updated accordingly, or discuss why your original\n> is better than the suggested update, and then the reviewer responds to\n> it, and repeat the process until everybody involved in the discussion\n> accepts the outcome.\n\nI replied on the very same day that I thought that Eric's comment\nwould better be addressed in a followup patch, and that patch would be\npurely cosmetic anyway.\nSo I felt like I could wait until these two are merged or commented by\nmore people. My bad; will resubmit the series with the third patch\nincluded to hopefully get Eric's full approval.\n\nThanks,\nRoman.\n"}]}