Re: [PATCH] sigchain: recover from concurrent double-pops of the same signal

Hi Phillip,

Thanks for the follow-up.  All three points make sense; let me
confirm what I plan to do so we are on the same page.

On SIGPIPE: you are right that my wording was wrong.  Blocking is
not ignoring, and the write() fails with EPIPE either way.  What
changes is that the signal is not delivered while it is blocked;
and since a SIGPIPE raised by a write is thread-directed, it stays
pending on the writing thread instead of being picked up by the
main thread.  If that thread never unblocks it, it is never
delivered, so a worker that blocks SIGPIPE simply stops dying of
it.  I left SIGPIPE out precisely to keep the "upstream of a pipe
dies of SIGPIPE" behaviour that common-init.c restores.  The set I
plan to block is SIGINT, SIGHUP, SIGTERM and SIGQUIT.  If you
think SIGPIPE belongs in it too, I will add it.

On the blocking pattern: agreed.  I will block once around the
spawn loop and restore before the join loop, rather than around
each pthread_create(), via a small guard pair

    struct signal_mask_guard guard;

    block_sigchain_signals(&guard);
    for (i = 0; i < nr_threads; i++)
        pthread_create(&t[i], NULL, fn, &data[i]);
    restore_sigchain_signals(&guard);

plus a git_pthread_create() wrapper on top for the single-thread
call sites.  The loops I have in mind are preload_index(),
index-pack, pack-objects, grep, name-hash and the read-cache entry
loader; the fsmonitor--daemon and simple-ipc threads I would leave
alone for now, as they have their own lifecycle and signal
handling.

On sigaction: agreed as well.  The handled signal is deferred by
default, so the remaining hole is a handler being interrupted by
one of the other common signals that share the same handler.  I
will set sa_mask to sigfillset() and save/restore the whole
struct sigaction, so pop() does not clobber the caller's
sa_flags/sa_mask.

The one gap I cannot close is a thread created by library code
rather than by us.  libcurl's threaded resolver is the case I am
aware of, depending on how libcurl is built; I do not know of an
in-tree instance, and I will mention it in the cover letter rather
than claim it is covered.

So I will drop this patch and build the series along those lines.
As you suggested, I will wait a few days for Peff and Junio to
comment before spending much time on it; if nobody objects I will
send the replacement as v2.

Thanks,
ian4hu