[Telepathy] tq-qt4 refcount proposal

[Olli Salli]

On Mon, Mar 23, 2009 at 5:04 PM, Andre Moreira Magalhaes
<andre.magalhaes at collabora.co.uk> wrote:
> Simon McVittie wrote:
>> Is there a Qt/KDE convention for this sort of thing, or are we in uncharted
>> territory here?
>>
>> If there's no Qt/KDE convention, we should follow GObject's lead, since it's
>> a tried-and-tested refcounted object model that many of us have experience
>> with. In particular, we *will* need weak references in a number of places.
>>
>>
> Afaik there is no such convention in Qt/KDE world, shared pointers were
> introduced in Qt 4.x series and QExplicitlySharedDataPointer in Qt 4.4,
> that is not a long time ago.
> So I based mostly on WebKit refcounted objects which is quite similar to
> GObjects world.

Yeah, the related convention, if any, in the Qt/KDE world has been the
parent/child hierarchy. As discussed, it doesn't really fit our uses,
though, so the oceans we must venture into are relatively uncharted.

>> On Fri, 20 Mar 2009 at 17:50:23 -0300, Andre Moreira Magalhaes wrote:
>>
>>> 2.1 - Problems:
>>> 2.1.1 - Circular dependency:
>>> In the ideal world we should use QExplicitlySharedDataPointer<Object> in
>>> the library, even for objects that needs to know about each other, for
>>> example StreamedMediaChannel that caches the MediaStreams and
>>> MediaStreams that have a pointer to the StreamedMediaChannel. We should
>>> be careful with this, as we hit circular dependency, which can lead to
>>> even more memory leaks when the objects refcount never hits 0.
>>>
>>
>> Tidying up circular dependencies when a natural end-of-life is reached
>> (invalidated signal) seems a reasonable approach; many Telepathy objects
>> (Channel, Connection) have such a thing.
>>
>> Account and AccountManager might be tricky - there's no natural end-of-life
>> for the AccountManager, and Accounts' lives only end if the account is
>> deleted by the user (a rare event!). Perhaps Account could weakly ref the
>> AccountManager, and the AccountManager could strongly ref the Accounts?
>>
>> (In practice, almost every UI will want to have a ref to the AccountManager
>> for the entire lifetime of the UI, I think, so the Accounts won't be GC'd
>> until later.)
>>
>>
> Yep. that's another solution, what I did for now in my experimental
> shared branch was to make AccountManager do not cache Accounts, and
> Accounts have strong ref to the AccountManager.
>

I think this is a bad choice. At one point we used to have the
Accounts as QObject children of the AccountManager, right? In my head,
the QObject child-to-parent relationship intuitively corresponds to a
weak reference (and the parent-to-child relationship to a strong
reference). So I think making AM strong ref the Accounts and the
Accounts weak ref the AM is the best choice, and in general, every
reference "cycle" corresponding to a parent-child relationship in a
world with more traditional family values should be a strong-weak
pair.

>>> 3 - Naming:
>>> So I propose to have a template class SharedPtr that inherits
>>> QExplicitlySharedDataPointer, and a class named SharedData that is a
>>> typedef for QSharedData (in case we want to change to use another class
>>> in the future, we don't need to change all classes). We may want to make
>>> SharedData call deleteLater on ref==0 instead of delete for example.
>>>
>>> So the usage would be:
>>>
>>> public Account : SharedData
>>> {
>>> public:
>>> ...
>>> static SharedPtr<Account> create();
>>> ...
>>> };
>>>
>>
>> Do we really need to invent our own template class? Surely this problem has
>> been solved in Qt-land before‽
>>
>> Can we have some arrangement where we have a typedef or trivial subclass called
>> AccountPtr, etc.? Otherwise, we'll have signals like
>>
>>     accountValidityChanged(bool, Telepathy::SharedPtr<Telepathy::Client::Account>)
>>
>> which is horrible! Using Telepathy::Client::AccountPtr is still quite verbose,
>> but I think it's the best we're going to get.
>>
>>     S
>>
>>
> The biggest problem here is that you can't forward declare typedefs, so
> that would be quite difficult.
> You could declare typedef AccountPtr on account.h and typedef
> AccountManagerPtr on account-manager.h and both headers have to include
> the other, so this wouldn't work.
>
> Solutions:
> 1 - Have a header that defines all XXXPtr typedefs.

I would go for this one. Despite having declarations for a lot of
"unrelated" types coming from all around the library it will still be
a quite lightweight header (most of the bulk comes from the
QExplicitlySharedDataPointer headers, which are bound to be included
mostly everywhere anyway).

> 2 - Instead of using typedefs making the XXXPtr a class inherited from
> QExplicitlySharedDataPointer<XXX> and forward declare it.

Right, like the UIntList & co action we already have going on? That
was absolutely necessary, and could be auto-generated, but in this
case, if we want all the features of QESDP including the
cast-to-parent-class copy constructors etc we'll have to write quite a
bit of boilerplate littering the headers for each pointer class. So,
don't like.

> 3 - Do not use typedefs at all, always use
> QExplicitlySharedDataPointer<XXX> or SharedPtr<XXX> as I did.

No :)

> 4 - ???
>
> BR
> Andrunko
> _______________________________________________
> telepathy mailing list
> telepathy at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/telepathy
>

I would also adapt Ian's suggestion of creating a custom
Telepathy::SharedPtr class with private inheritance from QESDP,
allowing us to obscure a bit the fact that yes, it really is a
QExplicitlySharedDataPointer. The intended use for QESDP is wildly
different (like its name implies, aiding implementation of
explicitly-shared classes) from what we are planning to employ it for,
which might be confusing to a library user. This also means dropping
stuff like detach(), and that we should most likely have a specialized
weak pointer (sub)class as well.

Generally I'm not very fond of how QObject and reference counting
schemes interact. The fact that you need the bare pointer to the
QObject for many things (the whole of the QObject API, signal
connections most importantly) means that users are prone to stash a
bare pointer away and maybe even accidentally delete the object
through it (most probably by assigning it as a QObject child of
another object. This differs from the intrusively ref-counted GObject
world in that there is no natural "delete obj" equivalent, as even
gobject destructor functions aren't public by convention. I also don't
like the "oh, your object just went away so when you try to use me,
the pointer you think refers to it, you hit an assert/segfault"
behavior of QPointer & co AT ALL - as there is no notification
(signal) for this event on the actual pointer object (and can't be)
it's even worse than the DBusGProxy landmine behavior we've been
bashing for ages. We could possibly do a slight sense injection here
though:

In the destructor of our SharedObject (or whatever the SharedData
subclass will be) class, we could emit a warning, if there still are
non-zero refs. This allows deleting a floating object in addition to
the usual refs-came-and-went sequence to happen happily without a
warning, but at least gives some indication to somebody debugging his
app crashing on something using shared objects that hey, you should
look for an accidental child assignment or plain old deletion.

-- 

Br,
Olli Salli