Skip to content
  • Sebastian Kügler's avatar
    address race condition around setoperation · 17199d32
    Sebastian Kügler authored
    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
    17199d32