Re: [PATCH] run_processes_parallel(): fix order of sigpipe handling
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 9, 2026, 13:40 UTC
- Message-ID
- <xmqqo6jsw8u1.fsf@gitster.g>
- In-Reply-To
- <20260408234236.GA2980392@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 19 quoted lines
>> Currently neither subprocess is marked with the clean-on-exit bit. >> but if somebody is careless and flips the bit for these >> subprocesses, start_command() will call mark_child_for_cleanup() and >> causes sigchain_push_common() to set up cleanup_children_on_signal() >> to be called, which would lead to a very similar bug. > > Hmm, good catch. This is a bit worrisome, as we've added some > clean_on_exit calls recently, and might do so again. > >> I wonder if swapping the order of start_command() and sigchain_push() >> in these two code paths have downsides, or is it making the code worse >> just to futureproof it against a future that is unlikely to come? > > I don't think it's really making the code worse. It's putting the > SIGPIPE handling closer to where we're actually doing the writes. But I > don't like that it is a subtle thing that people writing new code have > to worry about. And I wouldn't be surprised if there are other cases > where we disable SIGPIPE, and then call start_command() in a much lower > level of the call chain, which would make reordering harder.
OK. Such a change is easy, compared to anything we try to do better ;-).
Show 8 quoted lines
> I wonder if we can do better. > > The root of the issue is that sigchain_push_common() is not really > expressing what we want. We are trying to install a handler to do > cleanup _if_ we are about to exit. And that is always going to be true > for SIGTERM, etc, where we might install handlers but never expect them > to rescue us from exiting. But for SIGPIPE, our desired action is really > dependent on whether the signal is being ignored in general.
Hmph, I actually was hoping that we do not have to go in the route that each specific signal differently, but I guess it is unavoidable as they come with their own semantics (somewhat similar to the way how their default disposition has to be different).
> That's a total rewrite of our signal handling, though. There might be > details I haven't considered. > > All of that is out-of-scope for -rc2, though. ;)
Of course.