Re: [PATCH] remote-curl: Fix push status report when all branches fail

On Fri, Jan 20, 2012 at 09:03:31AM -0800, Shawn O. Pearce wrote:

This hunk is causing intermittent failures of t5541 for me, especially when the system is under heavy load (e.g., make -j32 test). Before your patch, this is what happened:

  1. remote-curl relays the status lines from send-pack, then sees that
     send-pack reported error, and it exits

  2. push reads the status lines, looking for a blank line to terminate
     them. It sees EOF instead of the blank line and exits(128) itself.

After your patch, this happens:

  1. remote-curl relays the status lines, alway appends the blank line
     terminator, and then exits

  2. push reads the status lines, including the blank line terminator,
     and reports them to the user.

  3. push then disconnects the remote-curl helper by writing a blank
     line to it (to signal end-of-input), followed by finish_command().
     The latter propagates the error code from the exit in step 1, and
     we use that to signal failure from "git push".

There's a race condition now in step 3. The push process may write to the pipe going to remote-curl after it has exited, causing it to receive SIGPIPE and die. We can block SIGPIPE, but that's not sufficient; we'll still notice that our write() returns EPIPE and die.

Obviously we can't not print the post-push "\n" in remote-curl, for the reasons you outlined in the commit message of this patch. We also can't not exit from remote-curl on error. Even though in the test in t5541 we have signaled error via the ref statuses, we might have received an error that does not come through a ref status (e.g., if we couldn't run send-pack at all).

We can't not write the "\n" to signal end-of-input to remote-curl, because we don't actually know yet that there's an error (we find out when we wait() on the process). Barring any asynchronous SIGCHLD handling, of course, but I don't think we want to get into that.

So it's kind of a bug in the remote helper protocol. The helpers can signal failure only by dying, but we can find out about that failure only after disconnecting, which involves writing to them. It would be much more sane if the helpers returned an overall text status from each command (e.g., printed "error push failed" instead of dying).

But that would involve changing the protocol, of course. I think our best option is to work around it by considering the final blank line we send before disconnect as "best effort". That is, it is a courtesy to the remote helper to tell it we are hanging up cleanly, and if it does not arrive, then we can ignore the problem and proceed with closing the pipe. I.e., something like: