On Fri, Mar 01, 2019 at 04:02:22PM +0100, Johannes Schindelin wrote: > > I think that patch does the write_or_die conversion to handle EPIPE, but > > it would still need to turn off SIGPIPE for the whole process. > > > > So you'd also need to stick a: > > > > sigchain_push(SIGPIPE, SIG_IGN); > > > > somewhere near the start of cmd_fetch(). (There may be a less > > coarse-grained place to put it, but at this point I think we're just > > trying to find out whether this approach even solves the problem). > > This fixes it, it seems. I let the job run with `--stress=50` and even > after half an hour, it did not fail: > > attempts ://git.visualstudio.com/git/_build/results?buildId=354 Cool, I'm glad it works! > (I had to cancel it, I thought that `--stress=50` would stop trying after > 50 runs, but I was obviously incorrect in that assumption...) No, that will do 50 simultaneous jobs. You want --stress-limit=50. It might make more sense to have --stress-jobs, and make --stress=X set the limit, as I think it's much more useful to set the limit than it is to set the number of jobs. At least that's my experience. > So. Good, we have a diff that works. But you mentioned that you'd like to > put it somewhere else? I am a bit unfamiliar with the code paths of > `cmd_fetch()`, so I would be hard pressed to find a better spot. Any hint? Well, I'm of two minds there. If we want to make the minimum change, then we'd just want to disable SIGPIPE while we're actually conversing with the other side. So I guess that would be somewhere in do_fetch(), before we start talking to the other side, and restoring it after we've finished our half of the conversation (i.e., after we've done all the want/have bits and we're receiving the pack). But that's actually pretty awkward to do, because most of those details are handled under the hood by the transport code. So probably something like this is the least-terrible place to put it: diff --git a/builtin/fetch.c b/builtin/fetch.c index b620fd54b4..4ba63d5ac6 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1556,7 +1556,9 @@ static int fetch_one(struct remote *remote, int argc, const char **argv, int pru sigchain_push_common(unlock_pack_on_signal); atexit(unlock_pack); + sigchain_push(SIGPIPE, SIG_IGN); exit_code = do_fetch(gtransport, &rs); + sigchain_pop(SIGPIPE); refspec_clear(&rs); transport_disconnect(gtransport); gtransport = NULL; That said, I actually think it's kind of pointless for git-fetch to use SIGPIPE at all. The purpose of SIGPIPE is that you can write a naive program that spews output, and you'll get informed (forcefully) by the OS if the process consuming your output stops listening. That makes sense for programs like "git log", whose primary purpose is generating output. But for git-fetch, our primary purpose is receiving data and writing it to disk. We should be checking all of our write()s already, and SIGPIPE is just confusing. The only "big" output we generate is the status table at the end. And even if that is going to a pipe that closes, I don't think we'd want to fail the whole command (we'd want to finalize any writes for what we just fetched, clean up after ourselves, etc). So I'd actually be fine with just declaring that certain commands (like fetch) just ignore SIGPIPE entirely. -Peff
