Re: [PATCH 1/1] upload-pack: fix race condition in error messages

On 8/29/2019 10:13 AM, Jeff King wrote:

On Thu, Aug 29, 2019 at 08:58:55AM -0400, Derrick Stolee wrote:

quoted

However, I do have a theory: the process exits before flushing the
packet line. Adding this line before exit(1) should fix it:

	packet_writer_flush(writer);

I can send this in a v2, but it would be nice if you could test this
in your environment that already demonstrated the failure.

I don't think we should need such a call. For one thing, if it were
necessary, that would mean we're not writing out the packet at all. But
your whole problem is that we're writing the message twice, one of which
comes from the packet.

The problem the flush() was trying to solve was the new "Broken pipe" error,
which I had assumed was due to a communication race. (Looking at the message
more closely now, I see that Szeder was able to repro this broken pipe both
with and without my change. I am still unable to repro the broken pipe.)

Second is that this is not "flush the output stream", but "write a flush
packet". The packet_writer_error() function immediately calls write()
without buffering. And no matter where we are in the conversation, a
flush packet would not be necessary, because the error packet we send
would be interpreted immediately by the client as aborting the
connection.

This clearly shows that my proposed solution is absolutely wrong.

Thanks,
-Stolee