[Kde-hardware-devel] PowerManager Library

[Lamarque V. Souza]

Em Wednesday 20 June 2012, Daniel Nicoletti escreveu:
> 2012/6/20 Lamarque V. Souza <lamarque at kde.org>:
> > Avoid using waitForFinished or QDBusReply, they can cause freezes in the
> > desktop. Always try the asynchronous approach first. Unfortunately in
> > this case we will need them to retrieve the initial status of some
> > variables. However, restrict the usage of waitForFinished and QDBusReply
> > to early initialization. Do not use them in slots that can be called
> > several times during the lifetime of the class.
> 
> Sure, I know that but this is no different from the current behavior
> which is also blocking, also sync calls to DBus properties are much
> less expensive than sync calls to methods.

	They may be less expensive but I still prefer to cache the value and use 
signals/slots to update it, that is more guaranteed not to cause freeze 
problems.

> Also we need to coldplug things, so we need the blocking behavior,
> on the slots we don't need it to block because the value came from
> a signal, which already is async.

	The current behaviour is different because in some places Solid is using 
asynchronous calls as if it they were synchronous, which is wrong and we are 
lucky they are not causing nastty bugs so far. I still wonder why most of the 
time QDBusPendingReply::value() returns the correct value in the next line of 
the source code like QDBusReply::value() does. The calls below are all 
asynchornous (they return QDBusPendingReply) and we are using their values 
without checking for isValid(). We are lucky almost all the time they are 
valid:

Solid::PowerManagementPrivate::PowerManagementPrivate()
{
    powerSaveStatus = managerIface.GetPowerSaveStatus();

    if (managerIface.CanSuspend())
        supportedSleepStates+= Solid::PowerManagement::SuspendState;
    if (managerIface.CanHibernate())
        supportedSleepStates+= Solid::PowerManagement::HibernateState;
...

 
> > Well, I created the attached patch to fix the problem Lukas described
> > yesterday on #solid. Can you test it for me? If you wish you can add it
> > to git.reviewboard.kde.org too.
> 
> Well you are doing lot's of uneeded stuff like we don't need new on the
> interfaces, we need service watcher to coldplug everything when the
> interface is un/registered.

	Where some see "unneeded stuff" some see "being carefull".
 
> this for example is also wrong:
> -    QDBusPendingReply< uint > reply =
> QDBusConnection::sessionBus().asyncCall(call);
> -    reply.waitForFinished();
> +    QDBusReply< uint > reply =
> QDBusConnection::sessionBus().asyncCall(call);
> 
> Async call returns a QDBusPendingReply not a QDBusReply,
> if you remove the waitForFinished you can't trust on isValid().

	QDBusReply is the same thing as QDBusPendingReply + waitForFinished, 
just shorter :-):

	http://qt-project.org/doc/qt-4.8/qdbusreply.html#QDBusReply-2
 
> What this class need is sync calls on cold plug, otherwise you can't trust
> it the fist time you call it.

	That is exactly what my patch does. managerIface, policyAgentIface and 
inhibitIface are no longer valid when their assolicated DBus service disappear 
from the bus. When services come back those objects are still invalid and need 
to be re-allocated, so basically my patch does a cold plug initialization 
everytime the services appear on the bus.

> Also for the desktop freezes this is quite different, what we really need
> are modules using threads on KDED.

	Sorry, by threads are not the answer for everything and they are not 
easy to debug. Anyway, the desktop freezes I am seeing in some places are 
indeed caused by synchronous calls, using the asynchronous API is enough to 
prevent them.

-- 
Lamarque V. Souza
KDE's Network Management maintainer
http://planetkde.org/pt-br
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-hardware-devel/attachments/20120620/3fa5983f/attachment.html>