system: Use two D-Bus RAUC proxy objects to avoid deadlock
This patch adds a second D-Bus proxy object for RAUC on a different
connection. This should avoid a deadlock happening in the sdbus-c++'s
code. Consider the following situation with two threads: T1 (executing
sdbus-c++ event loop) and T2 (executing Sysrepo callback) and a RAUC
installation invoked.
T1: Gets notified upon signal PropertyChanged on the property Progress.
sdbus-c++ locks sdbus mutex for the current connection (so only one
object can talk to sdbus at one point).
T1: Invokes callback that creates Sysrepo notification.
T1: Sysrepo starts validating the operational datastore and invokes
pull operational data callback.
T2: Starts processing the operational data callback.
T2: Invokes a code that calls GetSlotStatus on RAUC via D-Bus
(something that is done in the next commit) and gets stuck in
sdbus-c++ code because it is trying to acquire a mutex currently
held by T1.
In order not to get stuck in this situation we create two D-Bus proxy
objects. Both on different connections.
The first proxy is supposed to handle all the D-Bus signals. The second
proxy handles the method calls. Because both proxies are on different
connections, they do not share the same sdbus mutex in sdbus-c++ and
therefore the scenario described above won't result in a deadlock.
This could also be solved by not calling the GetSlotStatus method in the
pull ops data callback. However, we want those data to be provided by
Sysrepo and we want them to be "fresh". This would probably require some
kind of timer that would refresh the data every N time units and push
them into Sysrepo. But I don't want to mess velia project with timers now.
Change-Id: I209f57da247bc1745261c258f4e2db73f72a4f1a
diff --git a/src/main-system.cpp b/src/main-system.cpp
index 0e9f725..e7a7af6 100644
--- a/src/main-system.cpp
+++ b/src/main-system.cpp
@@ -77,7 +77,10 @@
// initialize ietf-system
spdlog::get("main")->debug("Initializing Sysrepo for system models");
auto sysrepoIETFSystem = velia::system::IETFSystem(srSess, "/etc/os-release");
- auto sysrepoFirmware = velia::system::Firmware(srConn, *g_dbusConnection);
+
+ auto dbusConnection = sdbus::createConnection(); // second connection for RAUC (for calling methods).
+ dbusConnection->enterEventLoopAsync();
+ auto sysrepoFirmware = velia::system::Firmware(srConn, *g_dbusConnection, *dbusConnection);
DBUS_EVENTLOOP_END
return 0;
diff --git a/src/system/Firmware.cpp b/src/system/Firmware.cpp
index 4838cbd..68fd860 100644
--- a/src/system/Firmware.cpp
+++ b/src/system/Firmware.cpp
@@ -20,14 +20,15 @@
namespace velia::system {
-Firmware::Firmware(std::shared_ptr<::sysrepo::Connection> srConn, sdbus::IConnection& dbusConnection)
+Firmware::Firmware(std::shared_ptr<::sysrepo::Connection> srConn, sdbus::IConnection& dbusConnectionSignals, sdbus::IConnection& dbusConnectionMethods)
: m_srConn(std::move(srConn))
, m_srSessionOps(std::make_shared<::sysrepo::Session>(m_srConn))
, m_srSessionRPC(std::make_shared<::sysrepo::Session>(m_srConn))
, m_srSubscribeOps(std::make_shared<::sysrepo::Subscribe>(m_srSessionOps))
, m_srSubscribeRPC(std::make_shared<::sysrepo::Subscribe>(m_srSessionRPC))
, m_rauc(std::make_shared<RAUC>(
- dbusConnection,
+ dbusConnectionSignals,
+ dbusConnectionMethods,
[this](const std::string& operation) {
if (operation == "installing") {
std::lock_guard<std::mutex> lck(m_mtx);
diff --git a/src/system/Firmware.h b/src/system/Firmware.h
index 6ab030e..5ff6723 100644
--- a/src/system/Firmware.h
+++ b/src/system/Firmware.h
@@ -17,7 +17,7 @@
class Firmware {
public:
- Firmware(std::shared_ptr<::sysrepo::Connection> srConn, sdbus::IConnection& dbusConnection);
+ Firmware(std::shared_ptr<::sysrepo::Connection> srConn, sdbus::IConnection& dbusConnectionSignals, sdbus::IConnection& dbusConnectionMethods);
private:
std::shared_ptr<::sysrepo::Connection> m_srConn;
diff --git a/src/system/RAUC.cpp b/src/system/RAUC.cpp
index 4338446..796f98b 100644
--- a/src/system/RAUC.cpp
+++ b/src/system/RAUC.cpp
@@ -34,20 +34,29 @@
namespace velia::system {
-RAUC::RAUC(sdbus::IConnection& connection, std::function<void(const std::string&)> operCb, std::function<void(int32_t, const std::string&)> progressCb, std::function<void(int32_t, const std::string&)> completedCb)
- : m_dbusObjectProxy(sdbus::createProxy(connection, BUS, OBJPATH))
+/** @brief Constructs a class communicating with RAUC via D-Bus.
+ *
+ * @param signalConnection A D-Bus connection. Used for handling signals on the object.
+ * @param methodConnection A D-Bus connection. Used for calling D-Bus methods on the object.
+ * @param operCb A function to execute whene RAUC's operation status changes.
+ * @param progressCb A function to execute when RAUC's installation makes progress.
+ * @param completedCb A function to execute when RAUC's installation completes.
+ */
+RAUC::RAUC(sdbus::IConnection& signalConnection, sdbus::IConnection& methodConnection, std::function<void(const std::string&)> operCb, std::function<void(int32_t, const std::string&)> progressCb, std::function<void(int32_t, const std::string&)> completedCb)
+ : m_dbusObjectProxySignals(sdbus::createProxy(signalConnection, BUS, OBJPATH))
+ , m_dbusObjectProxyMethods(sdbus::createProxy(methodConnection, BUS, OBJPATH))
, m_operCb(std::move(operCb))
, m_progressCb(std::move(progressCb))
, m_completedCb(std::move(completedCb))
, m_log(spdlog::get("system"))
{
- m_dbusObjectProxy->uponSignal("Completed").onInterface(INTERFACE).call([this](int32_t returnValue) {
- std::string lastError = m_dbusObjectProxy->getProperty("LastError").onInterface(INTERFACE);
+ m_dbusObjectProxySignals->uponSignal("Completed").onInterface(INTERFACE).call([this](int32_t returnValue) {
+ std::string lastError = m_dbusObjectProxySignals->getProperty("LastError").onInterface(INTERFACE);
m_log->info("InstallBundle completed. Return value {}, last error: '{}'", returnValue, lastError);
m_completedCb(returnValue, lastError);
});
- m_dbusObjectProxy->uponSignal("PropertiesChanged").onInterface("org.freedesktop.DBus.Properties").call([this](const std::string& iface, const std::map<std::string, sdbus::Variant>& changed, [[maybe_unused]] const std::vector<std::string>& invalidated) {
+ m_dbusObjectProxySignals->uponSignal("PropertiesChanged").onInterface("org.freedesktop.DBus.Properties").call([this](const std::string& iface, const std::map<std::string, sdbus::Variant>& changed, [[maybe_unused]] const std::vector<std::string>& invalidated) {
if (iface != INTERFACE) {
return;
}
@@ -69,7 +78,7 @@
}
});
- m_dbusObjectProxy->finishRegistration();
+ m_dbusObjectProxySignals->finishRegistration();
}
/** @brief Get current primary slot.
@@ -80,7 +89,7 @@
std::string RAUC::primarySlot() const
{
std::string primarySlot;
- m_dbusObjectProxy->callMethod("GetPrimary").onInterface(INTERFACE).storeResultsTo(primarySlot);
+ m_dbusObjectProxyMethods->callMethod("GetPrimary").onInterface(INTERFACE).storeResultsTo(primarySlot);
return primarySlot;
}
@@ -93,7 +102,7 @@
std::map<std::string, RAUC::SlotProperties> RAUC::slotStatus() const
{
std::vector<sdbus::Struct<std::string, std::map<std::string, sdbus::Variant>>> slots;
- m_dbusObjectProxy->callMethod("GetSlotStatus").onInterface(INTERFACE).storeResultsTo(slots);
+ m_dbusObjectProxyMethods->callMethod("GetSlotStatus").onInterface(INTERFACE).storeResultsTo(slots);
std::map<std::string, SlotProperties> res;
for (const auto& slot : slots) {
@@ -118,16 +127,16 @@
*/
void RAUC::install(const std::string& source)
{
- m_dbusObjectProxy->callMethod("InstallBundle").onInterface(INTERFACE).withArguments(source, std::map<std::string, sdbus::Variant> {});
+ m_dbusObjectProxyMethods->callMethod("InstallBundle").onInterface(INTERFACE).withArguments(source, std::map<std::string, sdbus::Variant> {});
}
std::string RAUC::operation() const
{
- return m_dbusObjectProxy->getProperty("Operation").onInterface(INTERFACE);
+ return m_dbusObjectProxyMethods->getProperty("Operation").onInterface(INTERFACE);
}
std::string RAUC::lastError() const
{
- return m_dbusObjectProxy->getProperty("LastError").onInterface(INTERFACE);
+ return m_dbusObjectProxyMethods->getProperty("LastError").onInterface(INTERFACE);
}
}
diff --git a/src/system/RAUC.h b/src/system/RAUC.h
index 45eba7c..31fbfbd 100644
--- a/src/system/RAUC.h
+++ b/src/system/RAUC.h
@@ -19,7 +19,7 @@
public:
using SlotProperties = std::map<std::string, std::variant<std::string, uint64_t, uint32_t>>;
- RAUC(sdbus::IConnection& connection, std::function<void(const std::string&)> operCb, std::function<void(int32_t, const std::string&)> progressCb, std::function<void(int32_t, const std::string&)> completedCb);
+ RAUC(sdbus::IConnection& signalConnection, sdbus::IConnection& methodConnection, std::function<void(const std::string&)> operCb, std::function<void(int32_t, const std::string&)> progressCb, std::function<void(int32_t, const std::string&)> completedCb);
std::string primarySlot() const;
std::map<std::string, SlotProperties> slotStatus() const;
void install(const std::string& source);
@@ -27,7 +27,14 @@
std::string lastError() const;
private:
- std::shared_ptr<sdbus::IProxy> m_dbusObjectProxy;
+ /* We have two objects on two connections here intentionally. On a D-Bus signal PropertyChanged we invoke a callback that does something
+ * with Sysrepo's operational datastore in velia::system::Firmware class. While executing this code, sdbus-c++'s mutex (sdbus-c++ src/SdBus.h, sdbusMutex_) is locked.
+ * However, this operational datastore change invokes a code, that (again) calls a D-Bus method via sdbus-c++ on the same sdbus::IProxy object.
+ * Calling method on this object would require to acquire the same mutex that is already held, which results in deadlock.
+ * Therefore, we maintain two separate connections to the same D-Bus object. The first one is used for handling signal callbacks and
+ * the second is only for calling the D-Bus methods.
+ */
+ std::shared_ptr<sdbus::IProxy> m_dbusObjectProxySignals, m_dbusObjectProxyMethods;
std::function<void(const std::string&)> m_operCb;
std::function<void(int32_t, const std::string&)> m_progressCb;
std::function<void(int32_t, const std::string&)> m_completedCb;