Re: [PATCH] fetch-pack: ignore SIGPIPE when writing to index-pack

Jeff King [off-list ref] writes:

When fetching, we send the incoming pack to index-pack (or
unpack-objects) via the sideband demuxer. If index-pack hits an error
(e.g., because an object fails fsck), then it will die immediately. This
may cause us to get SIGPIPE on the fetch, as we're still trying to write
pack contents from the sideband demuxer (which is typically a thread,
and thus takes down the whole fetch process).

So, ... we'd die anyway and won't update the refs and anything that
leaves permanent damage to the repository either way, but we choose
a better way to die by not taking SIGPIPE, but to get an error from
one of the write()s or the final close(), which will lead us to more
"controlled" death using the normal error path?

This is mostly cosmetic. The actual error of interest (in this case, the
object that failed the fsck check) comes from index-pack straight to
stderr, so the user still sees it. They _might_ even see fetch-pack
complaining about index-pack failing, because the main thread is racing
with the sideband-demuxer. But they'll definitely see the signal death
in the exit code, which is what the test is complaining about.

OK.

We can make this more predictable by just ignoring SIGPIPE. The sideband
demuxer uses write_or_die(), so it will notice and stop (gracefully,
because we hook die_routine() to exit just the thread). And during this
section we're not writing anywhere else where we'd be concerned about
SIGPIPE preventing us from wasting effort writing to nowhere.

OK.

Thanks.

On Fri, Nov 19, 2021 at 02:21:58PM -0800, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

When fetching, we send the incoming pack to index-pack (or
unpack-objects) via the sideband demuxer. If index-pack hits an error
(e.g., because an object fails fsck), then it will die immediately. This
may cause us to get SIGPIPE on the fetch, as we're still trying to write
pack contents from the sideband demuxer (which is typically a thread,
and thus takes down the whole fetch process).

So, ... we'd die anyway and won't update the refs and anything that
leaves permanent damage to the repository either way, but we choose
a better way to die by not taking SIGPIPE, but to get an error from
one of the write()s or the final close(), which will lead us to more
"controlled" death using the normal error path?

Yes, exactly. We'll be exiting either way; it's just a matter of racily
changing the exit code and possibly the error message from the parent
fetch process.

So I do doubt this really hurts users much in practice, but making the
tests robust seems worth it to me (because I found it after tracking
down a flaky test failure). And because it _is_ a race in the code, I
fixed it there rather than papering over the SIGPIPE exit in the test
script.

-Peff