Re: [RFC] cbdata NG

On 07/07/2012 02:19 PM, Henrik Nordström wrote:
> lör 2012-07-07 klockan 10:24 -0600 skrev Alex Rousskov:
>
>> Confirmed. cbdataReferenceDone() and friends will not call destructors
>> before deallocating memory for the unlocked and invalid object, as
>> suspected.

> The destructor is already called by delete. The only thing that remains
> is to actually deallocate the memory.

> CBDATA_CLASS is an interface change in cbdata using delete instead of
> cbdataFree(). It's not valid to use cbdataFree() in a CBDATA_CLASS
> class.

Correct. We have invalid cbdata uses.

>> Both Henrik and Amos mentioned that a transition to something like
>> RefCount+invalidation would be desirable. I agree. However, there is one
>> big problem with accomplishing that. We have lots of code that does,
>> essentially, this:
>>
>> lowLevelHandler(... void *callback_data) {
>>
>> void *data = cbdataReference(callback_data);
>>
>> ... time passes ...
>> ... this is usually in some "exit" handler function ...
>> ... but given here to simplify illustration ...
>>
>> cbdataRerefenceDone(data);
>> }
>
> Yes, and this is the intended use of cbdata, where the one keeping the
> reference knows nothing about the type of the object it refers to.
>
> But there is still some places where cbdata is abused as a refcount
> which it never really was intended for.

The question is, which model should we support going forward? Since
fixing known problems will probably require a lot of cbdata-related code
changes anyway, now is a good time to ask that question.

> What we need to settle on is if cbdata is a reference to the object, or
> purely a callback argument that can be checked for validity. The
> original idea was that it should purely be used for callback arguments.

Agreed.

> Suggested path forward:
>
> 1. Change cbdataReference() to always return a (cbdata) type which
> essentially is a (void *) to twart any misuses of cbdata as refcount.

This will break all async job calls, pinned connections, and possibly
other code that does dereference cbdata-protected pointers [after
checking that they are still valid]. None of that code is using cbdata
to reference-count, but it does need to dereference pointers to valid
data to call a method or extract information.

> 2. Rename cbdataFree() to cbdataOldFree().

Maybe, but I would try to make cbdataFree() on a CBDATA_CLASS object
fail (at compile time) first.

> 3. Convert as many as possible to CBDATA_CLASS.

Agreed.

> But I remember now that there is one catch. There is some places where
> we have needed to use explicit cbdataInternalLock() calls by the owner
> of the object to protect from races on cancellation. These need to get
> fixed as well.

I see three such cases: tunnel.cc, log/ModDaemon.cc, and
store_client.cc. Some of those are probably no longer needed because
close notifications are now async (cancellation races were not caused by
cbdata; they were caused by "reentrant" sync calls). I have not checked
the details though.

> Alternatively come up with a new interface how callbacks are
> cancelled/invalidated when the object they belong to is no longer valid.

We do not need a new API for C-style callbacks. cbdata does that fine
(we just need to enforce certain rules at compile time so that folks
cannot violate them).

What we are missing (and finding ways to abuse cbdata to provide) is an
API to safely _share_ information that may expire. This information is
usually various Connections and AsyncJob objects. For example, to fire
an async call, the dialer has to dereference a job pointer. The dialer
does not "own" that job, of course. Technically, this is a cbdata-intent
violation today.

I think the two uses are close enough to support with one API. The key
difference here is that the original model says that only the owner of
the data can dereference the pointer while we are saying that others can
also dereference it (but only after checking for validity).

Are you OK with legalizing such cbdata use? The answer may affect the
final API.

Another problem, IMO, is that this is all wrapped in void*. We ought to
be able to do better these days, but that would require even more
changes so I would probably not touch it for now.

Thank you,

Alex.
Received on Sun Jul 08 2012 - 00:54:48 MDT