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

From: Junio C Hamano <gitster@pobox.com>
Date: May 24, 2022, 00:02 UTC
Message-ID: <xmqqa6b7lrw6.fsf@gitster.g>
In-Reply-To: <nycvar.QRO.7.76.6.2205240124280.352@tvgsbejvaqbjf.bet>

Johannes Schindelin <Johannes.Schindelin@gmx.de> 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.

Show 7 quoted lines

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