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

From: Junio C Hamano <[email-masked]>
Date: Mon, 23 May 2022 17:02:17 -0700

Johannes Schindelin <[email-masked]> writes:

> It calls into cURL, which I suspect has a multi-threaded mode of
> operation,

https://curl.se/libcurl/c/threadsafe.html ;-)

My understanding is that what we have is pretty much select() driven
single-threaded multi-fd transfer.

> No, I suggested to replace the `finished` variable with an attribute (or
> "field" or "member variable") of the slot, and to respect it when looking
> for an unused slot, i.e. not only look for a slot whose `in_use` is 0 but
> also require `reserved_for_use` to be 0. In essence, the
> `run_active_slot()` function owns the slot, even if it is not marked as
> `in_use`. That should address the same concern as baa7b67d but without
> using a pointer to a local variable.

Not really.  An outer run_active_slot() and an inner
run_active_slot() have a pointer to the same slot object.

The inner one got hold of that object because the request the slot
used to represent for the outer run_active_slot() has finished, so
we would toggle either *(slot->finished) or the new slot->done in an
attempt to signal the completion to the outer run_active_slot() and
then make the slot not-in-use.  The slot becomes in-use again with a
different request and the inner run_active_slot() is run.  It first
says "this slot is not done yet---we are making a request using
it".  How would the inner one say that, exactly?

In baa7b67d's fix, it is done by setting slot->finished = &finished
to its own stackframe.  Because the outer run_active_slot() does not
look at slot->finished, but it looks at the finished on its
stackframe, what the inner run_active_slot() does here would not
break the outer one.

If we replace the mechanism with a separate member in the slot
structure, so that the outer run_active_slot() looks at slot->done
and the inner run_active_slot() also clears slot->done before
proceeding, then the inner one clobbers what the outer one will look
at when the recursive call that led to the inner one returns.