Re: [PATCHv2 02/10] pkt-line: drop safe_write function
On Mon, Feb 18, 2013 at 01:56:45AM -0800, Jonathan Nieder wrote: > Jeff King wrote: > > > This is just write_or_die by another name. > > > > Signed-off-by: Jeff King > > --- > > 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. > 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). > [...] > > --- 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... > 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. > 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. > 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 -- 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.o
Re: [PATCHv2 02/10] pkt-line: drop safe_write function
Jeff King wrote:
> This is just write_or_die by another name.
>
> Signed-off-by: Jeff King
> ---
> 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.
> Which makes me a little nervous.
Me, too. Let's see:
[...]
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -908,7 +908,7 @@ static void report(struct command *commands, const char
> *unpack_status)
> if (use_sideband)
> send_sideband(1, 1, buf.buf, buf.len, use_sideband);
> else
> - safe_write(1, buf.buf, buf.len);
> + write_or_die(1, buf.buf, buf.len);
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.
Since the pipe is closed, presumably the calling send-pack has hung up
and won't notice the exit status, so this should be safe.
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.
[...]
> --- 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.
[1]
http://www.chiark.greenend.org.uk/ucgi/~cjwatson/blosxom/2009-07-02-python-sigpipe.html
[...]
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -245,7 +245,7 @@ static void send_request(struct fetch_pack_args *args,
> send_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX);
> packet_flush(fd);
> } else
> - safe_write(fd, buf->buf, buf->len);
> + write_or_die(fd, buf->buf, buf->len);
Also a no-op except when the parent process is insane enough to let us
inherit signal(SIGPIPE, SIG_IGN).
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.
[...]
> --- a/http-backend.c
> +++ b/http-backend.c
> @@ -70,7 +70,7 @@ static void format_write(int fd, const char *fmt, ...)
> if (n >= sizeof(buffer))
> die("protocol error: impossibly long line");
>
> - safe_write(fd, buffer, n);
> + write_or_die(fd, buffer, n);
Etc. I'm stopping here.
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.
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).
Or alternatively:
2b. never set SIGPIPE to SIG_IGN except in short blocks of code that
do not call write_or_die()
What do you think?
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
