Re: kscreen daemon race

[Sebastian Kügler]

On Tuesday 31 May 2016 14:29:06 Sebastian Kügler wrote:
> Discussed with mgraesslin on IRC, preliminary conclusion below...
> 
> On dinsdag 31 mei 2016 14:05:06 CEST Sebastian Kügler wrote:
> > https://bugs.kde.org/show_bug.cgi?id=358011 dual screen not setup after
> > reboot
> > 
> > This bugreport seems to be a common problem, and it's a good example for
> > what's wrong with the screen configuration on startup.
> > 
> > TL;DR: We have a race condition when the kscreen daemon starts. It looks
> > up
> > a  known config, then applies it and subsequently resaves the config. The
> > same happens on config changes, it writes, then re-reads and then
> > re-writes
> > the config change.
> > I've managed to prevent this from happening by adding a timer that avoids
> > saving the config as a direct reaction to our own config changes.
> 
> So what we want to try is the same mechanism that KWin uses. Kwin watches
> for configuration changes for 100ms, if any change takes longer than 100ms,
> it's considered unrelated. Basically, what we want to do in kscreen daemon
> is:
> 
> - start a timer in kscreen daemon for 100ms after
> SetConfigOperation::finished and
> - restart it for every configChanged that arrives while the timer is still
>   running,
> - if the timer has timed out in the meantime, react to configChanged as
> usual
> 
> That should achieve the same effect as the "current" timer approach (which
> was just a proof of concept to prove my point, anyway.
> 
> Let's see how this goes.

Patch: https://phabricator.kde.org/D1730
-- 
sebas

Sebastian Kügler•http://vizZzion.org•http://www.kde.org
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: kscreen daemon race

[Sebastian Kügler]

On Wednesday 01 June 2016 00:05:10 Mark Gaiser wrote:
> Perhaps blockSignals can help you out [1]?
> 
> object->blockSignals(true);
> // ...
> Write your changes
> // ...
> object->blockSignals(false);
> 
> Object should be the object that currently receives the notification that
> the config file has been changed i think.
> 
> [1] http://doc.qt.io/qt-5/qobject.html#blockSignals 

That is essentially already what's happening, the problem is that signals come 
in after unblocking them.
-- 
sebas

Sebastian Kügler•http://vizZzion.org•http://www.kde.org
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: kscreen daemon race

[Mark Gaiser]

On Tue, May 31, 2016 at 2:29 PM, Sebastian Kügler  wrote:

> Discussed with mgraesslin on IRC, preliminary conclusion below...
>
> On dinsdag 31 mei 2016 14:05:06 CEST Sebastian Kügler wrote:
> > https://bugs.kde.org/show_bug.cgi?id=358011 dual screen not setup after
> > reboot
> >
> > This bugreport seems to be a common problem, and it's a good example for
> > what's wrong with the screen configuration on startup.
> >
> > TL;DR: We have a race condition when the kscreen daemon starts. It looks
> up
> > a  known config, then applies it and subsequently resaves the config. The
> > same happens on config changes, it writes, then re-reads and then
> re-writes
> > the config change.
> > I've managed to prevent this from happening by adding a timer that avoids
> > saving the config as a direct reaction to our own config changes.
>
> So what we want to try is the same mechanism that KWin uses. Kwin watches
> for
> configuration changes for 100ms, if any change takes longer than 100ms,
> it's
> considered unrelated. Basically, what we want to do in kscreen daemon is:
>
> - start a timer in kscreen daemon for 100ms after
> SetConfigOperation::finished
>   and
> - restart it for every configChanged that arrives while the timer is still
>   running,
> - if the timer has timed out in the meantime, react to configChanged as
> usual
>
> That should achieve the same effect as the "current" timer approach (which
> was
> just a proof of concept to prove my point, anyway.
>
> Let's see how this goes.
>
>
Perhaps blockSignals can help you out [1]?

object->blockSignals(true);
// ...
Write your changes
// ...
object->blockSignals(false);

Object should be the object that currently receives the notification that
the config file has been changed i think.

[1] http://doc.qt.io/qt-5/qobject.html#blockSignals
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: kscreen daemon race

[Sebastian Kügler]

Discussed with mgraesslin on IRC, preliminary conclusion below...

On dinsdag 31 mei 2016 14:05:06 CEST Sebastian Kügler wrote:
> https://bugs.kde.org/show_bug.cgi?id=358011 dual screen not setup after
> reboot
> 
> This bugreport seems to be a common problem, and it's a good example for 
> what's wrong with the screen configuration on startup.
> 
> TL;DR: We have a race condition when the kscreen daemon starts. It looks up
> a  known config, then applies it and subsequently resaves the config. The
> same happens on config changes, it writes, then re-reads and then re-writes
> the config change.
> I've managed to prevent this from happening by adding a timer that avoids 
> saving the config as a direct reaction to our own config changes.

So what we want to try is the same mechanism that KWin uses. Kwin watches for 
configuration changes for 100ms, if any change takes longer than 100ms, it's 
considered unrelated. Basically, what we want to do in kscreen daemon is:

- start a timer in kscreen daemon for 100ms after SetConfigOperation::finished 
  and 
- restart it for every configChanged that arrives while the timer is still 
  running, 
- if the timer has timed out in the meantime, react to configChanged as usual

That should achieve the same effect as the "current" timer approach (which was 
just a proof of concept to prove my point, anyway.

Let's see how this goes.
-- 
sebas

http://www.kde.org | http://vizZzion.org
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel