[Telepathy] tq-qt4 refcount proposal

[Kenneth Rohde Christiansen]

class ChannelRequest
    PendingOperation *proceed();
    PendingOperation *cancel();

Dont you accept/reject a request? How can I cancel something that
isn't started yet?


On Wed, Apr 1, 2009 at 1:00 AM, Andre Moreira Magalhaes
<andre.magalhaes at collabora.co.uk> wrote:
> Andre Moreira Magalhaes wrote:
>> Hi all,
>>
>> I would like to make a proposal for tp-qt4 refcounted classes.
>>
>> 1 - Rationale:
>> Many objects (read all high-level DBus interfaces classes AccoutManager,
>> Account, Channel, Connection, ...) in tp-qt4 are reference counted.
>> The pattern used is that classes inherit QSharedData and are used
>> together with QExplicitlySharedDataPointer, that takes care of managing
>> the reference count.
>> When the reference count hits 0, the object is deleted.
>> When instantiating new classes that inherits QSharedData without using
>> QExplicitlySharedDataPointer the reference count is 0, this is referred
>> to as the floating state.
>> This may lead to memory leaks, caused by objects in the floating state
>> that never get deleted.
>>
>> 2 - Proposal for avoiding objects in floating state as hard as possible:
>> New objects should be put into a QExplicitlySharedDataPointer as soon as
>> possible after creation to avoid possibly leaking an object while it’s
>> in a floating state.
>> To achieve this we could make the objects constructor protected* and
>> have public create function that returns a QExplicitlySharedDataPointer.
>>
>> Why not make the constructor private?
>> We need to support subclasses, and so we need the constructors to be at
>> least protected.
>>
>> 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.
>> This can be handled in different ways, and should be done case by case.
>> Bellow there is a code snippet to illustrate how to manage this on
>> StreamedMediaChannel/MediaStream case:
>>
>> class MediaStream : public QSharedData, ... {
>> public:
>> MediaStream(const QExplicitlySharedDataPointer<StreamedMediaChannel>
>> &channel, ...)
>> : QSharedData(),
>> mChannel(channel) // channel->ref++
>> { }
>>
>> ...
>>
>> QExplicitlySharedDataPointer<StreamedMediaChannel> channel() const {
>> return mChannel; }
>>
>> ...
>>
>> private:
>> ...
>>
>> QExplicitlySharedDataPointer<StreamedMediaChannel> mChannel;
>> };
>>
>> class StreamedMediaChannel : public QSharedData, ... {
>> public:
>> static QExplicitlySharedDataPointer<StreamedMediaChannel> create(...) {
>> ... }
>>
>> ...
>>
>> QList<QExplicitlySharedDataPointer<StreamedMediaChannel> > streams()
>> const { return mStreams; }
>>
>> ...
>>
>> protected:
>> StreamedMediaChannel(...); // StreamedMediaChannel should allow inheritance
>>
>> ...
>>
>> private Q_SLOTS:
>> void onInvalidated(...) // closed
>> {
>> foreach (stream, streams) {
>> // if nothing else is holding the stream, the stream will be deleted
>> stream.reset();
>> }
>> // no stream is referencing the channel anymore, when the library user
>> forget about the channel,
>> // it will be deleted.
>> }
>>
>> void onStreamRemoved(...)
>> {
>> ...
>> // if nothing else is holding the stream, the stream will be deleted
>> stream.reset();
>> ...
>> }
>>
>> private:
>> ...
>>
>> QList<QExplicitlySharedDataPointer<MediaStream> > mStreams;
>> };
>>
>>
>> 2.1.2 - Custom classes:
>> All custom classes should follow this guideline, making the constructor
>> protected and having a create method, to avoid objects in floating
>> state, as explained above.
>>
>> 2.1.3 - QObject parent relationship:
>> All refcounted objects should not have a parent QObject, as when the
>> QObject gets deleted the children objects are deleted as well.
>>
>> 3 - Naming:
>> As you can see typing QExplicitlySharedDataPointer all over the place is
>> not something really nice, and we may want to move to another class
>> later, a lightweight version or something else, so the ideal is to have
>> our own class that for now inherits QExplicitlySharedDataPointer, and if
>> we want to change it later we just change this class.
>> Note that typedef is not an option here, as you can't typedef template
>> (just template specializations) and can't forward declare typedef, which
>> we would need.
>>
>> 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();
>> ...
>> };
>>
>> I wrote a experimental branch to ilustrate what it's explained above
>> http://git.collabora.co.uk/?p=user/andrunko/telepathy-qt4.git;a=shortlog;h=refs/heads/shared
>>
>> Opinions, suggestions?
>>
>> BR
>> Andrunko
>>
>> P.s.: Some ideas are based on http://webkit.org/coding/RefPtr.html, as
>> suggested by Kenneth Rohde Christiansen.
>> _______________________________________________
>> telepathy mailing list
>> telepathy at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/telepathy
>>
>>
> First version of the proposal with all suggestions is up on my shared
> branch:
> http://git.collabora.co.uk/?p=user/andrunko/telepathy-qt4.git;a=shortlog;h=refs/heads/shared
>
> BR
> Andre
> _______________________________________________
> telepathy mailing list
> telepathy at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/telepathy
>



-- 
Kenneth Rohde Christiansen
http://kenneth.christiansen.googlepages.com