Merge pull request #1254 from janhq/j/replace-result-for-exception

chore: replace result api instead of throwing exception

Author: namchuai

engine/commands/engine_install_cmd.cc

@@ -1,18 +1,16 @@
 #include "engine_install_cmd.h"
-// clang-format off
-#include "utils/cortexso_parser.h" 
-#include "utils/archive_utils.h"
-// clang-format on
-#include "utils/cuda_toolkit_utils.h"
-#include "utils/engine_matcher_utils.h"
 #include "utils/logging_utils.h"
 
 namespace commands {
 
 void EngineInstallCmd::Exec(const std::string& engine,
                             const std::string& version,
                             const std::string& src) {
-  engine_service_.InstallEngine(engine, version, src);
-  CLI_LOG("Engine " << engine << " installed successfully!");
+  auto result = engine_service_.InstallEngine(engine, version, src);
+  if (result.has_error()) {
+    CLI_LOG(result.error());
+  } else {
+    CLI_LOG("Engine " << engine << " installed successfully!");
+  }
 }
 };  // namespace commands

engine/commands/engine_uninstall_cmd.cc

@@ -5,7 +5,12 @@
 namespace commands {
 
 void EngineUninstallCmd::Exec(const std::string& engine) {
-  engine_service_.UninstallEngine(engine);
-  CLI_LOG("Engine " << engine << " uninstalled successfully!");
+  auto result = engine_service_.UninstallEngine(engine);
+
+  if (result.has_error()) {
+    CLI_LOG(result.error());
+  } else {
+    CLI_LOG("Engine uninstalled successfully");
+  }
 }
 };  // namespace commands

engine/commands/run_cmd.cc

@@ -1,13 +1,12 @@
 #include "run_cmd.h"
 #include "chat_cmd.h"
-#include "cmd_info.h"
 #include "config/yaml_config.h"
 #include "model_start_cmd.h"
 #include "model_status_cmd.h"
 #include "server_start_cmd.h"
-#include "utils/cortex_utils.h"
-#include "utils/file_manager_utils.h"
+#include "utils/logging_utils.h"
 #include "utils/modellist_utils.h"
+
 namespace commands {
 
 void RunCmd::Exec() {
@@ -45,7 +44,10 @@ void RunCmd::Exec() {
         throw std::runtime_error("Engine " + mc.engine + " is incompatible");
       }
       if (required_engine.value().status == EngineService::kNotInstalled) {
-        engine_service_.InstallEngine(mc.engine);
+        auto install_engine_result = engine_service_.InstallEngine(mc.engine);
+        if (install_engine_result.has_error()) {
+          throw std::runtime_error(install_engine_result.error());
+        }
       }
     }
 

engine/exceptions/failed_curl_exception.h

@@ -4,13 +4,13 @@
 #include <stdio.h>
 #include <trantor/utils/Logger.h>
 #include <filesystem>
+#include <optional>
 #include <ostream>
 #include <thread>
-#include "exceptions/failed_curl_exception.h"
-#include "exceptions/failed_init_curl_exception.h"
-#include "exceptions/failed_open_file_exception.h"
+#include "download_service.h"
 #include "utils/format_utils.h"
 #include "utils/logging_utils.h"
+#include "utils/result.hpp"
 
 #ifdef _WIN32
 #define ftell64(f) _ftelli64(f)
@@ -27,47 +27,58 @@ size_t WriteCallback(void* ptr, size_t size, size_t nmemb, FILE* stream) {
 }
 }  // namespace
 
-void DownloadService::AddDownloadTask(
+cpp::result<void, std::string> DownloadService::AddDownloadTask(
     DownloadTask& task, std::optional<OnDownloadTaskSuccessfully> callback) {
   CLI_LOG("Validating download items, please wait..");
   // preprocess to check if all the item are valid
   auto total_download_size{0};
+  std::optional<std::string> err_msg = std::nullopt;
   for (auto& item : task.items) {
-    try {
-      auto size = GetFileSize(item.downloadUrl);
-      item.bytes = size;
-      total_download_size += size;
-    } catch (const FailedCurlException& e) {
-      CTL_ERR("Found invalid download item: " << item.downloadUrl << " - "
-                                              << e.what());
-      throw;
+    auto file_size = GetFileSize(item.downloadUrl);
+    if (file_size.has_error()) {
+      err_msg = file_size.error();
+      break;
     }
+
+    item.bytes = file_size.value();
+    total_download_size += file_size.value();
+  }
+
+  if (err_msg.has_value()) {
+    CTL_ERR(err_msg.value());
+    return cpp::fail(err_msg.value());
   }
 
   // all items are valid, start downloading
-  bool download_successfully = true;
+  // if any item from the task failed to download, the whole task will be
+  // considered failed
+  std::optional<std::string> dl_err_msg = std::nullopt;
   for (const auto& item : task.items) {
     CLI_LOG("Start downloading: " + item.localPath.filename().string());
-    try {
-      Download(task.id, item, true);
-    } catch (const std::runtime_error& e) {
-      CTL_ERR("Failed to download: " << item.downloadUrl << " - " << e.what());
-      download_successfully = false;
+    auto result = Download(task.id, item, true);
+    if (result.has_error()) {
+      dl_err_msg = result.error();
       break;
     }
   }
+  if (dl_err_msg.has_value()) {
+    CTL_ERR(dl_err_msg.value());
+    return cpp::fail(dl_err_msg.value());
+  }
 
-  if (download_successfully && callback.has_value()) {
+  if (callback.has_value()) {
     callback.value()(task);
   }
+  return {};
 }
 
-uint64_t DownloadService::GetFileSize(const std::string& url) const {
+cpp::result<uint64_t, std::string> DownloadService::GetFileSize(
+    const std::string& url) const noexcept {
   CURL* curl;
   curl = curl_easy_init();
 
   if (!curl) {
-    throw FailedInitCurlException();
+    return cpp::fail(static_cast<std::string>("Failed to init CURL"));
   }
 
   curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1L);
@@ -76,9 +87,8 @@ uint64_t DownloadService::GetFileSize(const std::string& url) const {
   CURLcode res = curl_easy_perform(curl);
 
   if (res != CURLE_OK) {
-    // if we have a failed here. it meant the url is invalid
-    throw FailedCurlException("CURL failed: " +
-                              std::string(curl_easy_strerror(res)));
+    return cpp::fail(static_cast<std::string>(
+        "CURL failed: " + std::string(curl_easy_strerror(res))));
   }
 
   curl_off_t content_length = 0;
@@ -90,19 +100,17 @@ uint64_t DownloadService::GetFileSize(const std::string& url) const {
 void DownloadService::AddAsyncDownloadTask(
     const DownloadTask& task,
     std::optional<OnDownloadTaskSuccessfully> callback) {
-
+  // just start one thread and handle all the download items
   for (const auto& item : task.items) {
     std::thread([this, task, &callback, item]() {
       this->Download(task.id, item, false);
     }).detach();
   }
-
-  // TODO: how to call the callback when all the download has finished?
 }
 
-void DownloadService::Download(const std::string& download_id,
-                               const DownloadItem& download_item,
-                               bool allow_resume) {
+cpp::result<void, std::string> DownloadService::Download(
+    const std::string& download_id, const DownloadItem& download_item,
+    bool allow_resume) noexcept {
   CTL_INF("Absolute file output: " << download_item.localPath.string());
 
   CURL* curl;
@@ -111,7 +119,7 @@ void DownloadService::Download(const std::string& download_id,
 
   curl = curl_easy_init();
   if (!curl) {
-    throw FailedInitCurlException();
+    return cpp::fail(static_cast<std::string>("Failed to init CURL"));
   }
 
   std::string mode = "wb";
@@ -121,45 +129,44 @@ void DownloadService::Download(const std::string& download_id,
     if (existing_file_size == -1) {
       CLI_LOG("Cannot get file size: " << download_item.localPath.string()
                                        << " . Start download over!");
-      return;
-    }
-    CTL_INF("Existing file size: " << download_item.downloadUrl << " - "
-                                   << download_item.localPath.string() << " - "
-                                   << existing_file_size);
-    auto missing_bytes = download_item.bytes.value() - existing_file_size;
-    if (missing_bytes > 0) {
-      CLI_LOG("Found unfinished download! Additional "
-              << format_utils::BytesToHumanReadable(missing_bytes)
-              << " need to be downloaded.");
-      std::cout << "Continue download [Y/n]: " << std::flush;
-      std::string answer{""};
-      std::getline(std::cin, answer);
-      if (answer == "Y" || answer == "y" || answer.empty()) {
-        mode = "ab";
-        CLI_LOG("Resuming download..");
-      } else {
-        CLI_LOG("Start over..");
-      }
     } else {
-      CLI_LOG(download_item.localPath.filename().string()
-              << " is already downloaded!");
-      std::cout << "Re-download? [Y/n]: " << std::flush;
-
-      std::string answer = "";
-      std::getline(std::cin, answer);
-      if (answer == "Y" || answer == "y" || answer.empty()) {
-        CLI_LOG("Re-downloading..");
+      CTL_INF("Existing file size: " << download_item.downloadUrl << " - "
+                                     << download_item.localPath.string()
+                                     << " - " << existing_file_size);
+      auto missing_bytes = download_item.bytes.value() - existing_file_size;
+      if (missing_bytes > 0) {
+        CLI_LOG("Found unfinished download! Additional "
+                << format_utils::BytesToHumanReadable(missing_bytes)
+                << " need to be downloaded.");
+        std::cout << "Continue download [Y/n]: " << std::flush;
+        std::string answer{""};
+        std::getline(std::cin, answer);
+        if (answer == "Y" || answer == "y" || answer.empty()) {
+          mode = "ab";
+          CLI_LOG("Resuming download..");
+        } else {
+          CLI_LOG("Start over..");
+        }
       } else {
-        return;
+        CLI_LOG(download_item.localPath.filename().string()
+                << " is already downloaded!");
+        std::cout << "Re-download? [Y/n]: " << std::flush;
+
+        std::string answer = "";
+        std::getline(std::cin, answer);
+        if (answer == "Y" || answer == "y" || answer.empty()) {
+          CLI_LOG("Re-downloading..");
+        } else {
+          return {};
+        }
       }
     }
   }
 
   file = fopen(download_item.localPath.string().c_str(), mode.c_str());
   if (!file) {
-    auto err_msg{"Failed to open output file " +
-                 download_item.localPath.string()};
-    throw FailedOpenFileException(err_msg);
+    return cpp::fail("Failed to open output file " +
+                     download_item.localPath.string());
   }
 
   curl_easy_setopt(curl, CURLOPT_URL, download_item.downloadUrl.c_str());
@@ -181,14 +188,13 @@ void DownloadService::Download(const std::string& download_id,
   res = curl_easy_perform(curl);
 
   if (res != CURLE_OK) {
-    fprintf(stderr, "curl_easy_perform() failed: %s\n",
-            curl_easy_strerror(res));
-    throw std::runtime_error("Failed to download file " +
-                             download_item.localPath.filename().string());
+    return cpp::fail("Download failed! Error: " +
+                     static_cast<std::string>(curl_easy_strerror(res)));
   }
 
   fclose(file);
   curl_easy_cleanup(curl);
+  return {};
 }
 
 curl_off_t DownloadService::GetLocalFileSize(
@@ -205,4 +211,4 @@ curl_off_t DownloadService::GetLocalFileSize(
   curl_off_t file_size = ftell64(file);
   fclose(file);
   return file_size;
-}
+}

engine/exceptions/failed_init_curl_exception.h

@@ -6,6 +6,7 @@
 #include <optional>
 #include <sstream>
 #include <vector>
+#include "utils/result.hpp"
 
 enum class DownloadType { Model, Engine, Miscellaneous, CudaToolkit, Cortex };
 
@@ -56,7 +57,7 @@ class DownloadService {
   using OnDownloadTaskSuccessfully =
       std::function<void(const DownloadTask& task)>;
 
-  void AddDownloadTask(
+  cpp::result<void, std::string> AddDownloadTask(
       DownloadTask& task,
       std::optional<OnDownloadTaskSuccessfully> callback = std::nullopt);
 
@@ -69,11 +70,13 @@ class DownloadService {
    *
    * @param url - url to get file size
    */
-  uint64_t GetFileSize(const std::string& url) const;
+  cpp::result<uint64_t, std::string> GetFileSize(
+      const std::string& url) const noexcept;
 
  private:
-  void Download(const std::string& download_id,
-                const DownloadItem& download_item, bool allow_resume);
+  cpp::result<void, std::string> Download(const std::string& download_id,
+                                          const DownloadItem& download_item,
+                                          bool allow_resume) noexcept;
 
   curl_off_t GetLocalFileSize(const std::filesystem::path& path) const;
 };