Re: [RFC] cbdata NG

On 07/08/2012 06:40 AM, Henrik Nordström wrote:
>>> 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?

I am not sure where the border between those two is exactly, but
AsyncCalls are more-or-less callbacks (with some call forwarding code in
the middle) whereas pinned connections are more about sharing "volatile"
data.

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

Yes, this is an important point! This (together with your weak_ptr
suggestion below) reminded me that it is a deficiency, IMO, of the
current cbdata design (especially if we continue to use it for async
calls and sharing): One has to destroy the object to make it invalid.

It would be nice if we could decouple invalidation from destruction.
This would allow us to protect fragile cleanup/destruction code from
reentrant callbacks and accesses. I know this came up in eCAP
development, and I am pretty sure it will be useful in Squid as we
continue to upgrade old code to async jobs.

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

Excellent suggestion. Squid already uses them (indirectly via libecap).
In eCAP, we had to wrap them to separate invalidation from destruction.
We can do the same in Squid.

However, std::weak_ptr requires following quite strict rules about the
underlying shared pointer. I doubt we can easily convert all Squid code
to obey those rules. For example, IIRC, one cannot create weak pointers
in class constructors (Boost has tricks to work around that but I do not
know whether they are now in std).

Are you OK with removing all the cbdata guts and replacing them with std
pointers? Not saying we should do that at this point, just asking
whether you think this is a viable option for cbdata.

This may be worth investigating further.

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

I think it all depends on whether the API has to be extended to support
more uses cases. In this context, I suspect the same minimal API can
support all three uses cases discussed so far (callbacks, async calls,
and sharing of volatile info). Removing any of these use cases will not
make API smaller (in fact, prohibiting abuse by other cases would only
make the API larger).

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

Agreed.

Thank you,

Alex.
Received on Sun Jul 08 2012 - 15:45:52 MDT