daemon-client: Separate context and callbacks

To address race conditions between the lifetime of the context and callbacks, it is safer to manage them separately. With sdbus-cpp version 2, I observed various failures in dnf5daemon-client, typically following this pattern: - The main thread has already completed its work and is in the process of destructing the Context class. - Meanwhile, the D-Bus event loop thread is still handling messages, and the handler attempts to access the context instance that is currently being destroyed.
rpm-software-management · Jan 7, 2025 · 80c6e46 · 80c6e46
1 parent 7ba21cc
commit 80c6e46
Show file tree

Hide file tree

Showing 5 changed files with 79 additions and 41 deletions.
diff --git a/dnf5daemon-client/callbacks.cpp b/dnf5daemon-client/callbacks.cpp
@@ -33,38 +33,50 @@ along with libdnf.  If not, see <https://www.gnu.org/licenses/>.
 
 namespace dnfdaemon::client {
 
+DbusCallback::DbusCallback(Context & context, sdbus::IConnection & connection)
+    : session_object_path(context.get_session_object_path()) {
+    session_proxy = sdbus::createProxy(connection, dnfdaemon::DBUS_NAME, session_object_path);
+}
 
 bool DbusCallback::signature_valid(sdbus::Signal & signal) {
     // check that signal is emitted by the correct session object
     sdbus::ObjectPath object_path;
     signal >> object_path;
-    return object_path == context.get_session_object_path();
+    return object_path == session_object_path;
 }
 
 
-DownloadCB::DownloadCB(Context & context) : DbusCallback(context) {
+DownloadCB::DownloadCB(Context & context, sdbus::IConnection & connection)
+    : DbusCallback(context, connection),
+      assume_yes(context.get_assumeyes_option()),
+      assume_no(context.get_assumeno_option()),
+      default_yes(context.get_defaultyes_option()) {}
+
+void DownloadCB::register_signals() {
     // register signal handlers
-    auto proxy = context.session_proxy.get();
-    proxy->registerSignalHandler(
+    session_proxy->registerSignalHandler(
         dnfdaemon::INTERFACE_BASE, dnfdaemon::SIGNAL_DOWNLOAD_ADD_NEW, [this](sdbus::Signal signal) -> void {
             this->add_new_download(signal);
         });
-    proxy->registerSignalHandler(
+    session_proxy->registerSignalHandler(
         dnfdaemon::INTERFACE_BASE, dnfdaemon::SIGNAL_DOWNLOAD_END, [this](sdbus::Signal signal) -> void {
             this->end(signal);
         });
-    proxy->registerSignalHandler(
+    session_proxy->registerSignalHandler(
         dnfdaemon::INTERFACE_BASE, dnfdaemon::SIGNAL_DOWNLOAD_PROGRESS, [this](sdbus::Signal signal) -> void {
             this->progress(signal);
         });
-    proxy->registerSignalHandler(
+    session_proxy->registerSignalHandler(
         dnfdaemon::INTERFACE_BASE, dnfdaemon::SIGNAL_DOWNLOAD_MIRROR_FAILURE, [this](sdbus::Signal signal) -> void {
             this->mirror_failure(signal);
         });
-    proxy->registerSignalHandler(
+    session_proxy->registerSignalHandler(
         dnfdaemon::INTERFACE_BASE, dnfdaemon::SIGNAL_REPO_KEY_IMPORT_REQUEST, [this](sdbus::Signal signal) -> void {
             this->key_import(signal);
         });
+#ifndef SDBUS_CPP_VERSION_2
+    session_proxy->finishRegistration();
+#endif
 }
 
 
@@ -224,11 +236,11 @@ void DownloadCB::key_import(sdbus::Signal & signal) {
         std::cerr << " From       : " + url << std::endl;
 
         // ask user for the key import confirmation
-        auto confirmed = libdnf5::cli::utils::userconfirm::userconfirm(context);
+        auto confirmed = libdnf5::cli::utils::userconfirm::userconfirm(*this);
 
         // signal the confirmation back to the server
         try {
-            context.session_proxy->callMethod("confirm_key")
+            session_proxy->callMethod("confirm_key")
                 .onInterface(dnfdaemon::INTERFACE_REPO)
                 .withTimeout(static_cast<uint64_t>(-1))
                 .withArguments(key_id, confirmed);
@@ -238,61 +250,66 @@ void DownloadCB::key_import(sdbus::Signal & signal) {
     }
 }
 
-TransactionCB::TransactionCB(Context & context) : DbusCallback(context) {
+TransactionCB::TransactionCB(Context & context, sdbus::IConnection & connection) : DbusCallback(context, connection) {}
+
+
+void TransactionCB::register_signals() {
     // register signal handlers
-    auto proxy = context.session_proxy.get();
-    proxy->registerSignalHandler(
+    session_proxy->registerSignalHandler(
         dnfdaemon::INTERFACE_RPM, dnfdaemon::SIGNAL_TRANSACTION_VERIFY_START, [this](sdbus::Signal signal) -> void {
             this->verify_start(signal);
         });
-    proxy->registerSignalHandler(
+    session_proxy->registerSignalHandler(
         dnfdaemon::INTERFACE_RPM, dnfdaemon::SIGNAL_TRANSACTION_VERIFY_PROGRESS, [this](sdbus::Signal signal) -> void {
             this->verify_progress(signal);
         });
-    proxy->registerSignalHandler(
+    session_proxy->registerSignalHandler(
         dnfdaemon::INTERFACE_RPM, dnfdaemon::SIGNAL_TRANSACTION_VERIFY_STOP, [this](sdbus::Signal signal) -> void {
             this->verify_end(signal);
         });
-    proxy->registerSignalHandler(
+    session_proxy->registerSignalHandler(
         dnfdaemon::INTERFACE_RPM,
         dnfdaemon::SIGNAL_TRANSACTION_TRANSACTION_START,
         [this](sdbus::Signal signal) -> void { this->transaction_start(signal); });
-    proxy->registerSignalHandler(
+    session_proxy->registerSignalHandler(
         dnfdaemon::INTERFACE_RPM,
         dnfdaemon::SIGNAL_TRANSACTION_TRANSACTION_PROGRESS,
         [this](sdbus::Signal signal) -> void { this->transaction_progress(signal); });
-    proxy->registerSignalHandler(
+    session_proxy->registerSignalHandler(
         dnfdaemon::INTERFACE_RPM, dnfdaemon::SIGNAL_TRANSACTION_TRANSACTION_STOP, [this](sdbus::Signal signal) -> void {
             this->transaction_end(signal);
         });
-    proxy->registerSignalHandler(
+    session_proxy->registerSignalHandler(
         dnfdaemon::INTERFACE_RPM, dnfdaemon::SIGNAL_TRANSACTION_ACTION_START, [this](sdbus::Signal signal) -> void {
             this->action_start(signal);
         });
-    proxy->registerSignalHandler(
+    session_proxy->registerSignalHandler(
         dnfdaemon::INTERFACE_RPM, dnfdaemon::SIGNAL_TRANSACTION_ACTION_PROGRESS, [this](sdbus::Signal signal) -> void {
             this->action_progress(signal);
         });
-    proxy->registerSignalHandler(
+    session_proxy->registerSignalHandler(
         dnfdaemon::INTERFACE_RPM, dnfdaemon::SIGNAL_TRANSACTION_ACTION_STOP, [this](sdbus::Signal signal) -> void {
             this->action_end(signal);
         });
-    proxy->registerSignalHandler(
+    session_proxy->registerSignalHandler(
         dnfdaemon::INTERFACE_RPM, dnfdaemon::SIGNAL_TRANSACTION_SCRIPT_START, [this](sdbus::Signal signal) -> void {
             this->script_start(signal);
         });
-    proxy->registerSignalHandler(
+    session_proxy->registerSignalHandler(
         dnfdaemon::INTERFACE_RPM, dnfdaemon::SIGNAL_TRANSACTION_SCRIPT_STOP, [this](sdbus::Signal signal) -> void {
             this->script_stop(signal);
         });
-    proxy->registerSignalHandler(
+    session_proxy->registerSignalHandler(
         dnfdaemon::INTERFACE_RPM, dnfdaemon::SIGNAL_TRANSACTION_SCRIPT_ERROR, [this](sdbus::Signal signal) -> void {
             this->script_error(signal);
         });
-    proxy->registerSignalHandler(
+    session_proxy->registerSignalHandler(
         dnfdaemon::INTERFACE_RPM, dnfdaemon::SIGNAL_TRANSACTION_FINISHED, [this](sdbus::Signal signal) -> void {
             this->finished(signal);
         });
+#ifndef SDBUS_CPP_VERSION_2
+    session_proxy->finishRegistration();
+#endif
 }
 
 void TransactionCB::new_progress_bar(uint64_t total, const std::string & description) {

diff --git a/dnf5daemon-client/callbacks.hpp b/dnf5daemon-client/callbacks.hpp
@@ -22,6 +22,7 @@ along with libdnf.  If not, see <https://www.gnu.org/licenses/>.
 
 #include <libdnf5-cli/progressbar/download_progress_bar.hpp>
 #include <libdnf5-cli/progressbar/multi_progress_bar.hpp>
+#include <libdnf5/conf/option_bool.hpp>
 #include <sdbus-c++/sdbus-c++.h>
 
 #include <string>
@@ -32,19 +33,20 @@ class Context;
 
 class DbusCallback {
 public:
-    explicit DbusCallback(Context & context) : context(context) {};
+    explicit DbusCallback(Context & context, sdbus::IConnection & connection);
     virtual ~DbusCallback() = default;
+    virtual void register_signals() = 0;
 
 protected:
-    Context & context;
-
     bool signature_valid(sdbus::Signal & signal);
+    sdbus::ObjectPath session_object_path;
+    std::unique_ptr<sdbus::IProxy> session_proxy;
 };
 
 
 class DownloadCB final : public DbusCallback {
 public:
-    explicit DownloadCB(Context & context);
+    explicit DownloadCB(Context & context, sdbus::IConnection & connection);
     virtual ~DownloadCB() = default;
 
     void add_new_download(sdbus::Signal & signal);
@@ -57,6 +59,13 @@ class DownloadCB final : public DbusCallback {
     void set_number_widget_visible(bool value);
     void set_show_total_bar_limit(std::size_t limit);
 
+    // methods required by cli::utils::userconfirm::userconfirm
+    libdnf5::OptionBool get_assumeno_option() const { return assume_no; }
+    libdnf5::OptionBool get_assumeyes_option() const { return assume_yes; }
+    libdnf5::OptionBool get_defaultyes_option() const { return default_yes; }
+
+    void register_signals() override;
+
 private:
     libdnf5::cli::progressbar::DownloadProgressBar * find_progress_bar(const std::string & download_id);
     void print();
@@ -67,12 +76,15 @@ class DownloadCB final : public DbusCallback {
     std::unique_ptr<libdnf5::cli::progressbar::MultiProgressBar> multi_progress_bar;
     // map {download_id: progressbar}
     std::unordered_map<std::string, libdnf5::cli::progressbar::DownloadProgressBar *> progress_bars;
+    libdnf5::OptionBool assume_yes{false};
+    libdnf5::OptionBool assume_no{false};
+    libdnf5::OptionBool default_yes{false};
 };
 
 
 class TransactionCB final : public DbusCallback {
 public:
-    explicit TransactionCB(Context & context);
+    explicit TransactionCB(Context & context, sdbus::IConnection & connection);
     virtual ~TransactionCB() = default;
 
     void verify_start(sdbus::Signal & signal);
@@ -95,6 +107,8 @@ class TransactionCB final : public DbusCallback {
 
     void finished(sdbus::Signal & signal);
 
+    void register_signals() override;
+
 private:
     libdnf5::cli::progressbar::MultiProgressBar multi_progress_bar;
     libdnf5::cli::progressbar::DownloadProgressBar * active_progress_bar{nullptr};

diff --git a/dnf5daemon-client/context.cpp b/dnf5daemon-client/context.cpp
@@ -62,9 +62,6 @@ void Context::init_session(sdbus::IConnection & connection) {
         .storeResultsTo(session_object_path);
 
     session_proxy = sdbus::createProxy(connection, dnfdaemon::DBUS_NAME, session_object_path);
-    // register progress bars callbacks
-    download_cb = std::make_unique<DownloadCB>(*this);
-    transaction_cb = std::make_unique<TransactionCB>(*this);
 #ifndef SDBUS_CPP_VERSION_2
     session_proxy->finishRegistration();
 #endif

diff --git a/dnf5daemon-client/context.hpp b/dnf5daemon-client/context.hpp
@@ -45,7 +45,7 @@ class Context : public libdnf5::cli::session::Session {
 
     /// Initialize dbus connection and server session
     void init_session(sdbus::IConnection & connection);
-    sdbus::ObjectPath & get_session_object_path() { return session_object_path; };
+    sdbus::ObjectPath get_session_object_path() { return session_object_path; };
 
     // signal handlers
     void on_repositories_ready(const bool & result);
@@ -68,12 +68,12 @@ class Context : public libdnf5::cli::session::Session {
     libdnf5::OptionString releasever{""};
 
     void reset_download_cb();
+    void set_download_cb(DownloadCB * download_cb) { this->download_cb = download_cb; }
 
 private:
     sdbus::ObjectPath session_object_path;
     dnfdaemon::RepoStatus repositories_status;
-    std::unique_ptr<DownloadCB> download_cb;
-    std::unique_ptr<TransactionCB> transaction_cb;
+    DownloadCB * download_cb{nullptr};
 };
 
 }  // namespace dnfdaemon::client

diff --git a/dnf5daemon-client/main.cpp b/dnf5daemon-client/main.cpp
@@ -17,6 +17,7 @@ You should have received a copy of the GNU General Public License
 along with libdnf.  If not, see <https://www.gnu.org/licenses/>.
 */
 
+#include "callbacks.hpp"
 #include "commands/advisory/advisory.hpp"
 #include "commands/clean/clean.hpp"
 #include "commands/distro-sync/distro-sync.hpp"
@@ -231,8 +232,6 @@ static void set_locale() {
 
 
 int main(int argc, char * argv[]) {
-    std::unique_ptr<sdbus::IConnection> connection;
-
     set_locale();
 
     dnfdaemon::client::Context context;
@@ -272,6 +271,7 @@ int main(int argc, char * argv[]) {
     try {
         command->pre_configure();
 
+        std::unique_ptr<sdbus::IConnection> connection;
         try {
             connection = sdbus::createSystemBusConnection();
         } catch (const sdbus::Error & ex) {
@@ -290,10 +290,20 @@ int main(int argc, char * argv[]) {
             return static_cast<int>(libdnf5::cli::ExitCode::ERROR);
         }
 
-        // Run selected command
-        command->run();
+        {
+            auto download_cb = std::make_unique<dnfdaemon::client::DownloadCB>(context, *connection);
+            auto transaction_cb = std::make_unique<dnfdaemon::client::TransactionCB>(context, *connection);
+            download_cb->register_signals();
+            transaction_cb->register_signals();
+
+            context.set_download_cb(download_cb.get());
 
-        connection->leaveEventLoop();
+            // Run selected command
+            command->run();
+            context.set_download_cb(nullptr);
+
+            connection->leaveEventLoop();
+        }
     } catch (libdnf5::cli::ArgumentParserError & ex) {
         std::cerr << ex.what() << _(". Add \"--help\" for more information about the arguments.") << std::endl;
         return static_cast<int>(libdnf5::cli::ExitCode::ARGPARSER_ERROR);