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.
