Changeset View
Changeset View
Standalone View
Standalone View
src/kdbusservice.cpp
Show First 20 Lines • Show All 136 Lines • ▼ Show 20 Line(s) | 136 | { | |||
---|---|---|---|---|---|
137 | auto bus = QDBusConnection::sessionBus(); | 137 | auto bus = QDBusConnection::sessionBus(); | ||
138 | bool objectRegistered = false; | 138 | bool objectRegistered = false; | ||
139 | objectRegistered = bus.registerObject(QStringLiteral("/MainApplication"), | 139 | objectRegistered = bus.registerObject(QStringLiteral("/MainApplication"), | ||
140 | QCoreApplication::instance(), | 140 | QCoreApplication::instance(), | ||
141 | QDBusConnection::ExportAllSlots | | 141 | QDBusConnection::ExportAllSlots | | ||
142 | QDBusConnection::ExportScriptableProperties | | 142 | QDBusConnection::ExportScriptableProperties | | ||
143 | QDBusConnection::ExportAdaptors); | 143 | QDBusConnection::ExportAdaptors); | ||
144 | if (!objectRegistered) { | 144 | if (!objectRegistered) { | ||
145 | qWarning() << "Failed to register /MainApplication on DBus"; | 145 | qWarning() << "Failed to register /MainApplication on DBus"; | ||
kossebau: What if the quit fails? No error handling?
I fear for having such logic in the library it… | |||||
146 | return; | 146 | return; | ||
147 | } | 147 | } | ||
148 | 148 | | |||
149 | objectRegistered = bus.registerObject(objectPath, s, QDBusConnection::ExportAdaptors); | 149 | objectRegistered = bus.registerObject(objectPath, s, QDBusConnection::ExportAdaptors); | ||
150 | if (!objectRegistered) { | 150 | if (!objectRegistered) { | ||
151 | qWarning() << "Failed to register" << objectPath << "on DBus"; | 151 | qWarning() << "Failed to register" << objectPath << "on DBus"; | ||
152 | return; | 152 | return; | ||
153 | } | 153 | } | ||
Show All 9 Lines | |||||
163 | } | 163 | } | ||
164 | 164 | | |||
165 | void attemptRegistration() | 165 | void attemptRegistration() | ||
166 | { | 166 | { | ||
167 | Q_ASSERT(!d->registered); | 167 | Q_ASSERT(!d->registered); | ||
168 | 168 | | |||
169 | auto queueOption = QDBusConnectionInterface::DontQueueService; | 169 | auto queueOption = QDBusConnectionInterface::DontQueueService; | ||
170 | 170 | | |||
171 | if (options & KDBusService::Unique) { | 171 | if (options & KDBusService::Replace && QDBusConnection::sessionBus().interface()->isServiceRegistered(d->serviceName)) { | ||
davidedmundson: This is still racey | |||||
172 | auto message = QDBusMessage::createMethodCall(d->serviceName, | ||||
173 | QStringLiteral("/MainApplication"), | ||||
174 | QStringLiteral("org.qtproject.Qt.QCoreApplication"), | ||||
175 | QStringLiteral("quit")); | ||||
176 | auto reply = QDBusConnection::sessionBus().call(message); //deliberately block until it's done, so we register the name after the app quits | ||||
177 | | ||||
178 | //Give a grace time to the original instance | ||||
179 | { | ||||
180 | QEventLoop unregistrationLoop; | ||||
181 | connect(bus, &QDBusConnectionInterface::serviceUnregistered, | ||||
QDBusConnectionInterface::serviceUnregistered is very different to QDBusServiceWatcher::serviceUnregistered QDBusConnectionInterface::serviceUnregistered will be called when our process loses the service name it had. Our process hasn't even requested a service name yet, so this won't happen. davidedmundson: QDBusConnectionInterface::serviceUnregistered is very different to QDBusServiceWatcher… | |||||
182 | this, [&unregistrationLoop, this](const QString &service) { | ||||
183 | if (service != d->serviceName) { | ||||
184 | return; | ||||
185 | } | ||||
186 | | ||||
187 | unregistrationLoop.quit(); | ||||
188 | }); | ||||
189 | | ||||
190 | QTimer quitTimer; | ||||
191 | quitTimer.start(2000); | ||||
192 | connect(&quitTimer, &QTimer::timeout, &unregistrationLoop, &QEventLoop::quit); | ||||
193 | unregistrationLoop.exec(); | ||||
194 | } | ||||
195 | queueOption = QDBusConnectionInterface::ReplaceExistingService; | ||||
196 | } else if (options & KDBusService::Unique) { | ||||
172 | // When a process crashes and gets auto-restarted by KCrash we may | 197 | // When a process crashes and gets auto-restarted by KCrash we may | ||
173 | // be in this code path "too early". There is a bit of a delay | 198 | // be in this code path "too early". There is a bit of a delay | ||
174 | // between the restart and the previous process dropping off of the | 199 | // between the restart and the previous process dropping off of the | ||
175 | // bus and thus releasing its registered names. As a result there | 200 | // bus and thus releasing its registered names. As a result there | ||
176 | // is a good chance that if we wait a bit the name will shortly | 201 | // is a good chance that if we wait a bit the name will shortly | ||
177 | // become registered. | 202 | // become registered. | ||
178 | 203 | | |||
179 | queueOption = QDBusConnectionInterface::QueueService; | 204 | queueOption = QDBusConnectionInterface::QueueService; | ||
180 | 205 | | |||
181 | connect(bus, &QDBusConnectionInterface::serviceRegistered, | 206 | connect(bus, &QDBusConnectionInterface::serviceRegistered, | ||
182 | this, [this](const QString &service) { | 207 | this, [this](const QString &service) { | ||
183 | if (service != d->serviceName) { | 208 | if (service != d->serviceName) { | ||
184 | return; | 209 | return; | ||
185 | } | 210 | } | ||
186 | 211 | | |||
187 | d->registered = true; | 212 | d->registered = true; | ||
188 | registrationLoop.quit(); | 213 | registrationLoop.quit(); | ||
189 | }); | 214 | }); | ||
190 | } | 215 | } | ||
191 | 216 | | |||
192 | d->registered = | 217 | d->registered = | ||
193 | (bus->registerService(d->serviceName, queueOption) == QDBusConnectionInterface::ServiceRegistered); | 218 | (bus->registerService(d->serviceName, queueOption) == QDBusConnectionInterface::ServiceRegistered); | ||
I don't like this part very much. It means we're registering the name whilst the former app is potentially still running. I would suggest rebasing on Harald's 5bf091ee07ac44ed1bf1e75a4d07847edb86c5d6 change. It'll allow us to do this all in a race-free way. We can register the name org.kde.plasmashell in a queued manner and it'll be better than the current approach plasma does. davidedmundson: I don't like this part very much. It means we're registering the name whilst the former app is… | |||||
194 | 219 | | |||
195 | if (d->registered) { | 220 | if (d->registered) { | ||
196 | return; | 221 | return; | ||
197 | } | 222 | } | ||
198 | 223 | | |||
199 | if (options & KDBusService::Unique) { | 224 | if (options & KDBusService::Unique) { | ||
200 | // Already running so it's ok! | 225 | // Already running so it's ok! | ||
201 | QVariantMap platform_data; | 226 | QVariantMap platform_data; | ||
▲ Show 20 Lines • Show All 146 Lines • Show Last 20 Lines |
What if the quit fails? No error handling?
I fear for having such logic in the library it needs to be more elaborared IMHO.