Jeff King [off-list ref] writes:
I'd be happy if we just ignored SIGPIPE everywhere, but turned it on for
the log family.
Hmmmm, now you confused me... What is special about the log family? Do
you mean "when we use pager"? But then we are writing into the pager,
which the user can make it exit, which in turn causes us to write into the
pipe, so I would expect that we would want to ignore SIGPIPE --- ah, then
we explicitly catch error in xwrite() and say die() which we do not want.
So you want to let SIGPIPE silently kill us when the pager is in use; is
that it?
On Fri, Feb 17, 2012 at 11:27:26PM -0800, Junio C Hamano wrote:
quoted
I'd be happy if we just ignored SIGPIPE everywhere, but turned it on for
the log family.
Hmmmm, now you confused me... What is special about the log family? Do
you mean "when we use pager"? But then we are writing into the pager,
which the user can make it exit, which in turn causes us to write into the
pipe, so I would expect that we would want to ignore SIGPIPE --- ah, then
we explicitly catch error in xwrite() and say die() which we do not want.
Sort of. I mentioned the log family because those are the commands that
tend to generate large amounts of input, and which are likely to hit
SIGPIPE. But let me elaborate on my thinking a bit.
There are basically three types of writing callsites in git:
1. Careful calls to write() or fwrite().
These are the majority of calls. In these, we check the return
value of write() and die, return the error code, or whatever is
appropriate for the callsite. SIGPIPE kills the program before the
careful code gets the chance to make a decision about what to do.
At best, this is a nuisance; even if the program is going to die(),
it's likely that the custom code could produce a useful error
message. At worst it causes bugs. For example, we may write to a
subprocess that only needs part of our input to make a decision
(e.g., some hooks and credential helpers). With SIGPIPE, we end up
dying even though no error has occurred. Worse, these are often
annoying heisenbugs that depend on the timing of write() and
close() between the two processes.
2. Non-careful calls of small, fixed-size output.
Things like "git remote" will write some output to stdout via
printf and not bother checking the return code. As the main purpose
of the program is to create output, it's OK to be killed by
SIGPIPE if it happens due to a write to stdout. But respecting
SIGPIPE doesn't buy as all that much. It might save us from making
a few write() calls that will go to nowhere, but most of the work
has already been done by the time we are outputting.
And it has a cost to the other careful write calls in the same
program, because respecting SIGPIPE is a per-process thing. So for
something like "git remote show foo", while we would like SIGPIPE
to kick in for output to stdout, we would not want it for a pipe we
opened to talk to ls-remote.
3. Non-careful calls of large, streaming data.
Commands like "git log" will non-carefully output to stdout, as
well, but they will generate tons of data, consuming possibly
minutes of CPU time (e.g., "git log -p"). If whoever is reading the
output stops doing so, we really want to kill the program to avoid
wasting CPU time. In this instance, SIGPIPE is a big win.
It still has the downside that careful calls in the same program
will be subject to using SIGPIPE. For "log" and friends, this is
probably OK with the current code, as we don't make a lot of pipes.
But that is somewhat of an implementation detail. E.g., "git log
-p" with external diff or textconv writes to a tempfile, and then
runs a subprocess with the tempfile as input. But we could just as
easily have used pipes, and may choose to do so in the future. You
may even be able to trigger a convert_to_git filter in the current
code, which does use pipes.
So basically, I find SIGPIPE to be a simplistic too-heavy hammer that
ends up affecting all writes to pipes, when in reality we are only
interested in affecting ones to some "main" output (usually stdout).
That works OK for small Unix-y programs like "head", but is overly
simplistic for something as big as git.
In an ideal world, we could set a per-descriptor version of SIGPIPE, and
just turn it on for stdout (or somehow find out which descriptor caused
the SIGPIPE after the fact). But that's not possible.
So our next best thing would be to actually check the results of
these non-careful writes. Unfortunately, this means either:
a. wrapping every printf with a function that will appropriately die
on error. This makes the code more cumbersome.
or
b. occasionally checking ferror(stdout) while doing long streams
(e.g., checking it after each commit is written in git log, and
aborting if we saw a write error). This is less cumbersome, but it
does mean that errno may or may not still be accurate by the time
we notice the error. So it's hard to reliably differentiate EPIPE
from other errors. It would be nice, for example, to have git log
exit silently on EPIPE, but properly print an error for something
like ENOSPC. But perhaps that isn't a big deal, as I believe right
now that we would silently ignore something like ENOSPC.
Less robust than that is to just ignore SIGPIPE in most git programs
(which don't benefit from it, and where it is only a liability), but
then manually enable it for the few that care (the log family, and
perhaps diff. Maybe things like "git tag -l", though that output tends
to be pretty small). But I think it would work OK in practice, because
those commands don't tend to make pipes other than the "main" output.
And it's quite easy to implement.
So you want to let SIGPIPE silently kill us when the pager is in use; is
that it?
That's an OK heuristic, as the pager being in use is a good sign that we
will generate long, streaming output. It has two downsides, though. One
is that it suffers from the too-heavy hammer of the preceding paragraph.
The other is that it only catches when _we_ create the pipe. You would
also want to catch something like:
git rev-list | head
and stop the traversal. So I think it is less about whether a pager is
in use and more about whether we are creating long output that would
benefit from being cut off early. The pager is an indicator of that, but
it's not a perfect one; I think we'd do better to mark those spots
manually (i.e., by re-enabling SIGPIPE in commands that we deem
appropriate).
-Peff
Hi,
Jeff King wrote:
Less robust than that is to just ignore SIGPIPE in most git programs
(which don't benefit from it, and where it is only a liability), but
then manually enable it for the few that care
This seems backwards. Aren't the only places where it is just a
liability places where git is writing to a pipe that git has created?
We could keep the benefits of SIGPIPE (including simpler error
handling and lack of distracting EPIPE message) in most code, and only
switch to SIGPIPE-ignored semantics where the signal has a chance to
cause harm. Maybe run_command should automatically ignore SIGPIPE
when creating a pipe for the launched command's standard input (with a
flag to ask not to), as a rough heuristic.
There's a subtlety I'm glossing over here, which is that for commands
that produce a lot of output (think: "git fetch --all"), output may
still not the primary goal. I think even they should not block
SIGPIPE, to follow the principle of least surprise in the following
interaction:
git fetch --all 2>&1 | less
... one page later, get bored ...
q (to quit)
Most Unix programs would be killed by SIGPIPE after such a sequence,
so I would expect git to be, too.
Just my two cents,
Jonathan
On Sat, Feb 18, 2012 at 04:06:07AM -0600, Jonathan Nieder wrote:
Jeff King wrote:
quoted
Less robust than that is to just ignore SIGPIPE in most git programs
(which don't benefit from it, and where it is only a liability), but
then manually enable it for the few that care
This seems backwards. Aren't the only places where it is just a
liability places where git is writing to a pipe that git has created?
Yes. But the problem is that those spots are buried deep within library
code, and are an implementation detail that the caller shouldn't need to
know about.
But more importantly, I see SIGPIPE as an optimization. The program on
the generating side of a pipe _could_ keep making output and dumping it
nowhere. So the optimization is about telling it early that it can stop
bothering. But that optimization is affecting correctness in some cases.
And in cases of correctness versus optimization, it's nice if we can be
correct by default and then optimize in places where it matters most and
where we've verified that correctness isn't harmed.
I also think it's simply easier to list the places that benefit from
SIGPIPE than those that are harmed by it. But if we wanted to go the
other way (allow by default and turn it off in a few programs), at the
very least all of the network programs (receive-pack. upload-pack, etc)
should start ignoring it completely.
We could keep the benefits of SIGPIPE (including simpler error
handling and lack of distracting EPIPE message) in most code, and only
switch to SIGPIPE-ignored semantics where the signal has a chance to
cause harm. Maybe run_command should automatically ignore SIGPIPE
when creating a pipe for the launched command's standard input (with a
flag to ask not to), as a rough heuristic.
That's a reasonable heuristic, but not foolproof. If I have a
long-running subprocess with a pipe, then SIGPIPE will be off for a long
time, meaning the code you thought was protected by it is not. In
practice, I think it would work OK with our current code-base, as we
tend to have short-lived subprocesses. You'd have to special case the
starting of the pager, of course, but that's not too hard.
There's a subtlety I'm glossing over here, which is that for commands
that produce a lot of output (think: "git fetch --all"), output may
still not the primary goal. I think even they should not block
SIGPIPE, to follow the principle of least surprise in the following
interaction:
git fetch --all 2>&1 | less
... one page later, get bored ...
q (to quit)
Most Unix programs would be killed by SIGPIPE after such a sequence,
so I would expect git to be, too.
But it's quite non-deterministic whether or when git-fetch will be
killed. It may have written all data to the pipe. You may quit, but
fetch still runs for a while before producing output. If you want it
killed, you are much better off to do so by sending it SIGINT.
-Peff