threads / patch / 37856

patchdiff-highlight: exit when a pipe is broken

Subject: [PATCH] diff-highlight: exit when a pipe is broken

## tl;dr

3 messages between Oct 31, 2014 and Nov 4, 2014. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

John Szakmeister· Oct 31, 2014, 11:04 UTC · lore

While using diff-highlight with other tools, I have discovered that Python ignores SIGPIPE by default. Unfortunately, this also means that tools attempting to launch a pager under Python--and don't realize this is happening--means that the subprocess inherits this setting. In this case, it means diff-highlight will be launched with SIGPIPE being ignored. Let's work with those broken scripts by explicitly setting up a SIGPIPE handler and exiting the process.

Signed-off-by: John Szakmeister <john@szakmeister.net>
---
 contrib/diff-highlight/diff-highlight | 9 +++++++++
 1 file changed, 9 insertions(+)
Show changes to contrib/diff-highlight/diff-highlight +9 −0
diff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight
index c4404d4..dfcc35a 100755
--- a/contrib/diff-highlight/diff-highlight
+++ b/contrib/diff-highlight/diff-highlight
@@ -14,6 +14,15 @@ my @removed;
 my @added;
 my $in_hunk;
 
+# Some scripts may not realize that SIGPIPE is being ignored when launching the
+# pager--for instance scripts written in Python.  Setting $SIG{PIPE} = 'DEFAULT'
+# doesn't work in these instances, so we install our own signal handler instead.
+sub pipe_handler {
+    exit(0);
+}
+
+$SIG{PIPE} = \&pipe_handler;
+
 while (<>) {
 	if (!$in_hunk) {
 		print;

-- 
2.0.1
Jeff King· Nov 1, 2014, 04:04 UTC · re: John Szakmeister · lore

Re: [PATCH] diff-highlight: exit when a pipe is broken

On Fri, Oct 31, 2014 at 07:04:04AM -0400, John Szakmeister wrote:
Show 7 quoted lines
> While using diff-highlight with other tools, I have discovered that Python
> ignores SIGPIPE by default.  Unfortunately, this also means that tools
> attempting to launch a pager under Python--and don't realize this is
> happening--means that the subprocess inherits this setting.  In this case, it
> means diff-highlight will be launched with SIGPIPE being ignored.  Let's work
> with those broken scripts by explicitly setting up a SIGPIPE handler and exiting
> the process.

My first thought was that this should be handled already by 7559a1b (unblock and unignore SIGPIPE, 2014-09-18), but after re-reading your message, it sounds like you are using diff-highlight with non-git programs?

> +# Some scripts may not realize that SIGPIPE is being ignored when launching the
> +# pager--for instance scripts written in Python.  Setting $SIG{PIPE} = 'DEFAULT'
> +# doesn't work in these instances, so we install our own signal handler instead.

Why doesn't $SIG{PIPE} = 'DEFAULT' work? I did some limited testing and it seemed to work fine for me. Though I simulated the condition with:

  (
    trap '' PIPE
    perl -e '$|=1; print "foo\n"; print STDERR "bar\n"'
  ) | true
which should not ever print "bar".

Is Python doing something more aggressive, like using sigprocmask to block the signal? I would think setting $SIG{PIPE} would handle that, but maybe not in some versions of perl. I dunno. Modern perl signal-handling is weird, as it catches everything and then defers propagation until a safe point in the script (if you strace the script above, you can see that it actually gets EPIPE from the write call!) I've no clue what implications all that has for the case you're addressing.

> +sub pipe_handler {
> +    exit(0);
> +}

Can we exit 141 here? If we are part of a pipeline to a pager, it should not matter either way, but I'd rather not lose the exit code if we can avoid it (in case of running the script standalone).

> +$SIG{PIPE} = \&pipe_handler;
A minor nit, but would:
  $SIG{PIPE} = sub { ... };
be nicer to avoid polluting the function namespace?
-Peff
John Szakmeister· Nov 4, 2014, 19:43 UTC · re: Jeff King · lore

Re: [PATCH] diff-highlight: exit when a pipe is broken

On Sat, Nov 1, 2014 at 12:04 AM, Jeff King <peff@peff.net> wrote:
Show 14 quoted lines
> On Fri, Oct 31, 2014 at 07:04:04AM -0400, John Szakmeister wrote:
>
>> While using diff-highlight with other tools, I have discovered that Python
>> ignores SIGPIPE by default.  Unfortunately, this also means that tools
>> attempting to launch a pager under Python--and don't realize this is
>> happening--means that the subprocess inherits this setting.  In this case, it
>> means diff-highlight will be launched with SIGPIPE being ignored.  Let's work
>> with those broken scripts by explicitly setting up a SIGPIPE handler and exiting
>> the process.
>
> My first thought was that this should be handled already by 7559a1b
> (unblock and unignore SIGPIPE, 2014-09-18), but after re-reading your
> message, it sounds like you are using diff-highlight with non-git
> programs?

Yes, that's correct. It's useful, so with a few tools that use diffs, I like to run the output through diff-highlight.

Show 13 quoted lines
>> +# Some scripts may not realize that SIGPIPE is being ignored when launching the
>> +# pager--for instance scripts written in Python.  Setting $SIG{PIPE} = 'DEFAULT'
>> +# doesn't work in these instances, so we install our own signal handler instead.
>
> Why doesn't $SIG{PIPE} = 'DEFAULT' work? I did some limited testing and
> it seemed to work fine for me. Though I simulated the condition with:
>
>   (
>     trap '' PIPE
>     perl -e '$|=1; print "foo\n"; print STDERR "bar\n"'
>   ) | true
>
> which should not ever print "bar".

Hehe, now that I see you right it out, I realize my mistake: I didn't capitalize 'default'. Trying it out again, it does appear that does the trick.

[snip]
Show 11 quoted lines
> Can we exit 141 here? If we are part of a pipeline to a pager, it should
> not matter either way, but I'd rather not lose the exit code if we can
> avoid it (in case of running the script standalone).
>
>> +$SIG{PIPE} = \&pipe_handler;
>
> A minor nit, but would:
>
>   $SIG{PIPE} = sub { ... };
>
> be nicer to avoid polluting the function namespace?

Sorry, my Perl-fu is kind of low these days. I used to use it all the time but switched away from it quite a while ago. Given that 'DEFAULT' does the trick, I'll just re-roll my patch to use that. Does that sound fair?

-John
PS  Sorry for the late response, I've been traveling.

← back to recent threads