core: Add support for retrieving CURL error messages, handle unexpect…

…ed CURL return code on macOS, and log such codes in tests (fixes y-scope#519). (y-scope#517)

Co-authored-by: kirkrodrigues <[email protected]>

components/core/src/clp/CurlDownloadHandler.cpp

@@ -2,13 +2,16 @@
 
 #include <chrono>
 #include <cstddef>
+#include <memory>
 #include <string>
 #include <string_view>
+#include <utility>
 
 #include <curl/curl.h>
 
 namespace clp {
 CurlDownloadHandler::CurlDownloadHandler(
+        std::shared_ptr<ErrorMsgBuf> error_msg_buf,
         ProgressCallback progress_callback,
         WriteCallback write_callback,
         void* arg,
@@ -17,7 +20,17 @@ CurlDownloadHandler::CurlDownloadHandler(
         bool disable_caching,
         std::chrono::seconds connection_timeout,
         std::chrono::seconds overall_timeout
-) {
+)
+        : m_error_msg_buf{std::move(error_msg_buf)} {
+    if (nullptr != m_error_msg_buf) {
+        // Set up error message buffer
+        // According to the docs (https://curl.se/libcurl/c/CURLOPT_ERRORBUFFER.html), since 7.60.0,
+        // a successful call to set `CURLOPT_ERRORBUFFER` will initialize the buffer to an empty
+        // string 7.60.0. Since we require at least 7.68.0, we don't need to clear the provided
+        // buffer before it's used.
+        m_easy_handle.set_option(CURLOPT_ERRORBUFFER, m_error_msg_buf->data());
+    }
+
     // Set up src url
     m_easy_handle.set_option(CURLOPT_URL, src_url.data());
 
@@ -46,5 +59,8 @@ CurlDownloadHandler::CurlDownloadHandler(
     if (false == m_http_headers.is_empty()) {
         m_easy_handle.set_option(CURLOPT_HTTPHEADER, m_http_headers.get_raw_list());
     }
+
+    // Set up failure on HTTP error reponse
+    m_easy_handle.set_option(CURLOPT_FAILONERROR, static_cast<long>(true));
 }
 }  // namespace clp

components/core/src/clp/CurlDownloadHandler.hpp

@@ -1,8 +1,10 @@
 #ifndef CLP_CURLDOWNLOADHANDLER_HPP
 #define CLP_CURLDOWNLOADHANDLER_HPP
 
+#include <array>
 #include <chrono>
 #include <cstddef>
+#include <memory>
 #include <string_view>
 
 #include <curl/curl.h>
@@ -18,6 +20,7 @@ namespace clp {
 class CurlDownloadHandler {
 public:
     // Types
+    using ErrorMsgBuf = std::array<char, CURL_ERROR_SIZE>;
     /**
      * libcurl progress callback. This method must have C linkage. Doc:
      * https://curl.se/libcurl/c/CURLOPT_WRITEFUNCTION.html
@@ -37,6 +40,9 @@ class CurlDownloadHandler {
 
     // Constructor
     /**
+     * @param error_msg_buf The buffer to store the CURL error message or `nullptr` if it shouldn't
+     * be stored.
+     * Doc: https://curl.se/libcurl/c/CURLOPT_ERRORBUFFER.html
      * @param progress_callback
      * @param write_callback
      * @param arg Argument to pass to `progress_callback` and `write_callback`
@@ -49,6 +55,7 @@ class CurlDownloadHandler {
      * `connection_timeout`. Doc: https://curl.se/libcurl/c/CURLOPT_TIMEOUT.html
      */
     explicit CurlDownloadHandler(
+            std::shared_ptr<ErrorMsgBuf> error_msg_buf,
             ProgressCallback progress_callback,
             WriteCallback write_callback,
             void* arg,
@@ -78,6 +85,7 @@ class CurlDownloadHandler {
 private:
     CurlEasyHandle m_easy_handle;
     CurlStringList m_http_headers;
+    std::shared_ptr<ErrorMsgBuf> m_error_msg_buf;
 };
 }  // namespace clp
 

components/core/src/clp/NetworkReader.cpp

@@ -13,6 +13,7 @@
 #include "CurlDownloadHandler.hpp"
 #include "CurlOperationFailed.hpp"
 #include "ErrorCode.hpp"
+#include "Platform.hpp"
 
 namespace clp {
 /**
@@ -158,6 +159,13 @@ auto NetworkReader::try_get_pos(size_t& pos) -> ErrorCode {
                 // offset specified in the HTTP header.
                 return ErrorCode_Failure;
             }
+            if constexpr (Platform::MacOs == cCurrentPlatform) {
+                // On macOS, HTTP response code 416 is not handled as `CURL_HTTP_RETURNED_ERROR` in
+                // some `libcurl` versions.
+                if (CURLE_RECV_ERROR == curl_return_code.value()) {
+                    return ErrorCode_Failure;
+                }
+            }
         }
 
         if (false == at_least_one_byte_downloaded()) {
@@ -200,6 +208,7 @@ auto NetworkReader::buffer_downloaded_data(NetworkReader::BufferView data) -> si
 auto NetworkReader::DownloaderThread::thread_method() -> void {
     try {
         CurlDownloadHandler curl_handler{
+                m_reader.m_curl_error_msg_buf,
                 curl_progress_callback,
                 curl_write_callback,
                 static_cast<void*>(&m_reader),

components/core/src/clp/NetworkReader.hpp

@@ -216,6 +216,19 @@ class NetworkReader : public ReaderInterface {
         return m_curl_ret_code.load();
     }
 
+    /**
+     * @return The error message set by the underlying CURL handler.
+     * @return std::nullopt if the download is still in-progress or no error has occured.
+     */
+    [[nodiscard]] auto get_curl_error_msg() const -> std::optional<std::string_view> {
+        if (auto const ret_code{get_curl_ret_code()};
+            false == ret_code.has_value() || CURLE_OK == ret_code.value())
+        {
+            return std::nullopt;
+        }
+        return std::string_view{m_curl_error_msg_buf->data()};
+    }
+
 private:
     /**
      * This class implements clp::Thread to download data using CURL.
@@ -334,6 +347,10 @@ class NetworkReader : public ReaderInterface {
     // These two members should only be set from `set_download_completion_status`
     std::atomic<State> m_state{State::InProgress};
     std::atomic<std::optional<CURLcode>> m_curl_ret_code;
+
+    std::shared_ptr<CurlDownloadHandler::ErrorMsgBuf> m_curl_error_msg_buf{
+            std::make_shared<CurlDownloadHandler::ErrorMsgBuf>()
+    };
 };
 }  // namespace clp
 

components/core/tests/test-NetworkReader.cpp

@@ -16,6 +16,7 @@
 #include "../src/clp/ErrorCode.hpp"
 #include "../src/clp/FileReader.hpp"
 #include "../src/clp/NetworkReader.hpp"
+#include "../src/clp/Platform.hpp"
 #include "../src/clp/ReaderInterface.hpp"
 
 namespace {
@@ -35,6 +36,16 @@ constexpr size_t cDefaultReaderBufferSize{1024};
 auto get_content(clp::ReaderInterface& reader, size_t read_buf_size = cDefaultReaderBufferSize)
         -> std::vector<char>;
 
+/**
+ * Asserts whether the given `CURLcode` and the CURL return code stored in the given `NetworkReader`
+ * instance are the same, and prints a log message if not.
+ * @param expected
+ * @param reader
+ * @return Whether the the assertion succeeded.
+ */
+[[nodiscard]] auto
+assert_curl_error_code(CURLcode expected, clp::NetworkReader const& reader) -> bool;
+
 auto get_test_input_local_path() -> std::string {
     std::filesystem::path const current_file_path{__FILE__};
     auto const tests_dir{current_file_path.parent_path()};
@@ -64,6 +75,28 @@ auto get_content(clp::ReaderInterface& reader, size_t read_buf_size) -> std::vec
     }
     return buf;
 }
+
+auto assert_curl_error_code(CURLcode expected, clp::NetworkReader const& reader) -> bool {
+    auto const ret_code{reader.get_curl_ret_code()};
+    if (false == ret_code.has_value()) {
+        WARN("The CURL error code hasn't been set yet in the given reader.");
+        return false;
+    }
+    auto const actual{ret_code.value()};
+    if (expected == actual) {
+        return true;
+    }
+    std::string message_to_log{
+            "Unexpected CURL error code: " + std::to_string(actual)
+            + "; expected: " + std::to_string(expected)
+    };
+    auto const curl_error_message{reader.get_curl_error_msg()};
+    if (curl_error_message.has_value()) {
+        message_to_log += "\nError message:\n" + std::string{curl_error_message.value()};
+    }
+    WARN(message_to_log);
+    return false;
+}
 }  // namespace
 
 TEST_CASE("network_reader_basic", "[NetworkReader]") {
@@ -73,10 +106,7 @@ TEST_CASE("network_reader_basic", "[NetworkReader]") {
     clp::CurlGlobalInstance const curl_global_instance;
     clp::NetworkReader reader{get_test_input_remote_url()};
     auto const actual{get_content(reader)};
-    auto const ret_code{reader.get_curl_ret_code()};
-    REQUIRE(ret_code.has_value());
-    // NOLINTNEXTLINE(bugprone-unchecked-optional-access)
-    REQUIRE((CURLE_OK == ret_code.value()));
+    REQUIRE(assert_curl_error_code(CURLE_OK, reader));
     REQUIRE((actual == expected));
 }
 
@@ -92,10 +122,7 @@ TEST_CASE("network_reader_with_offset_and_seek", "[NetworkReader]") {
         clp::CurlGlobalInstance const curl_global_instance;
         clp::NetworkReader reader{get_test_input_remote_url(), cOffset};
         auto const actual{get_content(reader)};
-        auto const ret_code{reader.get_curl_ret_code()};
-        REQUIRE(ret_code.has_value());
-        // NOLINTNEXTLINE(bugprone-unchecked-optional-access)
-        REQUIRE((CURLE_OK == ret_code.value()));
+        REQUIRE(assert_curl_error_code(CURLE_OK, reader));
         REQUIRE((reader.get_pos() == ref_end_pos));
         REQUIRE((actual == expected));
     }
@@ -106,10 +133,7 @@ TEST_CASE("network_reader_with_offset_and_seek", "[NetworkReader]") {
         clp::NetworkReader reader(get_test_input_remote_url());
         reader.seek_from_begin(cOffset);
         auto const actual{get_content(reader)};
-        auto const ret_code{reader.get_curl_ret_code()};
-        REQUIRE(ret_code.has_value());
-        // NOLINTNEXTLINE(bugprone-unchecked-optional-access)
-        REQUIRE((CURLE_OK == ret_code.value()));
+        REQUIRE(assert_curl_error_code(CURLE_OK, reader));
         REQUIRE((reader.get_pos() == ref_end_pos));
         REQUIRE((actual == expected));
     }
@@ -147,13 +171,20 @@ TEST_CASE("network_reader_illegal_offset", "[NetworkReader]") {
     constexpr size_t cIllegalOffset{UINT32_MAX};
     clp::CurlGlobalInstance const curl_global_instance;
     clp::NetworkReader reader{get_test_input_remote_url(), cIllegalOffset};
-    while (true) {
-        auto const ret_code{reader.get_curl_ret_code()};
-        if (ret_code.has_value()) {
-            REQUIRE((CURLE_HTTP_RETURNED_ERROR == ret_code.value()));
-            size_t pos{};
-            REQUIRE((clp::ErrorCode_Failure == reader.try_get_pos(pos)));
-            break;
-        }
+    while (false == reader.get_curl_ret_code().has_value()) {
+        // Wait until the return code is ready
+    }
+
+    if constexpr (clp::Platform::MacOs == clp::cCurrentPlatform) {
+        // On macOS, HTTP response code 416 is not handled as `CURL_HTTP_RETURNED_ERROR` in some
+        // `libcurl` versions.
+        REQUIRE(
+                (assert_curl_error_code(CURLE_HTTP_RETURNED_ERROR, reader)
+                 || assert_curl_error_code(CURLE_RECV_ERROR, reader))
+        );
+    } else {
+        REQUIRE(assert_curl_error_code(CURLE_HTTP_RETURNED_ERROR, reader));
     }
+    size_t pos{};
+    REQUIRE((clp::ErrorCode_Failure == reader.try_get_pos(pos)));
 }