Additional code cleanup.

Author: kinyoklion

libs/internal/tests/curl_requester_test.cpp

@@ -220,7 +220,7 @@ TEST_F(CurlRequesterTest, HandlesCustomHeaders) {
     server_->SetHandler(
         [](http::request<http::string_body> const& req)
             -> http::response<http::string_body> {
-            auto header_it = req.find("X-Custom-Header");
+            const auto header_it = req.find("X-Custom-Header");
             EXPECT_NE(req.end(), header_it);
             if (header_it != req.end()) {
                 EXPECT_EQ("custom-value", header_it->value());
@@ -275,7 +275,7 @@ TEST_F(CurlRequesterTest, Handles404Status) {
     });
 
     net::io_context client_ioc;
-    CurlRequester requester(
+    const CurlRequester requester(
         client_ioc.get_executor(),
         launchdarkly::config::shared::built::TlsOptions());
 
@@ -304,8 +304,8 @@ TEST_F(CurlRequesterTest, Handles404Status) {
 
 TEST_F(CurlRequesterTest, HandlesInvalidUrl) {
     net::io_context client_ioc;
-    CurlRequester requester(
-        client_ioc.get_executor(),
+    const CurlRequester requester(
+    client_ioc.get_executor(),
         launchdarkly::config::shared::built::TlsOptions());
 
     HttpRequest request("not a valid url", HttpMethod::kGet,

libs/networking/include/launchdarkly/network/curl_multi_manager.hpp

@@ -25,12 +25,6 @@ using SocketHandle = boost::asio::ip::tcp::socket;
  * multi interface with Boost.ASIO. Instead of blocking threads, CURL notifies
  * us via callbacks when sockets need attention, and we use ASIO to monitor
  * those sockets asynchronously.
- *
- * Key features:
- * - Non-blocking I/O using curl_multi_socket_action
- * - Cross-platform socket monitoring via ASIO tcp::socket
- * - Timer integration with ASIO steady_timer
- * - Thread-safe operation on ASIO executor
  */
 class CurlMultiManager : public std::enable_shared_from_this<CurlMultiManager> {
 public:
@@ -135,7 +129,6 @@ class CurlMultiManager : public std::enable_shared_from_this<CurlMultiManager> {
     };
 
     boost::asio::any_io_executor executor_;
-    // CURLM* multi_handle_;
     std::unique_ptr<CURLM, decltype(&curl_multi_cleanup)> multi_handle_;
     boost::asio::steady_timer timer_;
 

libs/server-sent-events/src/curl_client.cpp

@@ -346,11 +346,6 @@ curl_socket_t CurlClient::OpenSocketCallback(void* clientp,
     curl_socket_t sockfd = socket(address->family, address->socktype,
                                   address->protocol);
 
-    // Store it so we can close it during shutdown
-    if (sockfd != CURL_SOCKET_BAD) {
-        context->set_curl_socket(sockfd);
-    }
-
     return sockfd;
 }
 

libs/server-sent-events/src/curl_client.hpp

@@ -68,27 +68,26 @@ class CurlClient final : public Client,
     /**
     * The request context represents the state required by the executing CURL
     * request. Not directly including the shared data in the CurlClient allows
-    * for easy seperation of its lifetime from that of the CURL client. This
+    * for easy separation of its lifetime from that of the CURL client. This
     * facilitates destruction of the CurlClient being used to stop in-progress
     * requests.
     *
-    * Also the CURL client can be destructed and pending tasks will still
+    * The CURL client can be destructed and pending tasks will still
     * have a valid RequestContext and will detect the shutdown.
     */
     class RequestContext {
         // Only items used by both the curl thread and the executor/main
         // thread need to be mutex protected.
         std::mutex mutex_;
         std::atomic<bool> shutting_down_;
-        std::atomic<curl_socket_t> curl_socket_;
         // End mutex protected items.
         std::optional<Callbacks> callbacks_;
 
     public:
         // SSE parser using common parser from parser.hpp
         using ParserBody = detail::EventBody<std::function<void(Event)>>;
-        std::unique_ptr<typename ParserBody::value_type> parser_body;
-        std::unique_ptr<typename ParserBody::reader> parser_reader;
+        std::unique_ptr<ParserBody::value_type> parser_body;
+        std::unique_ptr<ParserBody::reader> parser_reader;
 
         // Track last event ID for reconnection (separate from parser state)
         std::optional<std::string> last_event_id;
@@ -165,34 +164,20 @@ class CurlClient final : public Client,
             return shutting_down_;
         }
 
-        void set_curl_socket(curl_socket_t curl_socket) {
-            std::lock_guard lock(mutex_);
-            curl_socket_ = curl_socket;
-        }
-
         void shutdown() {
             std::lock_guard lock(mutex_);
             shutting_down_ = true;
-            if (curl_socket_ != CURL_SOCKET_BAD) {
-#ifdef _WIN32
-                closesocket(curl_socket_);
-#else
-                close(curl_socket_);
-#endif
-            }
         }
 
-
         RequestContext(std::string url,
                        http::request<http::string_body> req,
                        std::optional<std::chrono::milliseconds> connect_timeout,
                        std::optional<std::chrono::milliseconds> read_timeout,
                        std::optional<std::chrono::milliseconds> write_timeout,
                        std::optional<std::string> custom_ca_file,
                        std::optional<std::string> proxy_url,
-                       bool skip_verify_peer
+                       const bool skip_verify_peer
             ) : shutting_down_(false),
-                curl_socket_(CURL_SOCKET_BAD),
                 last_download_amount(0),
                 req(std::move(req)),
                 url(std::move(url)),

libs/server-sent-events/tests/curl_client_test.cpp

@@ -8,7 +8,6 @@
 #include "mock_sse_server.hpp"
 
 #include <boost/asio/io_context.hpp>
-#include <boost/asio/steady_timer.hpp>
 
 #include <atomic>
 #include <chrono>
@@ -23,12 +22,13 @@ using namespace std::chrono_literals;
 namespace {
 
 // C++17-compatible latch replacement
+// https://en.cppreference.com/w/cpp/thread/latch.html
 class SimpleLatch {
 public:
-    explicit SimpleLatch(std::size_t count) : count_(count) {}
+    explicit SimpleLatch(const std::size_t count) : count_(count) {}
 
     void count_down() {
-        std::lock_guard<std::mutex> lock(mutex_);
+        std::lock_guard lock(mutex_);
         if (count_ > 0) {
             --count_;
         }
@@ -37,7 +37,7 @@ class SimpleLatch {
 
     template<typename Rep, typename Period>
     bool wait_for(std::chrono::duration<Rep, Period> timeout) {
-        std::unique_lock<std::mutex> lock(mutex_);
+        std::unique_lock lock(mutex_);
         return cv_.wait_for(lock, timeout, [this] { return count_ == 0; });
     }
 
@@ -82,12 +82,6 @@ class EventCollector {
         return errors_;
     }
 
-    void clear() {
-        std::lock_guard<std::mutex> lock(mutex_);
-        events_.clear();
-        errors_.clear();
-    }
-
 private:
     mutable std::mutex mutex_;
     std::condition_variable cv_;
@@ -456,7 +450,7 @@ TEST(CurlClientTest, Handles500Error) {
     std::atomic<int> connection_attempts{0};
 
     auto handler = [&](auto const&, auto send_response, auto, auto) {
-        connection_attempts++;
+        ++connection_attempts;
         http::response<http::string_body> res{http::status::internal_server_error, 11};
         res.body() = "Error";
         res.prepare_payload();
@@ -585,7 +579,7 @@ TEST(CurlClientTest, HandlesImmediateClose) {
     std::atomic<int> connection_attempts{0};
 
     auto handler = [&](auto const&, auto, auto, auto close) {
-        connection_attempts++;
+        ++connection_attempts;
         close();  // Immediately close without sending headers
     };
 
@@ -664,59 +658,26 @@ TEST(CurlClientTest, RespectsReadTimeout) {
     EXPECT_TRUE(shutdown_latch.wait_for(100ms));
 }
 
-// Resource management tests
-
-TEST(CurlClientTest, NoThreadLeaksAfterMultipleConnections) {
-    // This test verifies that threads are properly joined and not leaked
-    MockSSEServer server;
-    auto port = server.start(TestHandlers::simple_event("test"));
-
-    IoContextRunner runner;
-
-    // Create and destroy multiple clients
-    for (int i = 0; i < 5; i++) {
-        EventCollector collector;
-
-        auto client = Builder(runner.context().get_executor(), "http://localhost:" + std::to_string(port))
-            .receiver([&](Event e) { collector.add_event(std::move(e)); })
-                .build();
-
-        client->async_connect();
-        ASSERT_TRUE(collector.wait_for_events(1));
-
-        SimpleLatch shutdown_latch(1);
-        client->async_shutdown([&] { shutdown_latch.count_down(); });
-        EXPECT_TRUE(shutdown_latch.wait_for(5000ms));
-
-        // Client should be cleanly destroyed here
-    }
-
-    // If threads weren't properly joined, we'd likely see issues here
-    // The test passing indicates proper resource cleanup
-}
-
 TEST(CurlClientTest, DestructorCleansUpProperly) {
-    MockSSEServer server;
-    auto port = server.start([](auto const&, auto send_response, auto send_sse_event, auto) {
-        http::response<http::string_body> res{http::status::ok, 11};
-        res.set(http::field::content_type, "text/event-stream");
-        res.chunked(true);
-        send_response(res);
-
-        // Keep sending events
-        for (int i = 0; i < 100; i++) {
-            send_sse_event(SSEFormatter::event("event " + std::to_string(i)));
-            std::this_thread::sleep_for(10ms);
-        }
-    });
-
-    IoContextRunner runner;
-    EventCollector collector;
-
     {
+        MockSSEServer server;
+        auto port = server.start([](auto const&, auto send_response, auto send_sse_event, auto) {
+            http::response<http::string_body> res{http::status::ok, 11};
+            res.set(http::field::content_type, "text/event-stream");
+            res.chunked(true);
+            send_response(res);
+
+            // Keep sending events
+            for (int i = 0; i < 100; i++) {
+                send_sse_event(SSEFormatter::event("event " + std::to_string(i)));
+                std::this_thread::sleep_for(10ms);
+            }
+        });
+        EventCollector collector;
+        IoContextRunner runner;
         auto client = Builder(runner.context().get_executor(), "http://localhost:" + std::to_string(port))
-            .receiver([&](Event e) { collector.add_event(std::move(e)); })
-                .build();
+                      .receiver([&](Event e) { collector.add_event(std::move(e)); })
+                      .build();
 
         client->async_connect();
         ASSERT_TRUE(collector.wait_for_events(1));
@@ -728,8 +689,6 @@ TEST(CurlClientTest, DestructorCleansUpProperly) {
     // Test passing indicates proper cleanup in destructor
 }
 
-// Edge case tests
-
 TEST(CurlClientTest, HandlesEmptyEventData) {
     MockSSEServer server;
     auto port = server.start([](auto const&, auto send_response, auto send_sse_event, auto close) {
@@ -798,9 +757,9 @@ TEST(CurlClientTest, HandlesEventWithOnlyType) {
 
 TEST(CurlClientTest, HandlesRapidEvents) {
     MockSSEServer server;
-    const int num_events = 100;
+    constexpr int num_events = 100;
 
-    auto port = server.start([num_events](auto const&, auto send_response, auto send_sse_event, auto close) {
+    auto port = server.start([](auto const&, auto send_response, auto send_sse_event, auto close) {
         http::response<http::string_body> res{http::status::ok, 11};
         res.set(http::field::content_type, "text/event-stream");
         res.chunked(true);
@@ -832,14 +791,12 @@ TEST(CurlClientTest, HandlesRapidEvents) {
     EXPECT_TRUE(shutdown_latch.wait_for(5000ms));
 }
 
-// Shutdown-specific tests - critical for preventing crashes/hangs in user applications
-
 TEST(CurlClientTest, ShutdownDuringBackoffDelay) {
     // This ensures clean shutdown during backoff/retry wait period
     std::atomic<int> connection_attempts{0};
 
     auto handler = [&](auto const&, auto send_response, auto, auto) {
-        connection_attempts++;
+        ++connection_attempts;
         // Return 500 to trigger backoff
         http::response<http::string_body> res{http::status::internal_server_error, 11};
         res.body() = "Error";
@@ -906,7 +863,7 @@ TEST(CurlClientTest, ShutdownDuringDataReception) {
 
     IoContextRunner runner;
     // Shared ptr to prevent handling events during destruction.
-    std::shared_ptr<EventCollector> collector = std::make_shared<EventCollector>();
+    auto collector = std::make_shared<EventCollector>();
 
     auto client = Builder(runner.context().get_executor(), "http://localhost:" + std::to_string(port))
         .receiver([collector, &client_received_some](Event e) {