[Kde-hardware-devel] Solid DevicePrivate object ownership

[Jiří Paleček]

On Mon, 22 Feb 2010 15:31:23 +0100, Kevin Ottens <ervin at kde.org> wrote:

> On Saturday 19 December 2009 01:39:21 Jiri Palecek wrote:
>> On Tuesday 24 November 2009 21:24:39 Kevin Ottens wrote:
>> > 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).
>
> Yes, that's the intent. Once you did something advanced with a device  
> it's
> supposed to stay here until it's unplugged.
>
>> 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).
>
> Yup, mainly comes from the fact that I have been switching the way  
> ownership
> was handled and never got to actually finish the job... Volunteer spare  
> time
> can sometime drop quickly which has been the case for me regarding  
> libsolid
> (which is mainly a one man show for now).

I understand this - yet I'd prefer someone beyond you (the community?)  
would take care of libsolid (doing things like code review etc.), as it's  
code deployed on many computers with kdelibs.

>> 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).
>
> Definitely the preferred way as that complete what was left unfinished.

Thanks for committing it. However, won't there be any commit in the 4.4  
branch?

>> 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.
>
> Sure thing.
>
>> Note that the patches are all based on 4.3.4.
>
> Adapted the bits I was interested in and now committed.
>
> Regards.

Regards
    Jiri Palecek

-- 
Using Opera's revolutionary e-mail client: http://www.opera.com/mail/