Re: [PATCH] http.c: clear the 'finished' member once we are done with it

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 3352 bytes --]

Hi Junio,

On Tue, 24 May 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
> >> I _think_ we can even get away by not doing anything to
> >> slot->finished at the end of run_active_slot(), as we are not
> >> multi-threaded and the callee only returns to the caller, but if it
> >> helps pleasing the warning compiler, I'd prefer the simplest
> >> workaround, perhaps with an unconditional clearing there?
> >
> > I'll admit I haven't fully looked into this again, but does anything in
> > the subsequent analysis suggest that my original patch wouldn't be a
> > working solution to this, still:
> > https://lore.kernel.org/git/patch-1.1-1cec367e805-20220126T212921Z-avarab@gmail.com/ ?
>
> I traced _one_ code path as a demonstration to show why the current
> "slot->finished = &finished" based solution works.
>
> But I think what we need is to demonstrate a code path in the old
> version that shows why the old slot->in_use would not have worked
> and the slot->finished was needed, and demonstrate why it NO LONGER
> is the case in today's code.  Without that, especially with the
> latter, I cannot take the "just revert 16-year old bugfix because a
> new compiler throws a warning related to multi-threaded code to it,
> even though we are strictly single-threaded" as a serious solution.
>
> And because I do not think I've seen anybody has done that necessary
> digging, I would still prefer the "if the compiler somehow cares,
> then let's clear the finished member once we are done with it" much
> better than "we do not know why but we somehow think we can do
> without this bugfix, even though we wouldn't be making noises about
> this piece of code if a new compiler did not start emitting a
> warning".

The commit in question is baa7b67d091 (HTTP slot reuse fixes, 2006-03-10),
and I did look around in the Git mailing list archives for mails that were
sent around the same date, but did not see much that would help understand
the context, except that the patch series clearly talks about `http-push`:
https://lore.kernel.org/git/20060311041749.GB3997@reactrix.com/

The thing about `http-push` is that it adds a "fill function" that is
executed in `fill_active_slots()`, which is called in turn by
`step_active_slots()`, which, as you will recall, is called within that
busy loop in `run_active_slot()`.

And that "fill function" is where it starts to get interesting.

It's called `fill_active_slot()`:
https://github.com/git/git/blob/v2.36.1/http-push.c#L604-L625

This function starts requests, such as fetching loose objects, starting a
PUT or a MKCOL. Notably, though, `fill_active_slot()` does not wait for
the request to finish. In other words, it will potentially reuse the
current slot if it was _just_ marked as no longer in use, and then the
code flow will eventually return to that loop in `run_active_slot()`, with
any reused slot still being marked as `in_use`.

So yes, reverting that commit would reintroduce the regression, and I am
very happy that we now have a grip on this Chesterton's Fence.

This same analysis, of course, also puts a nail into the coffin of the
`reserved_for_use` idea because while it would fix the reuse bug, it would
unnecessarily squat on slots that might well be needed.

Ciao,
Dscho