[Telepathy] tq-qt4 refcount proposal

[Andre Moreira Magalhaes]

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