Re: [RFC] cbdata NG

lör 2012-07-07 klockan 18:54 -0600 skrev Alex Rousskov:

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

So those needs fixing.

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

"a lot" is a matter of perspective.

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

Hmm.. do these actually use cbdata as cbdata, or only as references that
can be invalidated?

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

Not sure how to do that.

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

Worth noting is that these is broken anyway after CBDATA_CLASS
migration.

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

Agreed.

> What we are missing (and finding ways to abuse cbdata to provide) is an
> API to safely _share_ information that may expire.

std::weak_ptr?

If there is a standard interface that fits the problem then we better
use that instead of making up our own.

> I think the two uses are close enough to support with one API.

No sure it's desireable to cram all usecases in one API. Better to use
constructs for the specific use cases. C++ is expressive enough to allow
that without introducing hazards.

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

cbdata as implemented supports both, but lacks the automatic validify
check guard.

From what I can tell std::weak_ptr does pretty much the same thing as
cbdata in the majority of uses not using the optional free callback,
except that std::weak_ptr do not support hiding the data type I think.

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

cbdata do not wrap it in void*, but it's intended use case is where the
one having the cbdata reference do not need to know the actual data type
so void* is used in C code.

I am sure there is other ways to model this in C++ code using template
magics.

Regards
Henrik
Received on Sun Jul 08 2012 - 12:40:34 MDT