git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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.
Previous: Jeff King
Message 18 of 18 in “Help needed on 2.54.0-rc0 t5301.13 looping.”
  1. rsbecker@nexbridge.comApr 7, 2026
  2. Jeff KingApr 8, 2026
  3. Jeff KingApr 8, 2026
  4. Adrian RatiuApr 8, 2026
  5. rsbecker@nexbridge.comApr 8, 2026
  6. rsbecker@nexbridge.comApr 8, 2026
  7. rsbecker@nexbridge.comApr 8, 2026
  8. Junio C HamanoApr 8, 2026
  9. rsbecker@nexbridge.comApr 8, 2026
  10. Adrian RatiuApr 8, 2026
  11. t5401: test SIGPIPE with parallel hooksJeff King, Apr 8, 2026
  12. Junio C HamanoApr 8, 2026
  13. Adrian RatiuApr 8, 2026
  14. run_processes_parallel(): fix order of sigpipe handlingJeff King, Apr 8, 2026
  15. Junio C HamanoApr 8, 2026
  16. Junio C HamanoApr 8, 2026
  17. Jeff KingApr 8, 2026
  18. Junio C HamanoApr 9, 2026

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.