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

From: Jeff King <peff@peff.net>
Date: Aug 22, 2024, 12:41 UTC
Message-ID: <20240822124100.GA1070988@coredump.intra.peff.net>
In-Reply-To: <cover.1724080006.git.ps@pks.im>

On Mon, Aug 19, 2024 at 05:39:38PM +0200, Patrick Steinhardt wrote:

Show 6 quoted lines

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

-Peff