On Wed, Jan 26, 2022 at 10:30:40PM +0100, Ævar Arnfjörð Bjarmason wrote:
> But we can instead amend the code added in baa7b67d091 (HTTP slot
> reuse fixes, 2006-03-10) to get rid of "int *finished" entirely. I
> instrumented the code to add this after every use of slot->finished or
> slot->in_use:
>
> if (slot->finished && slot->in_use == *slot->finished) BUG("in-use = %d and finished = %d disconnect", slot->in_use, *slot->finished);
> if (!slot->finished && !slot->in_use) BUG("have !in-use and no finished pointer");
>
> Which never fires, but we would get occurrences of:
>
> if (!slot->finished && slot->in_use) BUG("have in-use and no finished pointer");
>
> I.e. we can simply drop the field and rely on "slot->in_use" in cases
> where we used "finished" before. The two fields were mirror images of
> each other, and the tri-state nature of "finished" wasn't something we
> relied upon.
This sort of thing always makes me a little nervous, regardless of how
carefully it's done. I'm not sure I quite follow the above reasoning,
but let's take a look at the code...
> diff --git a/http-walker.c b/http-walker.c
> index 910fae539b8..5cc369dea85 100644
> --- a/http-walker.c
> +++ b/http-walker.c
> @@ -225,13 +225,9 @@ static void process_alternates_response(void *callback_data)
> alt_req->url->buf);
> active_requests++;
> slot->in_use = 1;
> - if (slot->finished != NULL)
> - (*slot->finished) = 0;
Makes sense; here we set slot->in_use to 1, and would have set
slot->finished to 0.
> if (!start_active_slot(slot)) {
> cdata->got_alternates = -1;
> slot->in_use = 0;
> - if (slot->finished != NULL)
> - (*slot->finished) = 1;
Vice-versa here, OK.
> diff --git a/http.c b/http.c
> index 229da4d1488..4507c9ac9c7 100644
> --- a/http.c
> +++ b/http.c
> @@ -197,9 +197,6 @@ static void finish_active_slot(struct active_request_slot *slot)
> closedown_active_slot(slot);
> curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CODE, &slot->http_code);
>
> - if (slot->finished != NULL)
> - (*slot->finished) = 1;
> -
But this one I don't quite follow. Or, at least, I don't readily see
that slot->in_use is necessarily going to be 0 here, or (if it isn't)
that we somehow don't care.
Could you walk me through your reasoning on why this particular hunk is
OK?
> @@ -1327,10 +1323,8 @@ void run_active_slot(struct active_request_slot *slot)
> fd_set excfds;
> int max_fd;
> struct timeval select_timeout;
> - int finished = 0;
>
> - slot->finished = &finished;
> - while (!finished) {
> + while (slot->in_use) {
> step_active_slots();
>
> if (slot->in_use) {
This part of the diff looks OK to me; you're just swapping the use of
'!finished' with 'slot->in_use', which makes sense *assuming* that they
are truly mirror images of each other.
The rest of the diff looks good to me, but I do think we should nail
down an answer to the question that I posed earlier in this message
first.
Thanks,
Taylor
