1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
|
From: Sebastian Kügler <sebas@kde.org>
Date: Wed, 01 Jun 2016 14:54:16 +0000
Subject: address race condition around setoperation
X-Git-Tag: v5.6.95
X-Git-Url: http://quickgit.kde.org/?p=kscreen.git&a=commitdiff&h=17199d32f292f7c44eb8cdce5b35396d3bd19eb8
---
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
---
--- a/kded/daemon.cpp
+++ b/kded/daemon.cpp
@@ -23,6 +23,7 @@
#include "kscreenadaptor.h"
#include "debug.h"
+#include <QElapsedTimer>
#include <QTimer>
#include <QAction>
#include <QShortcut>
@@ -52,6 +53,7 @@
, m_buttonTimer(new QTimer())
, m_saveTimer(new QTimer())
, m_lidClosedTimer(new QTimer())
+ , m_changeBlockTimer(new QElapsedTimer())
{
QMetaObject::invokeMethod(this, "requestConfig", Qt::QueuedConnection);
@@ -82,6 +84,7 @@
delete m_saveTimer;
delete m_buttonTimer;
delete m_lidClosedTimer;
+ delete m_changeBlockTimer;
Generator::destroy();
Device::destroy();
@@ -112,7 +115,6 @@
m_lidClosedTimer->setInterval(1000);
m_lidClosedTimer->setSingleShot(true);
connect(m_lidClosedTimer, &QTimer::timeout, this, &KScreenDaemon::lidClosedTimeout);
-
connect(Device::self(), &Device::lidClosedChanged, this, &KScreenDaemon::lidClosedChanged);
connect(Device::self(), &Device::resumingFromSuspend,
@@ -145,6 +147,9 @@
connect(new KScreen::SetConfigOperation(config), &KScreen::SetConfigOperation::finished,
[&]() {
qCDebug(KSCREEN_KDED) << "Config applied";
+ // We enable monitoring already, but we will ignore the first signals that come
+ // in the next 100ms, since these are likely our own changes still flushing out
+ m_changeBlockTimer->start();
setMonitorForChanges(true);
});
}
@@ -182,6 +187,12 @@
void KScreenDaemon::configChanged()
{
+ if (m_changeBlockTimer->isValid() && !m_changeBlockTimer->hasExpired(100)) {
+ m_changeBlockTimer->start();
+ qCDebug(KSCREEN_KDED) << "Change detected, but ignoring since it's our own noise";
+ return;
+ }
+ m_changeBlockTimer->invalidate();
qCDebug(KSCREEN_KDED) << "Change detected";
// Reset timer, delay the writeback
m_saveTimer->start();
--- a/kded/daemon.h
+++ b/kded/daemon.h
@@ -27,6 +27,7 @@
#include "generator.h"
+class QElapsedTimer;
class QTimer;
namespace KScreen
@@ -79,6 +80,7 @@
QTimer* m_buttonTimer;
QTimer* m_saveTimer;
QTimer* m_lidClosedTimer;
+ QElapsedTimer* m_changeBlockTimer;
};
#endif /*KSCREN_DAEMON_H*/
|
GitWeb