refactor

repertory/librepertory/include/file_manager/file_manager.hpp

@@ -68,7 +68,7 @@ private:
   std::unique_ptr<std::thread> upload_thread_;
 
 private:
-  void close_all(const std::string &api_path);
+  [[nodiscard]] auto close_all(const std::string &api_path) -> bool;
 
   void close_timed_out_files();
 
@@ -108,6 +108,11 @@ public:
   void remove_resume(const std::string &api_path,
                      const std::string &source_path) override;
 
+  static auto remove_source_and_shrink_cache(const std::string &api_path,
+                                             const std::string &source_path,
+                                             std::uint64_t file_size,
+                                             bool allocated) -> bool;
+
   void remove_upload(const std::string &api_path) override;
 
   void store_resume(const i_open_file &file) override;

repertory/librepertory/include/file_manager/i_open_file.hpp

@@ -40,25 +40,25 @@ public:
 
   [[nodiscard]] virtual auto get_filesystem_item() const -> filesystem_item = 0;
 
-  [[nodiscard]] virtual auto
+  [[nodiscard]] virtual auto get_open_data()
-  get_open_data() -> std::map<std::uint64_t, open_file_data> & = 0;
+      -> std::map<std::uint64_t, open_file_data> & = 0;
 
-  [[nodiscard]] virtual auto
+  [[nodiscard]] virtual auto get_open_data() const
-  get_open_data() const -> const std::map<std::uint64_t, open_file_data> & = 0;
+      -> const std::map<std::uint64_t, open_file_data> & = 0;
 
-  [[nodiscard]] virtual auto
+  [[nodiscard]] virtual auto get_open_data(std::uint64_t handle)
-  get_open_data(std::uint64_t handle) -> open_file_data & = 0;
+      -> open_file_data & = 0;
 
-  [[nodiscard]] virtual auto
+  [[nodiscard]] virtual auto get_open_data(std::uint64_t handle) const
-  get_open_data(std::uint64_t handle) const -> const open_file_data & = 0;
+      -> const open_file_data & = 0;
 
   [[nodiscard]] virtual auto get_open_file_count() const -> std::size_t = 0;
 
-  [[nodiscard]] virtual auto
+  [[nodiscard]] virtual auto get_read_state() const
-  get_read_state() const -> boost::dynamic_bitset<> = 0;
+      -> boost::dynamic_bitset<> = 0;
 
-  [[nodiscard]] virtual auto
+  [[nodiscard]] virtual auto get_read_state(std::size_t chunk) const
-  get_read_state(std::size_t chunk) const -> bool = 0;
+      -> bool = 0;
 
   [[nodiscard]] virtual auto get_source_path() const -> std::string = 0;
 
@@ -74,11 +74,11 @@ public:
                    native_operation_callback callback) -> api_error = 0;
 
   [[nodiscard]] virtual auto read(std::size_t read_size,
-                                  std::uint64_t read_offset,
+                                  std::uint64_t read_offset, data_buffer &data)
-                                  data_buffer &data) -> api_error = 0;
+      -> api_error = 0;
 
-  [[nodiscard]] virtual auto
+  [[nodiscard]] virtual auto resize(std::uint64_t new_file_size)
-  resize(std::uint64_t new_file_size) -> api_error = 0;
+      -> api_error = 0;
 
   virtual void set_api_path(const std::string &api_path) = 0;
 
@@ -93,12 +93,14 @@ class i_closeable_open_file : public i_open_file {
 public:
   virtual void add(std::uint64_t handle, open_file_data ofd) = 0;
 
+  [[nodiscard]] virtual auto get_allocated() const -> bool = 0;
+
   [[nodiscard]] virtual auto can_close() const -> bool = 0;
 
   virtual auto close() -> bool = 0;
 
-  [[nodiscard]] virtual auto
+  [[nodiscard]] virtual auto get_handles() const
-  get_handles() const -> std::vector<std::uint64_t> = 0;
+      -> std::vector<std::uint64_t> = 0;
 
   [[nodiscard]] virtual auto is_complete() const -> bool = 0;
 

repertory/librepertory/include/file_manager/open_file.hpp

@@ -98,6 +98,8 @@ protected:
 public:
   auto close() -> bool override;
 
+  [[nodiscard]] auto get_allocated() const -> bool override;
+
   [[nodiscard]] auto get_read_state() const -> boost::dynamic_bitset<> override;
 
   [[nodiscard]] auto get_read_state(std::size_t chunk) const -> bool override;

repertory/librepertory/include/file_manager/open_file_base.hpp

@@ -144,6 +144,8 @@ public:
 
   auto close() -> bool override;
 
+  [[nodiscard]] auto get_allocated() const -> bool override { return false; }
+
   [[nodiscard]] auto get_api_error() const -> api_error;
 
   [[nodiscard]] auto get_api_path() const -> std::string override;
@@ -158,17 +160,17 @@ public:
 
   [[nodiscard]] auto get_handles() const -> std::vector<std::uint64_t> override;
 
-  [[nodiscard]] auto
+  [[nodiscard]] auto get_open_data()
-  get_open_data() -> std::map<std::uint64_t, open_file_data> & override;
+      -> std::map<std::uint64_t, open_file_data> & override;
 
   [[nodiscard]] auto get_open_data() const
       -> const std::map<std::uint64_t, open_file_data> & override;
 
-  [[nodiscard]] auto
+  [[nodiscard]] auto get_open_data(std::uint64_t handle)
-  get_open_data(std::uint64_t handle) -> open_file_data & override;
+      -> open_file_data & override;
 
-  [[nodiscard]] auto
+  [[nodiscard]] auto get_open_data(std::uint64_t handle) const
-  get_open_data(std::uint64_t handle) const -> const open_file_data & override;
+      -> const open_file_data & override;
 
   [[nodiscard]] auto get_open_file_count() const -> std::size_t override;
 

repertory/librepertory/src/file_manager/file_manager.cpp

@@ -75,13 +75,13 @@ void file_manager::close(std::uint64_t handle) {
   closeable_file->remove(handle);
 }
 
-void file_manager::close_all(const std::string &api_path) {
+auto file_manager::close_all(const std::string &api_path) -> bool {
   REPERTORY_USES_FUNCTION_NAME();
 
   unique_recur_mutex_lock file_lock(open_file_mtx_);
   auto file_iter = open_file_lookup_.find(api_path);
   if (file_iter == open_file_lookup_.end()) {
-    return;
+    return false;
   }
 
   auto closeable_file = file_iter->second;
@@ -90,6 +90,8 @@ void file_manager::close_all(const std::string &api_path) {
 
   closeable_file->remove_all();
   closeable_file->close();
+
+  return closeable_file->get_allocated();
 }
 
 void file_manager::close_timed_out_files() {
@@ -184,20 +186,18 @@ auto file_manager::evict_file(const std::string &api_path) -> bool {
   if (open_file_lookup_.contains(api_path)) {
     closeable_file = open_file_lookup_.at(api_path);
   }
+
   open_file_lookup_.erase(api_path);
+
+  auto allocated = closeable_file ? closeable_file->get_allocated() : true;
+
+  auto removed = remove_source_and_shrink_cache(api_path, source_path, fsi.size,
+                                                allocated);
   open_lock.unlock();
 
   closeable_file.reset();
 
-  auto file = utils::file::file{source_path};
-  auto removed = file.remove();
   if (removed) {
-    res = cache_size_mgr::instance().shrink(fsi.size);
-    if (res != api_error::success) {
-      utils::error::raise_api_path_error(function_name, api_path, res,
-                                         "failed to shrink cache");
-    }
-
     event_system::instance().raise<filesystem_item_evicted>(api_path,
                                                             source_path);
   }
@@ -553,7 +553,7 @@ auto file_manager::remove_file(const std::string &api_path) -> api_error {
     return res;
   }
 
-  close_all(api_path);
+  auto allocated = close_all(api_path);
 
   unique_mutex_lock upload_lock(upload_mtx_);
   remove_upload(api_path, true);
@@ -568,20 +568,8 @@ auto file_manager::remove_file(const std::string &api_path) -> api_error {
     return res;
   }
 
-  auto file = utils::file::file{fsi.source_path};
+  remove_source_and_shrink_cache(api_path, fsi.source_path, fsi.size,
-  if (file.remove()) {
+                                 allocated);
-    res = cache_size_mgr::instance().shrink(fsi.size);
-    if (res != api_error::success) {
-      utils::error::raise_api_path_error(function_name, api_path, res,
-                                         "failed to shrink cache");
-    }
-    return api_error::success;
-  }
-
-  utils::error::raise_api_path_error(
-      function_name, fsi.api_path, fsi.source_path,
-      utils::get_last_error_code(), "failed to delete source");
-
   return api_error::success;
 }
 
@@ -611,6 +599,34 @@ void file_manager::remove_resume(const std::string &api_path,
   }
 }
 
+auto file_manager::remove_source_and_shrink_cache(
+    const std::string &api_path, const std::string &source_path,
+    std::uint64_t file_size, bool allocated) -> bool {
+  REPERTORY_USES_FUNCTION_NAME();
+
+  auto file = utils::file::file{source_path};
+
+  if (not file.remove()) {
+    utils::error::raise_api_path_error(function_name, api_path, source_path,
+                                       utils::get_last_error_code(),
+                                       "failed to delete source");
+    return false;
+  }
+
+  auto source_size = file.size().value_or(0U);
+  if (not allocated || source_size == 0U) {
+    return true;
+  }
+
+  auto res = cache_size_mgr::instance().shrink(file_size);
+  if (res != api_error::success) {
+    utils::error::raise_api_path_error(function_name, api_path, source_path,
+                                       res, "failed to shrink cache");
+  }
+
+  return true;
+}
+
 void file_manager::remove_upload(const std::string &api_path) {
   remove_upload(api_path, false);
 }

repertory/librepertory/src/file_manager/open_file.cpp

@@ -23,6 +23,7 @@
 
 #include "file_manager/cache_size_mgr.hpp"
 #include "file_manager/events.hpp"
+#include "file_manager/file_manager.hpp"
 #include "file_manager/i_upload_manager.hpp"
 #include "platform/platform.hpp"
 #include "providers/i_provider.hpp"
@@ -111,26 +112,32 @@ auto open_file::check_allocation() -> api_error {
     return api_error::success;
   }
 
-  allocated = true;
+  auto file_size = nf_->size().value_or(0U);
-  if (fsi_.size == 0U) {
+  if (file_size == fsi_.size) {
+    allocated = true;
     return api_error::success;
   }
 
-  auto file_size = nf_->size().value_or(0U);
+  file_lock.unlock();
-  if (file_size == fsi_.size) {
+  if (file_size > fsi_.size) {
-    return api_error::success;
+    auto res = cache_size_mgr::instance().shrink(file_size - fsi_.size);
+    if (res != api_error::success) {
+      return res;
+    }
+  } else if (file_size < fsi_.size) {
+    auto res = cache_size_mgr::instance().expand(fsi_.size - file_size);
+    if (res != api_error::success) {
+      return res;
+    }
   }
+  file_lock.lock();
 
   if (not nf_->truncate(fsi_.size)) {
     return api_error::os_error;
   }
-  file_lock.unlock();
 
-  if (file_size > fsi_.size) {
+  allocated = true;
-    return cache_size_mgr::instance().shrink(file_size - fsi_.size);
+  return api_error::success;
-  }
-
-  return cache_size_mgr::instance().expand(fsi_.size - file_size);
 }
 
 auto open_file::close() -> bool {
@@ -190,18 +197,16 @@ auto open_file::close() -> bool {
     return true;
   }
 
-  auto file = utils::file::file{fsi_.source_path};
+  if (allocated) {
-  if (file.remove()) {
+    file_manager::remove_source_and_shrink_cache(
-    auto res = cache_size_mgr::instance().shrink(fsi_.size);
+        fsi_.api_path, fsi_.source_path, fsi_.size, allocated);
-    if (res != api_error::success) {
-      utils::error::raise_api_path_error(function_name, fsi_.api_path,
-                                         fsi_.source_path, res,
-                                         "failed to shrink cache");
-    }
   } else {
-    utils::error::raise_api_path_error(
+    auto file = utils::file::file{fsi_.source_path};
-        function_name, fsi_.api_path, fsi_.source_path,
+    if (not file.remove()) {
-        utils::get_last_error_code(), "failed to delete source file");
+      utils::error::raise_api_path_error(
+          function_name, fsi_.api_path, fsi_.source_path,
+          utils::get_last_error_code(), "failed to delete source file");
+    }
   }
 
   auto parent = utils::path::get_parent_path(fsi_.source_path);
@@ -340,6 +345,11 @@ void open_file::download_range(std::size_t begin_chunk, std::size_t end_chunk,
   }
 }
 
+auto open_file::get_allocated() const -> bool {
+  recur_mutex_lock file_lock(file_mtx_);
+  return allocated;
+}
+
 auto open_file::get_read_state() const -> boost::dynamic_bitset<> {
   recur_mutex_lock file_lock(file_mtx_);
   return read_state_;

repertory/repertory_test/include/mocks/mock_open_file.hpp

@@ -47,6 +47,8 @@ public:
 
   MOCK_METHOD(boost::dynamic_bitset<>, get_read_state, (), (const, override));
 
+  MOCK_METHOD(bool, get_allocated, (), (const, override));
+
   MOCK_METHOD(bool, get_read_state, (std::size_t chunk), (const, override));
 
   MOCK_METHOD(std::string, get_source_path, (), (const, override));