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

[Derrick Stolee <stolee@gmail.com>]

On 8/29/2019 10:13 AM, Jeff King wrote:
> On Thu, Aug 29, 2019 at 08:58:55AM -0400, Derrick Stolee wrote:
> 
>> 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