continue dead-lock fix

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<std::thread> reader_thread_;
   std::unique_ptr<std::thread> 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;

repertory/librepertory/src/drives/eviction.cpp

@@ -30,6 +30,7 @@
 #include "utils/file_utils.hpp"
 #include "utils/time.hpp"
 #include "utils/utils.hpp"
+#include <spdlog/fmt/bundled/base.h>
 
 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;
       }
 

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<i_open_file> &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<mutex_lock> lock;
+  std::unique_ptr<mutex_lock> upload_lock;
   if (not no_lock) {
-    lock = std::make_unique<mutex_lock>(upload_mtx_);
+    upload_lock = std::make_unique<mutex_lock>(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<mutex_lock> lock;
+  std::unique_ptr<mutex_lock> upload_lock;
   if (not no_lock) {
-    lock = std::make_unique<mutex_lock>(upload_mtx_);
+    upload_lock = std::make_unique<mutex_lock>(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<mutex_lock> lock;
+  std::unique_ptr<mutex_lock> upload_lock;
   if (not no_lock) {
-    lock = std::make_unique<mutex_lock>(upload_mtx_);
+    upload_lock = std::make_unique<mutex_lock>(upload_mtx_);
   }
 
   if (not mgr_db_->remove_upload(api_path)) {

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<download_begin>(fsi_.api_path,
                                                      fsi_.source_path);
     }
     event_system::instance().raise<download_chunk_begin>(
-        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<download>();
-    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<download_chunk_end>(
-            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<double>(read_state_.count()) /
-                           static_cast<double>(read_state_.size())) *
+          auto progress = (static_cast<double>(state.count()) /
+                           static_cast<double>(state.size())) *
                           100.0;
           event_system::instance().raise<download_progress>(
               fsi_.api_path, fsi_.source_path, progress);
-          if (read_state_.all() && not notified_) {
+          if (state.all() && not notified_) {
             notified_ = true;
             event_system::instance().raise<download_end>(
                 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<std::size_t>(
         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) {

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");

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;