Re: [PATCH v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE
Jeff King wrote:
The write_or_die function will always die on an error,
including EPIPE. However, it currently treats EPIPE
specially by suppressing any error message, and by exiting
with exit code 0.
Suppressing the error message makes some sense; a pipe death
may just be a sign that the other side is not interested in
what we have to say. However, exiting with a successful
error code is not a good idea, as write_or_die is frequently
used in cases where we want to be careful about having
written all of the output, and we may need to signal to our
caller that we have done so (e.g., you would not want a push
whose other end has hung up to report success).
This distinction doesn't typically matter in git, because we
do not ignore SIGPIPE in the first place. Which means that
we will not get EPIPE, but instead will just die when we get
a SIGPIPE. But it's possible for a default handler to be set
by a parent process,
Not so much default as insane inherited, as in the example
of old versions of Python's subprocess.Popen.
I suspect this used exit(0) instead of raise(SIGPIPE) in the first
place to work around a bash bug (too much verbosity about SIGPIPE).
If any programs still have that kind of bug, I'd rather put pressure
on them to fix it by *not* working around it. So the basic idea here
looks good to me.
[...]
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -1,5 +1,15 @@
#include cache.h
+static void check_pipe(int err)
+{
+ if (err == EPIPE) {
+ signal(SIGPIPE, SIG_DFL);
+ raise(SIGPIPE);
+ /* Should never happen, but just in case... */
+ exit(141);
How about
die(BUG: another thread changed SIGPIPE handling behind my
back!);
to make it easier to find and fix such problems?
Thanks,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE
On Wed, Feb 20, 2013 at 01:51:11PM -0800, Jonathan Nieder wrote:
This distinction doesn't typically matter in git, because we
do not ignore SIGPIPE in the first place. Which means that
we will not get EPIPE, but instead will just die when we get
a SIGPIPE. But it's possible for a default handler to be set
by a parent process,
Not so much default as insane inherited, as in the example
of old versions of Python's subprocess.Popen.
It's possible that somebody could have a legitimate reason for doing so.
I just can't think of one. :)
I suspect this used exit(0) instead of raise(SIGPIPE) in the first
place to work around a bash bug (too much verbosity about SIGPIPE).
If any programs still have that kind of bug, I'd rather put pressure
on them to fix it by *not* working around it. So the basic idea here
looks good to me.
Yeah, if you look for old discussions on SIGPIPE in the git list, it is
mostly Linus complaining about the bash behavior, and this code does
date back to that era. The bash bug is long since fixed.
+ if (err == EPIPE) {
+ signal(SIGPIPE, SIG_DFL);
+ raise(SIGPIPE);
+ /* Should never happen, but just in case... */
+ exit(141);
How about
die(BUG: another thread changed SIGPIPE handling behind my
back!);
to make it easier to find and fix such problems?
You mean for the should never happen bit, not the first part, right? I
actually wonder if we should simply exit(141) in the first place. That
is shell exit-code for SIGPIPE death already (so it's what our
run_command would show us, and what anybody running us through shell
would see).
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE
Jeff King wrote:
On Wed, Feb 20, 2013 at 01:51:11PM -0800, Jonathan Nieder wrote:
+ if (err == EPIPE) {
+ signal(SIGPIPE, SIG_DFL);
+ raise(SIGPIPE);
+ /* Should never happen, but just in case... */
+ exit(141);
How about
die(BUG: another thread changed SIGPIPE handling behind my
back!);
to make it easier to find and fix such problems?
You mean for the should never happen bit, not the first part, right? I
actually wonder if we should simply exit(141) in the first place. That
is shell exit-code for SIGPIPE death already (so it's what our
run_command would show us, and what anybody running us through shell
would see).
Yes, for the should never happen part. Raising a signal is nice
because it means the wait()-ing process can see what happened by
checking WIFSIGNALED(status).
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE
On Wed, Feb 20, 2013 at 02:01:14PM -0800, Jonathan Nieder wrote: How about die(BUG: another thread changed SIGPIPE handling behind my back!); to make it easier to find and fix such problems? You mean for the should never happen bit, not the first part, right? I actually wonder if we should simply exit(141) in the first place. That is shell exit-code for SIGPIPE death already (so it's what our run_command would show us, and what anybody running us through shell would see). Yes, for the should never happen part. Raising a signal is nice because it means the wait()-ing process can see what happened by checking WIFSIGNALED(status). Right. My point is that only happens if there's no shell in the way. But I guess it doesn't hurt to make the attempt to help the people using wait() directly. I don't mind adding a BUG: message like you described, but we should still try to exit(141) as the backup, since that is the shell-equivalent code to the SIGPIPE signal death. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE
Jeff King wrote: On Wed, Feb 20, 2013 at 02:01:14PM -0800, Jonathan Nieder wrote: How about die(BUG: another thread changed SIGPIPE handling behind my back!); to make it easier to find and fix such problems? You mean for the should never happen bit, not the first part, right? I actually wonder if we should simply exit(141) in the first place. That is shell exit-code for SIGPIPE death already (so it's what our run_command would show us, and what anybody running us through shell would see). Yes, for the should never happen part. [...] I don't mind adding a BUG: message like you described, but we should still try to exit(141) as the backup, since that is the shell-equivalent code to the SIGPIPE signal death. If you want. :) I think caring about graceful degradation of behavior in the case of an assertion failure is overengineering, but it's mostly harmless. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE
On Wed, Feb 20, 2013 at 02:06:37PM -0800, Jonathan Nieder wrote:
I don't mind adding a BUG: message like you described, but we should
still try to exit(141) as the backup, since that is the shell-equivalent
code to the SIGPIPE signal death.
If you want. :)
I think caring about graceful degradation of behavior in the case of
an assertion failure is overengineering, but it's mostly harmless.
I am more concerned that the assertion is not oops, another thread is
doing something crazy, and it is a bug, but rather that there is some
weird platform where SIG_DFL does not kill the program under SIGPIPE.
That seems pretty crazy, though. I think I'd squash in something like
this:
diff --git a/write_or_die.c b/write_or_die.c
index b50f99a..abb64db 100644
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -5,7 +5,9 @@ static void check_pipe(int err)
if (err == EPIPE) {
signal(SIGPIPE, SIG_DFL);
raise(SIGPIPE);
+
/* Should never happen, but just in case... */
+ error(BUG: SIGPIPE on SIG_DFL handler did not kill us.);
exit(141);
}
}
which more directly reports the assertion that failed, and degrades
reasonably gracefully. Yeah, it's probably overengineering, but it's
easy enough to do.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE
Jeff King p...@peff.net writes:
I am more concerned that the assertion is not oops, another thread is
doing something crazy, and it is a bug, but rather that there is some
weird platform where SIG_DFL does not kill the program under SIGPIPE.
That seems pretty crazy, though. I think I'd squash in something like
this:
diff --git a/write_or_die.c b/write_or_die.c
index b50f99a..abb64db 100644
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -5,7 +5,9 @@ static void check_pipe(int err)
if (err == EPIPE) {
signal(SIGPIPE, SIG_DFL);
raise(SIGPIPE);
+
/* Should never happen, but just in case... */
+ error(BUG: SIGPIPE on SIG_DFL handler did not kill us.);
exit(141);
}
}
which more directly reports the assertion that failed, and degrades
reasonably gracefully. Yeah, it's probably overengineering, but it's
easy enough to do.
Yeah, that sounds like a sensible thing to do, as it is cheap even
though we do not expect it to trigger.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
