Re: [PATCH] diff: fix textconv error zombies

From: Johannes Sixt <j6t@kdbg.org>
Date: Mar 30, 2010, 22:17 UTC
Message-ID: <201003310017.24979.j6t@kdbg.org>
In-Reply-To: <7vk4stv5y5.fsf@alter.siamese.dyndns.org>

On Dienstag, 30. März 2010, Junio C Hamano wrote:

> In "if necessary; see below", I tried to find something to help me judge
> why you said "That's really not _that_ important in this case", but I
> think I failed.  Perhaps we would want a bit more detailed discussion in
> the document there?

The "if necessary; see below" refers to the cases where the caller requests a file descriptor from start_command(). It is safe to follow the recipe. But I actually would not want to expand on the special case like we have in run_textconv, where it is not strictly necessary to close the fd before finish_command(), namely when we know that the writer will exit after it has closed its end of the pipe and the reader reads until EOF.

This could deadlock in the theoretical case that the read() in the parent returns an error even though the child is still writing data; the child would hang while waiting for the pipe being drained by the parent; but the parent would wait for the child to exit, which never happens. Doing the close() before finish_command() avoids this situation because the child dies of SIGPIPE.

But since the communication is via a pipe, and read failures are highly unlikely (and a read failure in a way that the writer does not fail, too, is even more unlikely, I think), the close() before finish_command() is "not important".

-- Hannes