Re: [RFC] cbdata NG

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.

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

> As you can see, cbdataRerefenceDone() operates on a typeless data.
> Nothing we can do to cbdata*() will call the true data destructor(s) in
> this context. To solve thus, we must replace void* with a real type.
> Since low-level handlers (e.g. Comm) cannot know high-level types, this
> real type must be a base class with a virtual destructor.

No, on both.

* cbdataReferenceDone() SHOULD NOT call the destructor on a CBDATA_CLASS
type object.

* On types where it do call the destructor the callback performing the
destruction is responsible of casting the void * back into the right
type.

The issue is NOT the complexity of the object. It's the difference in
API between old style cbdataAlloc/Free and new style CBDATA_CLASS*. If
you mix the two cbdata styles in the owner of the object then bad things
happen.

Things you are NOT supposed to do:

* call cbdataFree on a CBDATA_CLASS typed object. You should use delete.

* reference the object pointed to by a cbdataReference() returned
pointer other than as an argument to the callback after checking for
validity.

> Do we have some callback data that is a true POD and must remain a POD
> for some low-level reasons like feeding it to system calls?

None that I know of. But the issue is not really full blown class vs
POD, only a slight confusion on when to use delete and when to use
cbdataFree().

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.

cbdata was originally designed for callback arguments where the one
making the callaback ideally knows nothing about what the reference
point to, only how to check it for validity and pass as argument to the
given callback. But to ease debugging and adaption of existing code I
selected to implement it in a way that preserved the original pointer
and datatype, and things went downhill from there. The fact that the
cbdata free callback is called when last reference is gone further adds
to this confusion.

The CBDATA_CLASS macros slightly returns in the correct direction by
being consistent about calling the destructor early (on delete) but
dangerous since the data type is still preserved and easily misused by
direct use of cbdataFree() instead of using delete causing the
destructor to never be called, plus I doubt transition have been audited
for the catch mentioned below.

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.

2. Rename cbdataFree() to cbdataOldFree().

3. Convert as many as possible to CBDATA_CLASS.

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.

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

Regards
Henrik
Received on Sat Jul 07 2012 - 20:19:22 MDT