[Differential] [Request, 13 lines] D1730: address race condition around setoperation

[sebas (Sebastian Kügler)]

sebas created this revision.
sebas added a reviewer: graesslin.
sebas added a subscriber: Plasma.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.

REVISION 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

REPOSITORY
  rKSCREEN KScreen

BRANCH
  sebas/setop-race

REVISION DETAIL
  https://phabricator.kde.org/D1730

AFFECTED FILES
  kded/daemon.cpp
  kded/daemon.h

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: sebas, graesslin
Cc: plasma-devel, Plasma, sebas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20160601/ec6848cb/attachment-0001.html>