Solid DevicePrivate object ownership

[Jiri Palecek]

On Tuesday 24 November 2009 21:24:39 Kevin Ottens wrote:
> Hello,

Hello,

sorry for replying a bit late, I somehow missed your mail (I was expecting you would respond to -hardware-devel and found this reply only by Googling). It would be probably better to Cc: me.

> On Thursday 5 November 2009 11:49:36 Jiří Paleček wrote:
> > 1) A DevicePrivate for a specific device exists at most once in the
> > program, and is always cached in the DeviceManager, is that right?
> 
> Yes, that's the intent.
> 
> > 2) By whom is the m_backendObject of the DevicePrivate owned (and should
> > be deleted)?
> 
> By the DeviceManager (as in some cases the DevicePrivate backend counterpart 
> should disappear and only the manager knows when that's supposed to happen).

This doesn't correspond to the actual code, since DeviceManager doesn't hold any (strong) reference to the backendObject. Also, I don't think this decision is wise (more on that later).

> > 3)
> > 
> > void Solid::DevicePrivate::setInterface(const DeviceInterface::Type &type,
> > DeviceInterface *interface)
> > {
> >      ref.ref();
> >      m_ifaces[type] = interface;
> > }
> > 
> > Why is the reference count raised every time we add an interface?
> 
> We want to prevent DevicePrivate to be destroyed even if there's no Device 
> instance pointing to it as soon as the client code accessed an interface (use 
> case here is that some interfaces have signals, if the DevicePrivate object 
> would be destroyed, connecting to those interface signals would be useless as 
> the emitter would disappear when no corresponding Device instance is left in 
> the client code).

OK, that makes sense, however, that would mean the DevicePrivate is never deleted in this case (unless the device is removed).

The problem I found is that 1) and 2) are not true in actual code. The problem is that when a DevicePrivate ceases to exist, the DeviceManager won't have any reference on the (say) HalDevice, which will however never be deleted, and further requests for the same device will create a new HalDevice. For a proof of the behavior, apply attached patch "Proof of the leak", and run the changed test (halbasictest) under valgrind. On my system, it shows some 1.5 MB in "still reachable" memory, which is made by the leaked objects. Note that the objects are leaked regardless they are "still reachable", because the references leading to them are some callback references, that do not allow the objects to be ever used again (compare to the state without the patch).

I propose two ways of getting rid of this:

1) Make the backendObject owned by the DevicePrivate

This solution strives to delete the backend objects as much as possible (eg. when the Device goes out of scope...). IMHO, for DevicePrivate it is natural to handle the deletion of its backend object, because existence of the backendObject beyond the lifetime of its DevicePrivate is nonsensical (except for FakeHw backend, more on that later), and there is a 1-1 correspondence between DevicePrivate and backendObject. The DeviceManger can still manipulate the backendObject if the device is remove by doing setBackendObject. The patch that implements this approach is implemented by the "Fix solid DevicePrivate ... memory leak ... by moving its ownership... " (the largest one).

The only problem with this is that the FakeHw Manager backend doesn't create an object in its createDevice function, but rather returns a pointer to a pre-existing object. This is of course incompatible with any scheme that will delete the backend object when it's no longer needed. The patch changes it by making a copy in createDevice and managing FakeDevice::Private by a shared pointer. I think this is a reasonable cost as I don't have any "fake" hardware in my computer, and it seems natural to me that something only needed for testing should adapt for changes needed for the production code to work.

2) Never delete a DevicePrivate

This solution goes a radically different way, by never deleting a DevicePrivate, even when its Device goes out of scope. The reasoning behind this is that we already have this behavior in solid (the situation in paragraph 3 in my original mail). So this just makes it consistent for all DevicePrivates in the program. This makes it always possible to reuse an old backendObject, so there is no memory leak. However, the memory taken by the backend object will never be freed, so you will possibly lose like (number of devices in the system)*(about 1 KB) memory. This approach is implemented by the other "Fix ... " patch (the smaller one, sorry, but the full names didn't make it into the filenames; they are however written in the patches).

Finally, I'm curious if the code had undergone any review or something, and why this problem, which is present for almost a year in "stable" software, hasn't been noticed before. Please, choose either way and fix this problem before 4.4.

Note that the patches are all based on 4.3.4.

Regards
   Jiri Palecek
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Proof-of-the-leak.patch
Type: text/x-patch
Size: 820 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20091219/1265746d/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-solid-DevicePrivete-m_BackendObject-memory-leak-.patch
Type: text/x-patch
Size: 7443 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20091219/1265746d/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Fix-the-solid-DevicePrivate-m_backendObject-memory-l.patch
Type: text/x-patch
Size: 4092 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20091219/1265746d/attachment-0002.bin>