From 02053ad8f27e7c42576d771a5be96552ec9da5ad Mon Sep 17 00:00:00 2001 From: "Scott E. Graves" Date: Tue, 24 Dec 2024 13:23:46 -0600 Subject: [PATCH] continue dead-lock fix --- .../include/file_manager/open_file.hpp | 12 +- .../librepertory/src/drives/eviction.cpp | 7 +- .../src/file_manager/file_manager.cpp | 41 ++++--- .../src/file_manager/open_file.cpp | 104 ++++++++++-------- .../librepertory/src/file_manager/upload.cpp | 3 +- .../src/providers/base_provider.cpp | 36 +++--- 6 files changed, 121 insertions(+), 82 deletions(-) diff --git a/repertory/librepertory/include/file_manager/open_file.hpp b/repertory/librepertory/include/file_manager/open_file.hpp index 67f59254..2de570f5 100644 --- a/repertory/librepertory/include/file_manager/open_file.hpp +++ b/repertory/librepertory/include/file_manager/open_file.hpp @@ -67,13 +67,13 @@ private: i_upload_manager &mgr_; private: - bool notified_ = false; + bool notified_{false}; std::size_t read_chunk_{}; boost::dynamic_bitset<> read_state_; std::unique_ptr reader_thread_; std::unique_ptr download_thread_; mutable std::recursive_mutex rw_mtx_; - stop_type stop_requested_ = false; + stop_type stop_requested_{false}; private: void download_chunk(std::size_t chunk, bool skip_active, bool should_reset); @@ -83,12 +83,14 @@ private: void set_modified(); + void set_read_state(std::size_t chunk); + + void set_read_state(boost::dynamic_bitset<> read_state); + void update_background_reader(std::size_t read_chunk); protected: - auto is_download_complete() const -> bool override { - return read_state_.all(); - } + [[nodiscard]] auto is_download_complete() const -> bool override; public: auto close() -> bool override; diff --git a/repertory/librepertory/src/drives/eviction.cpp b/repertory/librepertory/src/drives/eviction.cpp index b22ccff0..1d14397a 100644 --- a/repertory/librepertory/src/drives/eviction.cpp +++ b/repertory/librepertory/src/drives/eviction.cpp @@ -30,6 +30,7 @@ #include "utils/file_utils.hpp" #include "utils/time.hpp" #include "utils/utils.hpp" +#include namespace repertory { auto eviction::check_minimum_requirements(const std::string &file_path) @@ -75,8 +76,10 @@ void eviction::service_function() { try { std::string api_path; - if (provider_.get_api_path_from_source(file_path, api_path) != - api_error::success) { + fmt::println("path|{}", file_path); + auto res = provider_.get_api_path_from_source(file_path, api_path); + if (res != api_error::success) { + fmt::println("not found|{}", api_error_to_string(res)); continue; } diff --git a/repertory/librepertory/src/file_manager/file_manager.cpp b/repertory/librepertory/src/file_manager/file_manager.cpp index d96ce09c..4c65c513 100644 --- a/repertory/librepertory/src/file_manager/file_manager.cpp +++ b/repertory/librepertory/src/file_manager/file_manager.cpp @@ -149,11 +149,13 @@ auto file_manager::evict_file(const std::string &api_path) -> bool { fmt::println("proccessing|{}", api_path); return false; } + fmt::println("not proccessing|{}", api_path); if (get_open_file_count(api_path) != 0U) { fmt::println("open count|{}", api_path); return false; } + fmt::println("not open|{}", api_path); std::string pinned; auto res = provider_.get_item_meta(api_path, META_PINNED, pinned); @@ -245,7 +247,7 @@ auto file_manager::get_open_file_count(const std::string &api_path) const auto file_manager::get_open_file(std::uint64_t handle, bool write_supported, std::shared_ptr &file) -> bool { - recur_mutex_lock open_lock(open_file_mtx_); + unique_recur_mutex_lock open_lock(open_file_mtx_); auto file_ptr = get_open_file_by_handle(handle); if (not file_ptr) { return false; @@ -357,23 +359,31 @@ auto file_manager::is_processing(const std::string &api_path) const -> bool { return false; } + fmt::println("ul lock|{}", api_path); unique_mutex_lock upload_lock(upload_mtx_); if (upload_lookup_.find(api_path) != upload_lookup_.end()) { return true; } upload_lock.unlock(); + fmt::println("ul unlocked|{}", api_path); auto upload = mgr_db_->get_upload(api_path); if (upload.has_value()) { return true; }; - recur_mutex_lock open_lock(open_file_mtx_); + fmt::println("of lock|{}", api_path); + unique_recur_mutex_lock open_lock(open_file_mtx_); auto file_iter = open_file_lookup_.find(api_path); - return (file_iter == open_file_lookup_.end()) - ? false - : file_iter->second->is_modified() || - not file_iter->second->is_complete(); + if (file_iter == open_file_lookup_.end()) { + return false; + } + + auto file = file_iter->second; + fmt::println("of unlocked|{}", api_path); + open_lock.unlock(); + + return file->is_modified() || not file->is_complete(); } auto file_manager::open(const std::string &api_path, bool directory, @@ -516,9 +526,9 @@ void file_manager::queue_upload(const std::string &api_path, return; } - std::unique_ptr lock; + std::unique_ptr upload_lock; if (not no_lock) { - lock = std::make_unique(upload_mtx_); + upload_lock = std::make_unique(upload_mtx_); } remove_upload(api_path, true); @@ -542,8 +552,6 @@ void file_manager::queue_upload(const std::string &api_path, auto file_manager::remove_file(const std::string &api_path) -> api_error { REPERTORY_USES_FUNCTION_NAME(); - recur_mutex_lock open_lock(open_file_mtx_); - filesystem_item fsi{}; auto res = provider_.get_filesystem_item(api_path, false, fsi); if (res != api_error::success) { @@ -552,10 +560,13 @@ auto file_manager::remove_file(const std::string &api_path) -> api_error { close_all(api_path); - mutex_lock lock(upload_mtx_); + unique_mutex_lock upload_lock(upload_mtx_); remove_upload(api_path, true); remove_resume(api_path, fsi.source_path, true); upload_notify_.notify_all(); + upload_lock.unlock(); + + recur_mutex_lock open_lock(open_file_mtx_); res = provider_.remove_file(api_path); if (res != api_error::success) { @@ -591,9 +602,9 @@ void file_manager::remove_resume(const std::string &api_path, return; } - std::unique_ptr lock; + std::unique_ptr upload_lock; if (not no_lock) { - lock = std::make_unique(upload_mtx_); + upload_lock = std::make_unique(upload_mtx_); } if (mgr_db_->remove_resume(api_path)) { @@ -617,9 +628,9 @@ void file_manager::remove_upload(const std::string &api_path, bool no_lock) { return; } - std::unique_ptr lock; + std::unique_ptr upload_lock; if (not no_lock) { - lock = std::make_unique(upload_mtx_); + upload_lock = std::make_unique(upload_mtx_); } if (not mgr_db_->remove_upload(api_path)) { diff --git a/repertory/librepertory/src/file_manager/open_file.cpp b/repertory/librepertory/src/file_manager/open_file.cpp index 4297c13f..df67b025 100644 --- a/repertory/librepertory/src/file_manager/open_file.cpp +++ b/repertory/librepertory/src/file_manager/open_file.cpp @@ -135,12 +135,13 @@ auto open_file::close() -> bool { return false; } + auto read_state = get_read_state(); auto err = get_api_error(); if (err == api_error::success || err == api_error::download_incomplete || err == api_error::download_stopped) { - if (modified_ && not read_state_.all()) { + if (modified_ && not read_state.all()) { set_api_error(api_error::download_incomplete); - } else if (not modified_ && (fsi_.size > 0U) && not read_state_.all()) { + } else if (not modified_ && (fsi_.size > 0U) && not read_state.all()) { set_api_error(api_error::download_stopped); } @@ -161,7 +162,7 @@ auto open_file::close() -> bool { } } - if (err != api_error::success || read_state_.all()) { + if (err != api_error::success || read_state.all()) { mgr_.remove_resume(fsi_.api_path, get_source_path()); } @@ -204,16 +205,17 @@ void open_file::download_chunk(std::size_t chunk, bool skip_active, reset_timeout(); } - unique_recur_mutex_lock file_lock(rw_mtx_); - if ((get_api_error() == api_error::success) && (chunk < read_state_.size()) && - not read_state_[chunk]) { + unique_recur_mutex_lock rw_lock(rw_mtx_); + auto read_state = get_read_state(); + if ((get_api_error() == api_error::success) && (chunk < read_state.size()) && + not read_state[chunk]) { if (active_downloads_.find(chunk) != active_downloads_.end()) { if (skip_active) { return; } auto active_download = active_downloads_.at(chunk); - file_lock.unlock(); + rw_lock.unlock(); active_download->wait(); return; @@ -221,17 +223,17 @@ void open_file::download_chunk(std::size_t chunk, bool skip_active, auto data_offset = chunk * chunk_size_; auto data_size = - (chunk == read_state_.size() - 1U) ? last_chunk_size_ : chunk_size_; - if (active_downloads_.empty() && (read_state_.count() == 0U)) { + (chunk == read_state.size() - 1U) ? last_chunk_size_ : chunk_size_; + if (active_downloads_.empty() && (read_state.count() == 0U)) { event_system::instance().raise(fsi_.api_path, fsi_.source_path); } event_system::instance().raise( - fsi_.api_path, fsi_.source_path, chunk, read_state_.size(), - read_state_.count()); + fsi_.api_path, fsi_.source_path, chunk, read_state.size(), + read_state.count()); active_downloads_[chunk] = std::make_shared(); - file_lock.unlock(); + rw_lock.unlock(); if (should_reset) { reset_timeout(); @@ -240,19 +242,21 @@ void open_file::download_chunk(std::size_t chunk, bool skip_active, std::async(std::launch::async, [this, chunk, data_size, data_offset, should_reset]() { const auto notify_complete = [this, chunk, should_reset]() { + auto state = get_read_state(); + unique_recur_mutex_lock lock(rw_mtx_); auto active_download = active_downloads_.at(chunk); active_downloads_.erase(chunk); event_system::instance().raise( - fsi_.api_path, fsi_.source_path, chunk, read_state_.size(), - read_state_.count(), get_api_error()); + fsi_.api_path, fsi_.source_path, chunk, state.size(), state.count(), + get_api_error()); if (get_api_error() == api_error::success) { - auto progress = (static_cast(read_state_.count()) / - static_cast(read_state_.size())) * + auto progress = (static_cast(state.count()) / + static_cast(state.size())) * 100.0; event_system::instance().raise( fsi_.api_path, fsi_.source_path, progress); - if (read_state_.all() && not notified_) { + if (state.all() && not notified_) { notified_ = true; event_system::instance().raise( fsi_.api_path, fsi_.source_path, get_api_error()); @@ -301,9 +305,7 @@ void open_file::download_chunk(std::size_t chunk, bool skip_active, return; } - unique_recur_mutex_lock lock(rw_mtx_); - read_state_.set(chunk); - lock.unlock(); + set_read_state(chunk); notify_complete(); }).wait(); @@ -320,7 +322,7 @@ void open_file::download_range(std::size_t start_chunk, std::size_t end_chunk, } auto open_file::get_read_state() const -> boost::dynamic_bitset<> { - recur_mutex_lock file_lock(rw_mtx_); + recur_mutex_lock file_lock(file_mtx_); return read_state_; } @@ -328,9 +330,10 @@ auto open_file::get_read_state(std::size_t chunk) const -> bool { return get_read_state()[chunk]; } -auto open_file::is_complete() const -> bool { - recur_mutex_lock file_lock(rw_mtx_); - return read_state_.all(); +auto open_file::is_complete() const -> bool { return get_read_state().all(); } + +auto open_file::is_download_complete() const -> bool { + return get_read_state().all(); } auto open_file::native_operation( @@ -339,7 +342,7 @@ auto open_file::native_operation( return api_error::download_stopped; } - unique_recur_mutex_lock file_lock(rw_mtx_); + unique_recur_mutex_lock rw_lock(rw_mtx_); return do_io([&]() -> api_error { return callback(nf_->get_handle()); }); } @@ -363,18 +366,20 @@ auto open_file::native_operation( new_file_size, chunk_size_)) - 1U; - unique_recur_mutex_lock file_lock(rw_mtx_); - if (not is_empty_file && (last_chunk < read_state_.size())) { - file_lock.unlock(); + unique_recur_mutex_lock rw_lock(rw_mtx_); + auto read_state = get_read_state(); + if (not is_empty_file && (last_chunk < read_state.size())) { + rw_lock.unlock(); update_background_reader(0U); download_chunk(last_chunk, false, true); if (get_api_error() != api_error::success) { return get_api_error(); } - file_lock.lock(); + rw_lock.lock(); } + read_state = get_read_state(); auto original_file_size = get_file_size(); auto res = do_io([&]() -> api_error { return callback(nf_->get_handle()); }); @@ -397,15 +402,16 @@ auto open_file::native_operation( } } - if (is_empty_file || (read_state_.size() != (last_chunk + 1U))) { - auto old_size = read_state_.size(); - read_state_.resize(is_empty_file ? 0U : last_chunk + 1U); + if (is_empty_file || (read_state.size() != (last_chunk + 1U))) { + auto old_size = read_state.size(); + read_state.resize(is_empty_file ? 0U : last_chunk + 1U); if (not is_empty_file) { for (std::size_t chunk = old_size; chunk <= last_chunk; ++chunk) { - read_state_.set(chunk); + read_state.set(chunk); } } + set_read_state(read_state); last_chunk_size_ = static_cast( new_file_size <= chunk_size_ ? new_file_size @@ -463,7 +469,7 @@ auto open_file::read(std::size_t read_size, std::uint64_t read_offset, }); }; - if (read_state_.all()) { + if (get_read_state().all()) { reset_timeout(); return read_from_source(); } @@ -479,7 +485,7 @@ auto open_file::read(std::size_t read_size, std::uint64_t read_offset, return get_api_error(); } - unique_recur_mutex_lock file_lock(rw_mtx_); + unique_recur_mutex_lock rw_lock(rw_mtx_); return get_api_error() == api_error::success ? read_from_source() : get_api_error(); } @@ -487,8 +493,8 @@ auto open_file::read(std::size_t read_size, std::uint64_t read_offset, void open_file::remove(std::uint64_t handle) { open_file_base::remove(handle); - recur_mutex_lock file_lock(rw_mtx_); - if (modified_ && read_state_.all() && + recur_mutex_lock rw_lock(rw_mtx_); + if (modified_ && get_read_state().all() && (get_api_error() == api_error::success)) { mgr_.queue_upload(*this); modified_ = false; @@ -502,7 +508,7 @@ void open_file::remove(std::uint64_t handle) { void open_file::remove_all() { open_file_base::remove_all(); - recur_mutex_lock file_lock(rw_mtx_); + recur_mutex_lock rw_lock(rw_mtx_); modified_ = false; removed_ = true; @@ -552,8 +558,18 @@ void open_file::set_modified() { } } +void open_file::set_read_state(std::size_t chunk) { + recur_mutex_lock file_lock(file_mtx_); + read_state_.set(chunk); +} + +void open_file::set_read_state(boost::dynamic_bitset<> read_state) { + recur_mutex_lock file_lock(file_mtx_); + read_state_ = std::move(read_state); +} + void open_file::update_background_reader(std::size_t read_chunk) { - recur_mutex_lock file_lock(rw_mtx_); + recur_mutex_lock rw_lock(rw_mtx_); read_chunk_ = read_chunk; if (reader_thread_ || stop_requested_) { @@ -564,7 +580,8 @@ void open_file::update_background_reader(std::size_t read_chunk) { std::size_t next_chunk{}; while (not stop_requested_) { unique_recur_mutex_lock lock(rw_mtx_); - if ((fsi_.size == 0U) || read_state_.all()) { + auto read_state = get_read_state(); + if ((fsi_.size == 0U) || read_state.all()) { lock.unlock(); unique_mutex_lock io_lock(io_thread_mtx_); @@ -578,11 +595,12 @@ void open_file::update_background_reader(std::size_t read_chunk) { do { next_chunk = read_chunk_ = - ((read_chunk_ + 1U) >= read_state_.size()) ? 0U : read_chunk_ + 1U; + ((read_chunk_ + 1U) >= read_state.size()) ? 0U : read_chunk_ + 1U; } while ((next_chunk != 0U) && (active_downloads_.find(next_chunk) != active_downloads_.end())); lock.unlock(); + download_chunk(next_chunk, true, false); } }); @@ -612,13 +630,13 @@ auto open_file::write(std::uint64_t write_offset, const data_buffer &data, update_background_reader(start_chunk); - download_range(start_chunk, std::min(read_state_.size() - 1U, end_chunk), + download_range(start_chunk, std::min(get_read_state().size() - 1U, end_chunk), true); if (get_api_error() != api_error::success) { return get_api_error(); } - unique_recur_mutex_lock file_lock(rw_mtx_); + unique_recur_mutex_lock rw_lock(rw_mtx_); if ((write_offset + data.size()) > fsi_.size) { auto res = resize(write_offset + data.size()); if (res != api_error::success) { diff --git a/repertory/librepertory/src/file_manager/upload.cpp b/repertory/librepertory/src/file_manager/upload.cpp index 86107a9c..78a7e06c 100644 --- a/repertory/librepertory/src/file_manager/upload.cpp +++ b/repertory/librepertory/src/file_manager/upload.cpp @@ -53,7 +53,8 @@ void upload::upload_thread() { error_ = provider_.upload_file(fsi_.api_path, fsi_.source_path, stop_requested_); - if (not utils::file::reset_modified_time(fsi_.source_path)) { + if (error_ == api_error::success && + not utils::file::reset_modified_time(fsi_.source_path)) { utils::error::raise_api_path_error( function_name, fsi_.api_path, fsi_.source_path, utils::get_last_error_code(), "failed to reset modified time"); diff --git a/repertory/librepertory/src/providers/base_provider.cpp b/repertory/librepertory/src/providers/base_provider.cpp index a991095d..59816c51 100644 --- a/repertory/librepertory/src/providers/base_provider.cpp +++ b/repertory/librepertory/src/providers/base_provider.cpp @@ -51,8 +51,8 @@ void base_provider::add_all_items(const stop_type &stop_requested) { } auto base_provider::create_api_file(std::string path, std::string key, - std::uint64_t size, - std::uint64_t file_time) -> api_file { + std::uint64_t size, std::uint64_t file_time) + -> api_file { api_file file{}; file.api_path = utils::path::create_api_path(path); file.api_parent = utils::path::get_parent_api_path(file.api_path); @@ -84,8 +84,8 @@ auto base_provider::create_api_file(std::string path, std::uint64_t size, } auto base_provider::create_directory_clone_source_meta( - const std::string &source_api_path, - const std::string &api_path) -> api_error { + const std::string &source_api_path, const std::string &api_path) + -> api_error { REPERTORY_USES_FUNCTION_NAME(); bool exists{}; @@ -182,8 +182,8 @@ auto base_provider::create_directory(const std::string &api_path, return set_item_meta(api_path, meta); } -auto base_provider::create_file(const std::string &api_path, - api_meta_map &meta) -> api_error { +auto base_provider::create_file(const std::string &api_path, api_meta_map &meta) + -> api_error { REPERTORY_USES_FUNCTION_NAME(); bool exists{}; @@ -240,8 +240,9 @@ auto base_provider::create_file(const std::string &api_path, return api_error::error; } -auto base_provider::get_api_path_from_source( - const std::string &source_path, std::string &api_path) const -> api_error { +auto base_provider::get_api_path_from_source(const std::string &source_path, + std::string &api_path) const + -> api_error { REPERTORY_USES_FUNCTION_NAME(); if (source_path.empty()) { @@ -254,8 +255,9 @@ auto base_provider::get_api_path_from_source( return db3_->get_api_path(source_path, api_path); } -auto base_provider::get_directory_items( - const std::string &api_path, directory_item_list &list) const -> api_error { +auto base_provider::get_directory_items(const std::string &api_path, + directory_item_list &list) const + -> api_error { REPERTORY_USES_FUNCTION_NAME(); bool exists{}; @@ -319,9 +321,10 @@ auto base_provider::get_file_size(const std::string &api_path, return api_error::success; } -auto base_provider::get_filesystem_item( - const std::string &api_path, bool directory, - filesystem_item &fsi) const -> api_error { +auto base_provider::get_filesystem_item(const std::string &api_path, + bool directory, + filesystem_item &fsi) const + -> api_error { bool exists{}; auto res = is_directory(api_path, exists); if (res != api_error::success) { @@ -354,9 +357,10 @@ auto base_provider::get_filesystem_item( return api_error::success; } -auto base_provider::get_filesystem_item_and_file( - const std::string &api_path, api_file &file, - filesystem_item &fsi) const -> api_error { +auto base_provider::get_filesystem_item_and_file(const std::string &api_path, + api_file &file, + filesystem_item &fsi) const + -> api_error { auto res = get_file(api_path, file); if (res != api_error::success) { return res;