address race condition around setoperation

Authored by sebas on Jun 1 2016, 2:54 PM.

Description

address race condition around setoperation

Summary:
Use a timer to avoid catching configChanged signals after we set
changes.

The long version:

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 does
avoids saving the config as a direct reaction to our own config changes.

So what happens on kded5 startup after loading the kscreen2 module:

  • the kscreen config is requested and received
  • the kscreen daemon (KD) looks into its config directory for a suitable config file (a config file is identified by a combined hash of all screen

attached, so unique per connected set of outputs)

  • KD usually finds a config
  • KD ignores configChanged events before it starts ...
  • a KScreen::SetConfigOperation to apply the "known config"
  • SetConfigOperation returns after a while (say 100ms later)
  • we re-enable the change monitor
  • we receive a configChanged signal
  • we save the new config (usually to the existing config file)

I don't think this behavior is desirable. I don't see a reason why the
daemon should save its config right after applying it. I think this
causes more problems than we want, since the startup may overwrite the
user's config. This behavior seems to be desired by the code in KD, it's
already blocking configChanged signals during the SetOperation (which,
to be honest may result in nightmarish behavior in any way, so it might
be a kludge which aims too short).

From libkscreen perspective, SetConfigOperation::finished cannot
guarantee that all configChanged signals are already fired and that it's
safe to watch for new, independent changes now. At least on X11, we
simply don't know, and what we can do is wait a bit and cross fingers
that we're not catching our own noise. The changed signal *may* come
from a re-request of the edid information, but this is a bit hard to
track down, and not too useful, anyway, since changed Edid may affect a
large number of a screen's properties.
In the Wayland backend, that's a different story and we can prevent this
behavior at an earlier stage, so this timer is "probably not needed" (I
haven't tested that).

This effectively prevents KD from catching reactions to its own changes
and does not trigger saving the config file on every login. It still
reacts to changes from libkscreen, but will avoid re-saving the config a
few times. The timer may not be the neatest of solutions for this, but
it does help narrowing down the problem and may be a last resort action.
Most importantly, it avoids the re-writing of the config on startup and
plugging/unplugging a monitor effectively.

The timer value of 100ms is also used in kwin, which should make the
behavior (which is no problem in kwin) more solid.

CCBUG:346961
CCBUG:358011

Reviewers: graesslin

Reviewed By: graesslin

Subscribers: plasma-devel, Plasma

Tags: Plasma

Differential Revision: https://phabricator.kde.org/D1730

Details

Committed
sebasJun 1 2016, 2:55 PM
Reviewer
graesslin
Differential Revision
D1730: address race condition around setoperation
Parents
R104:c949f3ddfa35: Merge branch 'Plasma/5.6'
Branches
Unknown
Tags
Unknown
Reverted By
R104:31becb3b0806: revert 17199d32f29 to avoid regression