Refactor async torrent removal logic (#7059)

* Remove `tr_file_move_strict()`, add param to `tr_file_move()` instead

Also restores proper `local_error` logic to handle null error param.

* Remove unused `moved_files` logic

* Rename the callback type and params to match rename decls

* Add torrent ID param to torrent removal callback

* Remove mutex from torrent removal callback

Torrent IDs aren't reused during the lifetime of the session.

* Move main removal callback logic into a separate method

gtk/Session.cc

@@ -32,7 +32,6 @@
 #include <glibmm/i18n.h>
 #include <glibmm/main.h>
 #include <glibmm/miscutils.h>
-#include <glibmm/refptr.h>
 #include <glibmm/stringutils.h>
 #include <glibmm/variant.h>
 
@@ -52,7 +51,6 @@
 #include <iostream>
 #include <map>
 #include <memory>
-#include <mutex>
 #include <optional>
 #include <string>
 #include <string_view>
@@ -170,6 +168,8 @@ private:
     void on_torrent_completeness_changed(tr_torrent* tor, tr_completeness completeness, bool was_running);
     void on_torrent_metadata_changed(tr_torrent* raw_torrent);
 
+    void on_torrent_removal_done(tr_torrent_id_t id, bool succeeded);
+
 private:
     Session& core_;
 
@@ -914,56 +914,41 @@ void Session::remove_torrent(tr_torrent_id_t id, bool delete_files)
 
 void Session::Impl::remove_torrent(tr_torrent_id_t id, bool delete_files)
 {
+    static auto const callback = [](tr_torrent_id_t processed_id, bool succeeded, void* user_data)
+    {
+        // "Own" the core since refcount has already been incremented before operation start — only decrement required.
+        auto const core = Glib::make_refptr_for_instance(static_cast<Session*>(user_data));
+
+        Glib::signal_idle().connect_once([processed_id, succeeded, core]()
+                                         { core->impl_->on_torrent_removal_done(processed_id, succeeded); });
+    };
+
     if (auto const& [torrent, position] = find_torrent_by_id(id); torrent)
     {
-        struct CallbackUserData
-        {
-            Glib::RefPtr<Session> session;
-            tr_torrent_id_t id;
-        };
+        // Extend core lifetime, refcount will be decremented in the callback.
+        core_.reference();
 
-        // Callback to remove the torrent entry from the GUI if it was
-        // successfuly removed. This is called from the libtransmission thread.
-        auto handle_result = [](bool succeeded, void* user_data)
-        {
-            std::mutex wait_cb_thread;
-            wait_cb_thread.lock();
-
-            // Schedule the actual handler in the main thread:
-            Glib::signal_idle().connect_once(
-                [=, &wait_cb_thread]()
-                {
-                    // Take ownership of the raw pointer, so it gets deleted when done.
-                    auto ud = std::unique_ptr<CallbackUserData>(static_cast<CallbackUserData*>(user_data));
-
-                    if (succeeded)
-                    {
-                        auto const& impl = *ud->session->impl_;
-                        if (auto const& [torrent_, position_] = impl.find_torrent_by_id(ud->id); torrent_)
-                        {
-                            /* remove from the gui */
-                            impl.get_raw_model()->remove(position_);
-                        }
-                    }
-
-                    wait_cb_thread.unlock();
-                });
-
-            // It is better to wait for the idle signal to be processed before
-            // returning, just to avoid the extremely improbable case of the
-            // tr_torrent_id_t being reused before the idle signal is processed.
-            std::lock_guard waiter{ wait_cb_thread };
-        };
-
-        /* remove the torrent */
         tr_torrentRemove(
             &torrent->get_underlying(),
             delete_files,
             [](char const* filename, void* /*user_data*/, tr_error* error)
             { return gtr_file_trash_or_remove(filename, error); },
             nullptr,
-            handle_result,
-            new CallbackUserData{ get_core_ptr(), id });
+            callback,
+            &core_);
+    }
+}
+
+void Session::Impl::on_torrent_removal_done(tr_torrent_id_t id, bool succeeded)
+{
+    if (!succeeded)
+    {
+        return;
+    }
+
+    if (auto const& [torrent, position] = find_torrent_by_id(id); torrent)
+    {
+        get_raw_model()->remove(position);
     }
 }
 

libtransmission/torrent-files.cc

@@ -198,7 +198,7 @@ bool tr_torrent_files::move(
         }
 
         tr_logAddTrace(fmt::format("Moving file #{:d} to '{:s}'", i, old_path, path), parent_name);
-        if (!tr_file_move(old_path, path, error))
+        if (!tr_file_move(old_path, path, true, error))
         {
             err = true;
             break;
@@ -252,28 +252,15 @@ void tr_torrent_files::remove(std::string_view parent_in, std::string_view tmpdi
     }
 
     // move the local data to the tmpdir
+    auto const paths = std::array<std::string_view, 1>{ parent.sv() };
+    for (tr_file_index_t idx = 0, n_files = file_count(); idx < n_files; ++idx)
     {
-        struct moved_file
+        if (auto const found = find(idx, std::data(paths), std::size(paths)); found)
         {
-            tr_pathbuf from;
-            tr_pathbuf to;
-        };
-        std::vector<moved_file> moved_files;
-
-        auto const paths = std::array<std::string_view, 1>{ parent.sv() };
-        for (tr_file_index_t idx = 0, n_files = file_count(); idx < n_files; ++idx)
-        {
-            if (auto const found = find(idx, std::data(paths), std::size(paths)); found)
+            // if moving a file fails, give up and let the error propagate
+            if (!tr_file_move(found->filename(), tr_pathbuf{ tmpdir, '/', found->subpath() }, false, error))
             {
-                auto const f = moved_file{ found->filename(), tr_pathbuf{ tmpdir, '/', found->subpath() } };
-
-                // if moving a file fails, give up and let the error propagate
-                if (!tr_file_move_strict(f.from, f.to, error))
-                {
-                    return;
-                }
-
-                moved_files.push_back(f);
+                return;
             }
         }
     }

libtransmission/torrent.cc

@@ -763,8 +763,8 @@ void tr_torrentRemoveInSessionThread(
     bool delete_flag,
     tr_fileFunc delete_func,
     void* delete_user_data,
-    tr_result_notify_func notify_func,
-    void* notify_user_data)
+    tr_torrent_remove_done_func callback,
+    void* callback_user_data)
 {
     auto const lock = tor->unique_lock();
 
@@ -800,9 +800,9 @@ void tr_torrentRemoveInSessionThread(
         }
     }
 
-    if (notify_func != nullptr)
+    if (callback != nullptr)
     {
-        notify_func(ok, notify_user_data);
+        callback(tor->id(), ok, callback_user_data);
     }
 
     if (ok)
@@ -830,8 +830,8 @@ void tr_torrentRemove(
     bool delete_flag,
     tr_fileFunc delete_func,
     void* delete_user_data,
-    tr_result_notify_func notify_func,
-    void* notify_user_data)
+    tr_torrent_remove_done_func callback,
+    void* callback_user_data)
 {
     using namespace start_stop_helpers;
 
@@ -845,8 +845,8 @@ void tr_torrentRemove(
         delete_flag,
         delete_func,
         delete_user_data,
-        notify_func,
-        notify_user_data);
+        callback,
+        callback_user_data);
 }
 
 void tr_torrentFreeInSessionThread(tr_torrent* tor)

libtransmission/torrent.h

@@ -989,15 +989,15 @@ private:
         bool delete_flag,
         tr_fileFunc delete_func,
         void* delete_user_data,
-        tr_result_notify_func notify_func,
-        void* notify_user_data);
+        tr_torrent_remove_done_func callback,
+        void* callback_user_data);
     friend void tr_torrentRemove(
         tr_torrent* tor,
         bool delete_flag,
         tr_fileFunc delete_func,
         void* delete_user_data,
-        tr_result_notify_func notify_func,
-        void* notify_user_data);
+        tr_torrent_remove_done_func callback,
+        void* callback_user_data);
     friend void tr_torrentSetDownloadDir(tr_torrent* tor, char const* path);
     friend void tr_torrentSetPriority(tr_torrent* tor, tr_priority_t priority);
     friend void tr_torrentStart(tr_torrent* tor);

libtransmission/transmission.h

@@ -829,7 +829,7 @@ tr_torrent* tr_torrentNew(tr_ctor* ctor, tr_torrent** setme_duplicate_of);
 
 using tr_fileFunc = bool (*)(char const* filename, void* user_data, tr_error* error);
 
-using tr_result_notify_func = void (*)(bool succeeded, void* user_data);
+using tr_torrent_remove_done_func = void (*)(tr_torrent_id_t id, bool succeeded, void* user_data);
 
 /** @brief Removes our torrent and .resume files for this torrent */
 void tr_torrentRemove(
@@ -837,8 +837,8 @@ void tr_torrentRemove(
     bool delete_flag,
     tr_fileFunc delete_func,
     void* delete_user_data,
-    tr_result_notify_func notify_func,
-    void* notify_user_data);
+    tr_torrent_remove_done_func callback,
+    void* callback_user_data);
 
 /** @brief Start a torrent */
 void tr_torrentStart(tr_torrent* torrent);

libtransmission/utils.cc

@@ -569,13 +569,11 @@ std::string tr_strratio(double ratio, char const* infinity)
 
 // ---
 
-namespace
-{
-namespace tr_file_move_impl
+bool tr_file_move(std::string_view oldpath_in, std::string_view newpath_in, bool allow_copy, tr_error* error)
 {
+    auto const oldpath = tr_pathbuf{ oldpath_in };
+    auto const newpath = tr_pathbuf{ newpath_in };
 
-bool check_paths(std::string_view const oldpath, std::string_view const newpath, tr_error* error)
-{
     auto local_error = tr_error{};
     if (error == nullptr)
     {
@@ -604,48 +602,18 @@ bool check_paths(std::string_view const oldpath, std::string_view const newpath,
         return false;
     }
 
-    return true;
-}
-
-} // namespace tr_file_move_impl
-} // namespace
-
-bool tr_file_move_strict(std::string_view oldpath_in, std::string_view newpath_in, tr_error* error)
-{
-    if (!tr_file_move_impl::check_paths(oldpath_in, newpath_in, error))
+    /* they might be on the same filesystem... */
+    if (tr_sys_path_rename(oldpath, newpath, error))
     {
-        return false;
+        return true;
     }
 
-    auto const oldpath = tr_pathbuf{ oldpath_in };
-    auto const newpath = tr_pathbuf{ newpath_in };
-
-    /* do the actual moving */
-    if (!tr_sys_path_rename(oldpath, newpath, error))
+    if (!allow_copy)
     {
         error->prefix_message("Unable to move file: ");
         return false;
     }
 
-    return true;
-}
-
-bool tr_file_move(std::string_view oldpath_in, std::string_view newpath_in, tr_error* error)
-{
-    if (!tr_file_move_impl::check_paths(oldpath_in, newpath_in, error))
-    {
-        return false;
-    }
-
-    auto const oldpath = tr_pathbuf{ oldpath_in };
-    auto const newpath = tr_pathbuf{ newpath_in };
-
-    /* they might be on the same filesystem... */
-    if (tr_sys_path_rename(oldpath, newpath))
-    {
-        return true;
-    }
-
     /* Otherwise, copy the file. */
     if (!tr_sys_path_copy(oldpath, newpath, error))
     {

libtransmission/utils.h

@@ -64,18 +64,11 @@ std::optional<std::locale> tr_locale_set_global(std::locale const& locale) noexc
 bool tr_file_read(std::string_view filename, std::vector<char>& contents, tr_error* error = nullptr);
 
 /**
- * Tries to move a file by renaming, but never by copying.
+ * Tries to move a file by renaming, and [optionally] if that fails, by copying.
  *
  * Creates the destination directory if it doesn't exist.
  */
-bool tr_file_move_strict(std::string_view oldpath_in, std::string_view newpath_in, tr_error* error = nullptr);
-
-/**
- * Tries to move a file by renaming, and if that fails, by copying.
- *
- * Creates the destination directory if it doesn't exist.
- */
-bool tr_file_move(std::string_view oldpath, std::string_view newpath, tr_error* error = nullptr);
+bool tr_file_move(std::string_view oldpath, std::string_view newpath, bool allow_copy, tr_error* error = nullptr);
 
 bool tr_file_save(std::string_view filename, std::string_view contents, tr_error* error = nullptr);