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

[Junio C Hamano <gitster@pobox.com>]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> I stumbled over the need for this while investigating the build failures
> caused by upgrading Git for Windows' SDK's GCC to v12.x.
>
>> diff --git a/http.c b/http.c
>> index 229da4d148..85437b1980 100644
>> --- a/http.c
>> +++ b/http.c
>> @@ -1367,6 +1367,9 @@ void run_active_slot(struct active_request_slot *slot)
>>  			select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);
>>  		}
>>  	}
>> +
>> +	if (slot->finished == &finished)
>> +		slot->finished = NULL;
>
> First of all, I suspect that
> https://github.com/git/git/blob/v2.36.1/http.c#L1207 makes sure that GCC's
> complaint is not actually accurate: we always re-set `finished` to `NULL`
> when getting an unused slot, so even if there is a left-over dangling
> pointer, it is not actually used, ever.
>
> But we need something to pacify GCC. Let's look at your patch.
>
> The first thing to note is that this is not _quite_ thread-safe: between

Does this part of the code ever run multi-threaded?

> If that analysis is correct, I would expect the correct solution to turn
> `finished` into an attribute of the slot, and change its role to be a flag
> that this slot is spoken for and cannot be re-used quite yet even if it is
> not currently in use.

I have a feeling that we've mentioned that at least twice (perhaps
three times) in the recent past that it is in essense reverting what
the "finished" change baa7b67d (HTTP slot reuse fixes, 2006-03-10)
did.  We used to use the in-use bit of the slot as an indicator that
the slot dispatched by run_active_slot() has finished (i.e. the
in-use bit must be cleared when the request held in the struct is
fully done), but that broke when a slot we are looking at in
run_active_slot() is serviced (which makes in_use false), and then
another request reuses the slot (now no longer in_use), before the
control comes back to the loop.  "while (slot->in_use)" at the
beginning of the loop was still true, but the original request the
slot was being used for, the one that the run_active_slot() function
cares about, has completed.

So...