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

From: Junio C Hamano <gitster@pobox.com>
Date: May 23, 2022, 22:58 UTC
Message-ID: <xmqqr14jluu4.fsf@gitster.g>
In-Reply-To: <nycvar.QRO.7.76.6.2205232248360.352@tvgsbejvaqbjf.bet>

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

Show 24 quoted lines

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