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

From: Jeff King <peff@peff.net>
Date: Aug 29, 2019, 14:38 UTC
Message-ID: <20190829143805.GB1746@sigill.intra.peff.net>
In-Reply-To: <131dfa37-0f65-f0c3-6f30-5b6eca12d9c0@gmail.com>

On Thu, Aug 29, 2019 at 10:27:16AM -0400, Derrick Stolee wrote:

Show 9 quoted lines

> > 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.)

I think the broken pipe is coming the _other_ way. We do send the packet from the server to the client, but since the client is still writing when the server has hung up, we get a write error instead of seeing the error packet.

So any fixes there have to happen on the client side. I am still confused about why the client is writing in this case, per the argument in 014ade7484 (upload-pack: send ERR packet for non-tip objects, 2019-04-13). It would be nice to use GIT_TRACE_PACKET to see what it's trying to write, but I still haven't been able to reproduce the issue.

-Peff