From dbea32809ae7b75d52d236ebe1e6a4739d453349 Mon Sep 17 00:00:00 2001 From: Mike Gelfand Date: Tue, 13 Aug 2024 06:19:36 +0100 Subject: [PATCH] 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 | 71 +++++++++++++------------------- libtransmission/torrent-files.cc | 27 ++++-------- libtransmission/torrent.cc | 16 +++---- libtransmission/torrent.h | 8 ++-- libtransmission/transmission.h | 6 +-- libtransmission/utils.cc | 46 ++++----------------- libtransmission/utils.h | 11 +---- 7 files changed, 59 insertions(+), 126 deletions(-) diff --git a/gtk/Session.cc b/gtk/Session.cc index 76347365e..5327a21c9 100644 --- a/gtk/Session.cc +++ b/gtk/Session.cc @@ -32,7 +32,6 @@ #include #include #include -#include #include #include @@ -52,7 +51,6 @@ #include #include #include -#include #include #include #include @@ -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(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; - 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(static_cast(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); } } diff --git a/libtransmission/torrent-files.cc b/libtransmission/torrent-files.cc index 47bec1467..db5bc811b 100644 --- a/libtransmission/torrent-files.cc +++ b/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{ 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_files; - - auto const paths = std::array{ 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; } } } diff --git a/libtransmission/torrent.cc b/libtransmission/torrent.cc index 1d9ee36f2..6a6d85374 100644 --- a/libtransmission/torrent.cc +++ b/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) diff --git a/libtransmission/torrent.h b/libtransmission/torrent.h index 967b59cde..d15997adb 100644 --- a/libtransmission/torrent.h +++ b/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); diff --git a/libtransmission/transmission.h b/libtransmission/transmission.h index f00205f49..ddd3e0e6e 100644 --- a/libtransmission/transmission.h +++ b/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); diff --git a/libtransmission/utils.cc b/libtransmission/utils.cc index e6680d3c8..c7ec3a1e4 100644 --- a/libtransmission/utils.cc +++ b/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)) { diff --git a/libtransmission/utils.h b/libtransmission/utils.h index 46e9bdbb7..d2822bdb5 100644 --- a/libtransmission/utils.h +++ b/libtransmission/utils.h @@ -64,18 +64,11 @@ std::optional tr_locale_set_global(std::locale const& locale) noexc bool tr_file_read(std::string_view filename, std::vector& 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);