core-clp: Add class to encapsulate libcurl's global resource manage…

…ment. (#461)

Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>

components/core/CMakeLists.txt

@@ -283,6 +283,8 @@ set(SOURCE_FILES_unitTest
         src/clp/CurlDownloadHandler.cpp
         src/clp/CurlDownloadHandler.hpp
         src/clp/CurlEasyHandle.hpp
+        src/clp/CurlGlobalInstance.cpp
+        src/clp/CurlGlobalInstance.hpp
         src/clp/CurlOperationFailed.hpp
         src/clp/CurlStringList.hpp
         src/clp/database_utils.cpp

components/core/src/clp/CurlGlobalInstance.cpp

@@ -0,0 +1,45 @@
+#include "CurlGlobalInstance.hpp"
+
+#include <mutex>
+
+#include <curl/curl.h>
+
+#include "CurlOperationFailed.hpp"
+#include "ErrorCode.hpp"
+
+namespace clp {
+CurlGlobalInstance::CurlGlobalInstance() {
+    std::scoped_lock const global_lock{m_ref_count_mutex};
+    if (0 == m_ref_count) {
+        if (auto const err{curl_global_init(CURL_GLOBAL_ALL)}; 0 != err) {
+            throw CurlOperationFailed(
+                    ErrorCode_Failure,
+                    __FILE__,
+                    __LINE__,
+                    err,
+                    "`curl_global_init` failed."
+            );
+        }
+    }
+    ++m_ref_count;
+}
+
+CurlGlobalInstance::~CurlGlobalInstance() {
+    std::scoped_lock const global_lock{m_ref_count_mutex};
+    --m_ref_count;
+    if (0 == m_ref_count) {
+#if defined(__APPLE__)
+        // NOTE: On macOS, calling `curl_global_init` after `curl_global_cleanup` will fail with
+        // CURLE_SSL_CONNECT_ERROR. Thus, for now, we skip `deinit` on macOS. Luckily, it is safe to
+        // call `curl_global_init` multiple times without calling `curl_global_cleanup`. Related
+        // issues:
+        // - https://github.com/curl/curl/issues/12525
+        // - https://github.com/curl/curl/issues/13805
+        // TODO: Remove this conditional logic when the issues are resolved.
+        return;
+#else
+        curl_global_cleanup();
+#endif
+    }
+}
+}  // namespace clp

components/core/src/clp/CurlGlobalInstance.hpp

@@ -0,0 +1,35 @@
+#ifndef CLP_CURLGLOBALINSTANCE_HPP
+#define CLP_CURLGLOBALINSTANCE_HPP
+
+#include <cstddef>
+#include <mutex>
+
+namespace clp {
+/**
+ * Class to wrap `libcurl`'s global initialization/de-initialization calls using RAII. Before using
+ * any `libcurl` functionalities, an instance of this class must be created. Although unnecessasry,
+ * it can be safely instantiated multiple times; it maintains a static reference count to all
+ * existing instances and only de-initializes `libcurl`'s global resources when the reference count
+ * reaches 0.
+ */
+class CurlGlobalInstance {
+public:
+    // Constructors
+    CurlGlobalInstance();
+
+    // Disable copy/move constructors and assignment operators
+    CurlGlobalInstance(CurlGlobalInstance const&) = delete;
+    CurlGlobalInstance(CurlGlobalInstance&&) = delete;
+    auto operator=(CurlGlobalInstance const&) -> CurlGlobalInstance& = delete;
+    auto operator=(CurlGlobalInstance&&) -> CurlGlobalInstance& = delete;
+
+    // Destructor
+    ~CurlGlobalInstance();
+
+private:
+    static inline std::mutex m_ref_count_mutex;
+    static inline size_t m_ref_count{0};
+};
+}  // namespace clp
+
+#endif  // CLP_CURLGLOBALINSTANCE_HPP

components/core/src/clp/NetworkReader.cpp

@@ -110,33 +110,6 @@ curl_write_callback(char* ptr, size_t size, size_t nmemb, void* reader_ptr) -> s
 }
 }  // namespace
 
-bool NetworkReader::m_static_init_complete{false};
-
-auto NetworkReader::init() -> ErrorCode {
-    if (m_static_init_complete) {
-        return ErrorCode_Success;
-    }
-    if (0 != curl_global_init(CURL_GLOBAL_ALL)) {
-        return ErrorCode_Failure;
-    }
-    m_static_init_complete = true;
-    return ErrorCode_Success;
-}
-
-auto NetworkReader::deinit() -> void {
-#if defined(__APPLE__)
-    // NOTE: On macOS, calling `curl_global_init` after `curl_global_cleanup` will fail with
-    // CURLE_SSL_CONNECT_ERROR. Thus, for now, we skip `deinit` on macOS. Related issues:
-    // - https://github.com/curl/curl/issues/12525
-    // - https://github.com/curl/curl/issues/13805
-    // TODO: Remove this conditional logic when the issues are resolved.
-    return;
-#else
-    curl_global_cleanup();
-    m_static_init_complete = false;
-#endif
-}
-
 NetworkReader::NetworkReader(
         std::string_view src_url,
         size_t offset,
@@ -157,9 +130,6 @@ NetworkReader::NetworkReader(
         // NOLINTNEXTLINE(cppcoreguidelines-avoid-c-arrays)
         m_buffer_pool.emplace_back(std::make_unique<char[]>(m_buffer_size));
     }
-    if (false == m_static_init_complete) {
-        throw OperationFailed(ErrorCode_NotReady, __FILE__, __LINE__);
-    }
     m_downloader_thread = std::make_unique<DownloaderThread>(*this, offset, disable_caching);
     m_downloader_thread->start();
 }

components/core/src/clp/NetworkReader.hpp

@@ -18,6 +18,7 @@
 #include <curl/curl.h>
 
 #include "CurlDownloadHandler.hpp"
+#include "CurlGlobalInstance.hpp"
 #include "ErrorCode.hpp"
 #include "ReaderInterface.hpp"
 #include "Thread.hpp"
@@ -69,24 +70,20 @@ class NetworkReader : public ReaderInterface {
     static constexpr size_t cMinBufferPoolSize{2};
     static constexpr size_t cMinBufferSize{512};
 
-    /**
-     * Initializes static resources for this class. This must be called before using the class.
-     * @return ErrorCode_Success on success.
-     * @return ErrorCode_Failure if libcurl initialization failed.
-     */
-    [[nodiscard]] static auto init() -> ErrorCode;
-
-    /**
-     * De-initializes any static resources.
-     */
-    static auto deinit() -> void;
-
     /**
      * Constructs a reader to stream data from the given URL, starting at the given offset.
-     * TODO: the current implementation doesn't handle the case when the given offset is out of
-     * range. The file_pos will be set to an invalid state if this happens, which can be
-     * problematic if the other part of the program depends on this position. It can be fixed by
-     * capturing the error code 416 in the response header.
+     * NOTE: This class depends on `libcurl`, so an instance of `clp::CurlGlobalInstance` must
+     * remain alive for the entire lifespan of any instance of this class.
+     *
+     * This class maintains an instance of `CurlGlobalInstance` in case the user forgets to
+     * instantiate one, but it is better for performance if the user instantiates one. For instance,
+     * if the user doesn't instantiate a `CurlGlobalInstance` and only ever creates one
+     * `NetworkReader` at a time, then every construction and destruction of `NetworkReader` would
+     * cause `libcurl` to init and deinit. In contrast, if the user instantiates a
+     * `CurlGlobalInstance` before instantiating any `NetworkReader`s, then the `CurlGlobalInstance`
+     * maintained by this class will simply be a reference to the existing one rather than
+     * initializing and deinitializing `libcurl`.
+     *
      * @param src_url
      * @param offset Index of the byte at which to start the download
      * @param disable_caching Whether to disable the caching.
@@ -246,8 +243,6 @@ class NetworkReader : public ReaderInterface {
         bool m_disable_caching{false};
     };
 
-    static bool m_static_init_complete;
-
     /**
      * Submits a request to abort the ongoing curl download session.
      */
@@ -308,6 +303,8 @@ class NetworkReader : public ReaderInterface {
         return m_at_least_one_byte_downloaded.load();
     }
 
+    CurlGlobalInstance m_curl_global_instance;
+
     std::string m_src_url;
 
     size_t m_offset{0};

components/core/tests/test-NetworkReader.cpp

@@ -12,6 +12,7 @@
 #include <curl/curl.h>
 
 #include "../src/clp/CurlDownloadHandler.hpp"
+#include "../src/clp/CurlGlobalInstance.hpp"
 #include "../src/clp/ErrorCode.hpp"
 #include "../src/clp/FileReader.hpp"
 #include "../src/clp/NetworkReader.hpp"
@@ -71,15 +72,14 @@ TEST_CASE("network_reader_basic", "[NetworkReader]") {
     auto const expected{get_content(ref_reader)};
     ref_reader.close();
 
-    REQUIRE((clp::ErrorCode_Success == clp::NetworkReader::init()));
+    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((actual == expected));
-    clp::NetworkReader::deinit();
 }
 
 TEST_CASE("network_reader_with_offset_and_seek", "[NetworkReader]") {
@@ -91,10 +91,9 @@ TEST_CASE("network_reader_with_offset_and_seek", "[NetworkReader]") {
     auto const ref_end_pos{ref_reader.get_pos()};
     ref_reader.close();
 
-    REQUIRE((clp::ErrorCode_Success == clp::NetworkReader::init()));
-
     // Read from an offset onwards by starting the download from that offset.
     {
+        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()};
@@ -107,6 +106,7 @@ TEST_CASE("network_reader_with_offset_and_seek", "[NetworkReader]") {
 
     // Read from an offset onwards by seeking to that offset.
     {
+        clp::CurlGlobalInstance const curl_global_instance;
         clp::NetworkReader reader(get_test_input_remote_url());
         reader.seek_from_begin(cOffset);
         auto const actual{get_content(reader)};
@@ -117,19 +117,16 @@ TEST_CASE("network_reader_with_offset_and_seek", "[NetworkReader]") {
         REQUIRE((reader.get_pos() == ref_end_pos));
         REQUIRE((actual == expected));
     }
-
-    clp::NetworkReader::deinit();
 }
 
 TEST_CASE("network_reader_destruct", "[NetworkReader]") {
-    REQUIRE((clp::ErrorCode_Success == clp::NetworkReader::init()));
-
     // We sleep to fill out all the buffers, and then we delete the reader. The destructor will try
     // to abort the underlying download and then destroy the instance. So should ensure destructor
     // is successfully executed without deadlock or exceptions.
     bool no_exception{true};
     try {
         // NOLINTBEGIN(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
+        clp::CurlGlobalInstance const curl_global_instance;
         auto reader{std::make_unique<clp::NetworkReader>(
                 get_test_input_remote_url(),
                 0,
@@ -147,15 +144,12 @@ TEST_CASE("network_reader_destruct", "[NetworkReader]") {
         no_exception = false;
     }
     REQUIRE(no_exception);
-
-    clp::NetworkReader::deinit();
 }
 
 TEST_CASE("network_reader_illegal_offset", "[NetworkReader]") {
-    REQUIRE((clp::ErrorCode_Success == clp::NetworkReader::init()));
-
     // Try to read from an out-of-bound offset.
     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()};
@@ -166,6 +160,4 @@ TEST_CASE("network_reader_illegal_offset", "[NetworkReader]") {
             break;
         }
     }
-
-    clp::NetworkReader::deinit();
 }