Re: [RFC] cbdata NG

On 8/07/2012 12:54 p.m., Alex Rousskov wrote:
> 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.

We have RefCount as a separate type with plans to migrate that to stdlib
when the stdlib is faster.
Robert and I had a talk about this at AusMeet and we agreed to convert
the cbdata-as-refcount to pure RefCount.

Since we have no code built with .c any more I think we should be aiming
at erasing the C APIs whenever we have overlapping C++ ones like these
two cbdata ones.

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

That would seem to be done easiest by making cbdataFree() [new version]
depend on the CBDATA_CLASS2 toCbdata() method and fixing all the compile
errors by adding CBDATA_CLASS2 to the base objects.

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

If removal is not working a local CbcPointer variable would fix these
would it not? since the CbcPointer creates a local-scope reference lock
without the handler needing explicit cbdataInternal*().

These would seem to be a small step-1 patch I'm happy porting back to
3.2 by itself while we sort out the rest.

>
>
>> 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 - 03:47:51 MDT