Re: [PATCH] builtin/fetch: avoid aborting closed reference transaction

From: Justin Tobler <jltobler@gmail.com>
Date: Mar 24, 2025, 15:10 UTC
Message-ID: <vmmr2cwiyd2iz7c3yfyzym3l2aono7lsx7wig3pwmw33gst37h@orjtfygfyow4>
In-Reply-To: <Z-E2ns9VziejSLB2@pks.im>

On 25/03/24 11:40AM, Patrick Steinhardt wrote:

Show 17 quoted lines

> On Thu, Mar 20, 2025 at 07:44:37PM -0500, Justin Tobler wrote:
> > As part of the reference transaction commit phase, the transaction is
> > set to a closed state regardless of whether it was successful of not.
> > Attempting to abort a closed transaction via `ref_transaction_abort()`
> > results in a `BUG()`.
> 
> Yeah, this is one of the more intricate parts of ref transactions, and
> it has been biting me several times in the past. It feels somewhat
> similar in spirit to how the `ref_iterator` used to automatically free
> itself once it has reached its end, which led to the same class of bugs
> due to the interface being way too intricate.
> 
> So I wonderer whether we should refactor this interface in the same way:
> instead of automatically freeing the transaction on commit/abort, we'd
> never do so and require the caller to always free it themselves. This
> would make it way easier to use because we can now unconditionally free
> the transaction everywhere.

I was also considering this. The interface here feels rather awkward since aborted transactions free themselves automatically while committed ones do not. It would be easier to reason about if the caller was always reponsible for freeing the transaction.

> That wouldn't help with the fixed bug though, which is that we call
> abort after a failed commit even though the transaction was already
> aborted.

I wonder if it would make sense to stop closing the transaction on a failed commit and require the caller to abort it. This would allow error handling to unconditionally abort the transaction during cleanup.

I wouldn't mind sending a followup series to refactor these interfaces if that is something we would be interested in.

-Justin