Re: [PATCH 00/10] reftable: fix reload with active iterators

> On Mon, Aug 19, 2024 at 05:39:38PM +0200, Patrick Steinhardt wrote:
> 
> > This patch series fixes that issue by starting to refcount the readers.
> > Each iterator will bump the refcount, thus avoiding the problem. While
> > at it, I also found a second issue where we segfault when reloading a
> > table fails while reusing one of the table readers. In this scenario, we
> > would end up releasing the reader of the stack itself, even though it
> > would still be used by it.
> 
> I gave a fairly cursory look over this, as I'm not all that familiar
> with the reftable code. But everything looked pretty sensible to me.

> I wondered how we might test this. It looks like you checked the
> --stress output (which I confirmed seems fixed for me), along with a
> synthetic test directly calling various reftable functions. Both seem
> good to me.
> 
> I had hoped to be able to have a non-racy external test, where running
> actual Git commands showed the segfault in a deterministic way. But I
> couldn't come up with one. One trick we've used for "pausing" a reading
> process is to run "cat-file --batch" from a fifo. You can ask it to do
> some ref lookups, then while it waits for more input, update the
> reftable, and then ask it to do more.
> 
> But I don't think that's sufficient here, as the race happens while we
> are actually iterating. You'd really need some for_each_ref() callback
> that blocks in some externally controllable way. Possibly you could do
> something clever with partial-clone lazy fetches (where you stall the
> fetch and then do ref updates in the middle), but that is getting pretty
> convoluted.
> 
> So I think the tests you included seem like a good place to stop.

> I did have a little trouble applying this for testing. I wanted to do it
> on 'next', which has the maintenance changes to cause the race. But
> merging it there ended up with a lot of conflicts with other reftable
> topics (especially the tests). I was able to resolve them all, but you
> might be able to make Junio's life easier by coordinating the topics a
> bit.