{"thread":{"id":"30309","subject":"[PATCH 0/3] git-svn: fixes for intermittent SIGPIPE","startedAt":"2012-04-02T13:29:32Z","lastAt":"2012-04-23T21:13:51Z","messageCount":7,"participants":["Roman Kagan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"189891","messageId":"d21d7433574e8ea7628320dbe1a5fc0dc9d94e64.1335198921.git.rkagan@mail.ru","threadId":"30309","inReplyTo":"cover.1335198921.git.rkagan@mail.ru","subject":"[PATCH 1/3] git-svn: use POSIX::sigprocmask to block signals","fromName":"Roman Kagan","fromEmail":"rkagan@mail.ru","sentAt":"2012-04-02T13:29:32Z","receivedAt":"2012-04-02T13:29:32Z","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":"189892","messageId":"b5b0657dbf648eccab746b5e448aacbc8dc83c8b.1335198921.git.rkagan@mail.ru","threadId":"30309","inReplyTo":"cover.1335198921.git.rkagan@mail.ru","subject":"[PATCH 2/3] git-svn: ignore SIGPIPE","fromName":"Roman Kagan","fromEmail":"rkagan@mail.ru","sentAt":"2012-04-02T13:52:34Z","receivedAt":"2012-04-02T13:52:34Z","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":"189893","messageId":"dad745568f79055298e3c12942f5bab22d0103fd.1335198921.git.rkagan@mail.ru","threadId":"30309","inReplyTo":"cover.1335198921.git.rkagan@mail.ru","subject":"[PATCH 3/3] git-svn: drop redundant blocking of SIGPIPE","fromName":"Roman Kagan","fromEmail":"rkagan@mail.ru","sentAt":"2012-04-23T16:26:56Z","receivedAt":"2012-04-23T16:26:56Z","isPatch":true,"sender":{"key":"rkagan@mail.ru","avatar":"https://avatars.githubusercontent.com/u/10214432?v=4"},"body":"Now that SIGPIPE is ignored there's no point blocking it.\n\nSigned-off-by: Roman Kagan \n---\n git-svn.perl | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex aa14564..f8e9ef0 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -4070,7 +4070,7 @@ sub rev_map_set {\n \tif ($update_ref) {\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\t\tSIGALRM, SIGUSR1, SIGUSR2);\n \t\tsigprocmask(SIG_BLOCK, $signew, $sigmask) or\n \t\t\tcroak \"Can't block signals: $!\";\n \t}\n-- \n1.7.7.6\n"},{"id":"189890","messageId":"cover.1335198921.git.rkagan@mail.ru","threadId":"30309","inReplyTo":null,"subject":"[PATCH 0/3] git-svn: fixes for intermittent SIGPIPE","fromName":"Roman Kagan","fromEmail":"rkagan@mail.ru","sentAt":"2012-04-23T16:35:21Z","receivedAt":"2012-04-23T16:35:21Z","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\nThis is the second version of the series; it only differs from the first\nsubmission in that it includes the third patch with a cosmetic cleanup.\n\nRoman Kagan (3):\n git-svn: use POSIX::sigprocmask to block signals\n git-svn: ignore SIGPIPE\n git-svn: drop redundant blocking of SIGPIPE\n\n git-svn.perl | 20 ++++++++++++++------\n 1 files changed, 14 insertions(+), 6 deletions(-)\n\n-- \n1.7.7.6\n"},{"id":"189900","messageId":"xmqqipgqjqtk.fsf@junio.mtv.corp.google.com","threadId":"30309","inReplyTo":"d21d7433574e8ea7628320dbe1a5fc0dc9d94e64.1335198921.git.rkagan@mail.ru","subject":"Re: [PATCH 1/3] git-svn: use POSIX::sigprocmask to block signals","fromName":"Junio C Hamano","fromEmail":"junio@pobox.com","sentAt":"2012-04-23T17:33:43Z","receivedAt":"2012-04-23T17:33:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Roman Kagan writes:\n\n> rev_map_set() tries to avoid being interrupted by signals.\n\nThe wording \"tries to avoid\" was unclear and I had to read the code\ntwice. The code defers the signal processing but still wants to get the\nsignal after it is done what it is doing, which is different from simply\n\"ignoring\", which is another way to \"try to avoid\".\n\n> The conventional way to achieve this is through sigprocmask(), which is\n> available in the standard POSIX module.\n>\n> This is implemented by this patch. One important consequence of it is\n> that the signal handlers won't be unconditionally set to SIG_DFL anymore\n> upon the first invocation of rev_map_set() as they used to.\n\nThat may be the first degree consequence (another is what happens when\nyou received signals of different kinds while in the blocked section),\nbut how would that difference affect the overall program execution?\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\n> intrusive change.]\n\nEverything you discussed above in the log message before \"That said\"\npart made sense. Instead of catching and setting a single $sig and\nreplaying that later, potentially losing accumulated signals that are of\ndifferent kinds, blocking before entering the part you do not want to\nget interrupted and unblocking after you are done is better done using\nsigprocmask.\n\nIf the problem to solve is to implement deferral and delayed signal\nprocessing correctly, I think your patch did the right thing, but your\n\"necessary/sufficient\" comment implies that the problem you were trying\nto address is _different_ from that. But it is not clear what it is.\n\nCould you elaborate on it a bit more here, or if it will become clear in\nthe later patch, then please drop that parenthesized part out of the log\nmessage.\n"},{"id":"189915","messageId":"CANiYKX68AJyE3+3BT7QSjziGS=HGKuk95rxS_04c0sGz9pywXw@mail.gmail.com","threadId":"30309","inReplyTo":"xmqqipgqjqtk.fsf@junio.mtv.corp.google.com","subject":"Re: [PATCH 1/3] git-svn: use POSIX::sigprocmask to block signals","fromName":"Roman Kagan","fromEmail":"rkagan@mail.ru","sentAt":"2012-04-23T21:07:23Z","receivedAt":"2012-04-23T21:07:23Z","isPatch":true,"sender":{"key":"rkagan@mail.ru","avatar":"https://avatars.githubusercontent.com/u/10214432?v=4"},"body":"23 апреля 2012 г. 21:33 пользователь Junio C Hamano написал:\n> Roman Kagan writes:\n>\n>> rev_map_set() tries to avoid being interrupted by signals.\n>\n> The wording \"tries to avoid\" was unclear and I had to read the code\n> twice.  The code defers the signal processing but still wants to get the\n> signal after it is done what it is doing, which is different from simply\n> \"ignoring\", which is another way to \"try to avoid\".\n\nSorry. I'll try to avoid misunderstanding next time ;)\n\n>> This is implemented by this patch.  One important consequence of it is\n>> that the signal handlers won't be unconditionally set to SIG_DFL anymore\n>> upon the first invocation of rev_map_set() as they used to.\n>\n> That may be the first degree consequence (another is what happens when\n> you received signals of different kinds while in the blocked section),\n> but how would that difference affect the overall program execution?\n\nUnless the parent of this script decides to ignore those signals,\nnothing will change: the signals will be delivered at the end of the\nsection and script will die.\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\n>> intrusive change.]\n>\n> Everything you discussed above in the log message before \"That said\"\n> part made sense.  Instead of catching and setting a single $sig and\n> replaying that later, potentially losing accumulated signals that are of\n> different kinds, blocking before entering the part you do not want to\n> get interrupted and unblocking after you are done is better done using\n> sigprocmask.\n>\n> If the problem to solve is to implement deferral and delayed signal\n> processing correctly, I think your patch did the right thing, but your\n> \"necessary/sufficient\" comment implies that the problem you were trying\n> to address is _different_ from that.  But it is not clear what it is.\n>\n> Could you elaborate on it a bit more here, or if it will become clear in\n> the later patch, then please drop that parenthesized part out of the log\n> message.\n\nIf read the code correctly (and I'm not a \"native perl speaker\"), all\nthis is about maintaining consistency in the ad-hoc database for\nmapping svn revision numbers git commits.\n\nHowever, blocking some signals is obviously not a good enough\nprotection measure: the program may die on SIGKILL or the power may go\noff. Even worse, due to the unfortunate record size of 24 bytes\nthere's essentially no way to ensure atomicity of appends.\n\nSo if the consistency of this database is critical for git-svn, then\nit needs a stronger protection mechanism; if it isn't then it's\nprobably not worth the hassle at all.\n\nHowever, as I said I'm not strong enough in perl to understand which\nof the above applies and how to do it right. (Yes, ~7kloc perl script\nusing multi-level callback-based svn binding scares me off).\n\nSo I just made a fairly simple change that basically preserved the\nexisting behavior but allowed me to set SIGPIPE to SIG_IGN for the\nduration of the script run in the second patch of the series.\n\nDo you think I should merge this into the commit log, or just drop\nthat appendix from my original log and be done with it?\n\nRoman.\n"},{"id":"189916","messageId":"xmqqd36yi228.fsf@junio.mtv.corp.google.com","threadId":"30309","inReplyTo":"CANiYKX68AJyE3+3BT7QSjziGS=HGKuk95rxS_04c0sGz9pywXw@mail.gmail.com","subject":"Re: [PATCH 1/3] git-svn: use POSIX::sigprocmask to block signals","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-23T21:13:51Z","receivedAt":"2012-04-23T21:13:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Roman Kagan writes:\n\n> [B]locking some signals is obviously not a good enough\n> protection measure: the program may die on SIGKILL or the power may go\n> off.\n\nAhh, that is what you meant. I agree that it is of dubious value to\nonly try to catch signals, even though it may be better than nothing.\n\nIt should be sufficient to replace the \"[...]\" part with the above,\nand conclude it with \"But it will be a separate topic that this patch\ndoes not address.\" or something.\n\nThanks.\n"}]}