Re: [PATCHv2 02/10] pkt-line: drop safe_write function

From: Jeff King <peff@peff.net>
Date: Feb 18, 2013, 10:24 UTC
Message-ID: <20130218102430.GM5096@sigill.intra.peff.net>
In-Reply-To: <20130218095644.GA7049@elie.Belkin>

On Mon, Feb 18, 2013 at 01:56:45AM -0800, Jonathan Nieder wrote:

Show 10 quoted lines

> Jeff King wrote:
> 
> > This is just write_or_die by another name.
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> > Actually, they are not quite the same. write_or_die will exit(0) when it
> > sees EPIPE.
> 
> That information definitely belongs in the commit message.

Yeah, this one was more RFC; I was hoping for some input on the EPIPE thing, so I could know _what_ to put in the commit message. Is it safe, and if so, why? And if not, we should drop the patch.

Show 6 quoted lines

> If the connection to send-pack is lost and stdout becomes a broken
> pipe and I am updating enough refs to overflow the pipe buffer,
> receive-pack will die with SIGPIPE.  So unless the sadistic caller has
> set the inherited SIGPIPE action to SIG_IGN (for example by wrapping
> git with an uncautious Python wrapper that uses subprocess.Popen), the
> change to EPIPE handling is not a behavior change.

Yeah, but I don't want to count on always catching SIGPIPE. There's the inherited signal handler thing, but there's also the fact that we may end up ignoring SIGPIPE from backend programs like upload-pack and receive-pack; they check their writes anyway, and we have already run into issues with getting SIGPIPE when we don't necessarily expect or care about it.

The nice thing about write_or_die is that it still _exits_ on EPIPE. It's just that it doesn't print an error (which is really not a big deal) and exit with a 0 return code. I really wonder if we should just change the latter. For programs which are creating copious output (e.g., "git log"), the return value is not important anyway. For backend programs, an unexpected EPIPE from something like write_or_die should probably involve a non-successful return code.

> Arguably it would be more friendly to stay alive to run the
> post-receive and post-update hooks, though, given that a ref update
> has occurred.  Maybe transport commands like this one should always
> set the disposition of SIGPIPE to SIG_IGN.

Yeah, I've suggested that in the past. And I do think it's sane, because if you took a ref update, you almost certainly want to run the post-receive, even if the client is no longer around (e.g., if it is going to email out the changeset).

Show 15 quoted lines

> [...]
> > --- a/builtin/send-pack.c
> > +++ b/builtin/send-pack.c
> > @@ -79,7 +79,7 @@ static void print_helper_status(struct ref *ref)
> >  		}
> >  		strbuf_addch(&buf, '\n');
> >  
> > -		safe_write(1, buf.buf, buf.len);
> > +		write_or_die(1, buf.buf, buf.len);
> 
> A signal will kill send-pack before write_or_die has a chance to
> intervene so this change is a no-op unless the caller is sadistic
> (as in the [1] case).  In the signal(SIGPIPE, SIG_IGN) case, it might
> be a regression, since "git push" should not declare success when its
> connection to receive-pack closes early.

But that isn't going to receive-pack, is it? Send-pack's stdout is really just going to the user (or wherever). So it would have an effect more for:

  (git push && echo >&2 OK) | grep -m1 foo

which might print "OK" even if we failed. That's quite contrived, but it is at least a measurable change. And anyway...

Show 5 quoted lines

> In that case, if triggerable this looks like a bad change: if
> upload-pack has gone missing, the fetch should not be considered a
> success.
> [...]
> Etc.  I'm stopping here.

Yeah, there are definitely some bad ones.

Show 5 quoted lines

> I'm thinking before a patch like this we should make the following
> change:
> 
>  1. at startup, set the signal action of SIGPIPE to SIG_DFL, to make
>     the behavior a little more predictable.

I'm lukewarm on that, just because we may want to ignore SIGPIPE ourselves at some point.

Show 7 quoted lines

> Perhaps the following as well:
> 
>  2. in write_or_die(), when encountering EPIPE, set the signal action
>     of SIGPIPE to SIG_DFL and raise(SIGPIPE), ensuring the exit status
>     reflects the broken pipe.  If the parent process is unnecessarily
>     noisy about that, that's a bug in the parent process (hopefully
>     uncommon).

I like this. My suggestion would be to just exit(1) instead of exit(0). But really, raising SIGPIPE makes the most sense, because it communicates to the parent what happened (and the shell will wisely not print a message, but careful parents like "git fetch" and "git push" will check it properly and notice that the child did not succeed).

> Or alternatively:
> 
>  2b. never set SIGPIPE to SIG_IGN except in short blocks of code that
>      do not call write_or_die()

Yuck. :)

> What do you think?

I really like option 2. That exit(0) when we see SIGPIPE bugs me. Because we _are_ dying due to our write failing, and I think the only reason to exit(0) was to avoid unnecessary complaints from parents. But raising SIGPIPE seems like the best of both worlds to me.

-Peff