diff --git a/components/core/src/clp/CurlDownloadHandler.cpp b/components/core/src/clp/CurlDownloadHandler.cpp index 8ec8fbfb6..9a2720083 100644 --- a/components/core/src/clp/CurlDownloadHandler.cpp +++ b/components/core/src/clp/CurlDownloadHandler.cpp @@ -2,13 +2,16 @@ #include #include +#include #include #include +#include #include namespace clp { CurlDownloadHandler::CurlDownloadHandler( + std::shared_ptr 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(true)); } } // namespace clp diff --git a/components/core/src/clp/CurlDownloadHandler.hpp b/components/core/src/clp/CurlDownloadHandler.hpp index b45f644ca..6421257ba 100644 --- a/components/core/src/clp/CurlDownloadHandler.hpp +++ b/components/core/src/clp/CurlDownloadHandler.hpp @@ -1,8 +1,10 @@ #ifndef CLP_CURLDOWNLOADHANDLER_HPP #define CLP_CURLDOWNLOADHANDLER_HPP +#include #include #include +#include #include #include @@ -18,6 +20,7 @@ namespace clp { class CurlDownloadHandler { public: // Types + using ErrorMsgBuf = std::array; /** * 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 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 m_error_msg_buf; }; } // namespace clp diff --git a/components/core/src/clp/NetworkReader.cpp b/components/core/src/clp/NetworkReader.cpp index c0b5359bf..706763362 100644 --- a/components/core/src/clp/NetworkReader.cpp +++ b/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(&m_reader), diff --git a/components/core/src/clp/NetworkReader.hpp b/components/core/src/clp/NetworkReader.hpp index 68e6d1019..ba581d067 100644 --- a/components/core/src/clp/NetworkReader.hpp +++ b/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 { + 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 m_state{State::InProgress}; std::atomic> m_curl_ret_code; + + std::shared_ptr m_curl_error_msg_buf{ + std::make_shared() + }; }; } // namespace clp diff --git a/components/core/tests/test-NetworkReader.cpp b/components/core/tests/test-NetworkReader.cpp index 38172507d..5a900ced3 100644 --- a/components/core/tests/test-NetworkReader.cpp +++ b/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; +/** + * 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))); }