From ef5bba8f965418a2e5072a45d81a2b0b71230e5d Mon Sep 17 00:00:00 2001 From: Aster Seker Date: Sat, 23 May 2026 14:38:30 +0300 Subject: [PATCH 1/4] feat(auth): integrate HttpClient with auth providers, expand tests, fix CMake deprecation - Add HttpClient::set_auth_provider() to auto-authorize every request - Expand auth provider tests with empty token/key edge cases - Expand OAuth tests with validate_state and custom-parser error cases - Suppress CMP0169 FetchContent_Populate deprecation in CMake 3.30+ Co-Authored-By: Claude Opus 4.7 --- CMakeLists.txt | 7 ++++ include/kurlyk/http/HttpClient.hpp | 22 ++++++++++++ tests/auth/test_auth_providers.cpp | 48 +++++++++++++++++++++++++ tests/auth/test_oauth_token_parse.cpp | 51 +++++++++++++++++++++++++++ 4 files changed, 128 insertions(+) diff --git a/CMakeLists.txt b/CMakeLists.txt index ea5a97c..b809ed8 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1,4 +1,11 @@ cmake_minimum_required(VERSION 3.21) + +# Preserve legacy FetchContent_Populate behavior to avoid CMake 3.30+ deprecation warnings +# for fallback dependencies that populate manually (imported targets, interface libraries). +if(POLICY CMP0169) + cmake_policy(SET CMP0169 OLD) +endif() + project(kurlyk VERSION 1.0.2 LANGUAGES CXX) # options diff --git a/include/kurlyk/http/HttpClient.hpp b/include/kurlyk/http/HttpClient.hpp index b26297c..efe480d 100644 --- a/include/kurlyk/http/HttpClient.hpp +++ b/include/kurlyk/http/HttpClient.hpp @@ -5,6 +5,10 @@ /// \file HttpClient.hpp /// \brief Contains the definition of the concrete HttpClient class for making HTTP requests to a specific host. +#if KURLYK_AUTH_SUPPORT +# include "auth/IAuthProvider.hpp" +#endif + namespace kurlyk { /// \class HttpClient @@ -185,6 +189,16 @@ namespace kurlyk { m_request.headers = headers; } +#if KURLYK_AUTH_SUPPORT + /// \brief Assigns an authentication provider to all requests created by this client. + /// \param provider Shared pointer to an IAuthProvider implementation. + /// \note The provider's `authorize()` is called on every request created by this client + /// before submission. Set to `nullptr` to disable. + void set_auth_provider(std::shared_ptr provider) { + m_auth_provider = provider; + } +#endif + /// \brief Assigns an existing rate limit to future requests by ID. /// \param limit_id The unique identifier of the rate limit to assign. /// \param type Rate limit type to configure. @@ -896,6 +910,9 @@ namespace kurlyk { bool m_owns_specific_rate_limit = false; ///< Flag indicating if the client owns the specific rate limit. mutable std::mutex m_submit_mutex; ///< Protects client-side submission settings and admission checks. std::size_t m_max_in_flight = 0; ///< Maximum number of in-flight requests for this client group, or 0 for disabled. +# if KURLYK_AUTH_SUPPORT + std::shared_ptr m_auth_provider; ///< Optional authentication provider applied to every request. +# endif /// \brief Adds the request to the request manager and notifies the worker to process it. /// \param request_ptr The HTTP request to be sent. @@ -929,6 +946,11 @@ namespace kurlyk { request_ptr->set_url(m_host, path, query); request_ptr->headers.insert(headers.begin(), headers.end()); request_ptr->content = content; +# if KURLYK_AUTH_SUPPORT + if (m_auth_provider) { + m_auth_provider->authorize(*request_ptr); + } +# endif return request_ptr; } diff --git a/tests/auth/test_auth_providers.cpp b/tests/auth/test_auth_providers.cpp index d23e40b..4f03195 100644 --- a/tests/auth/test_auth_providers.cpp +++ b/tests/auth/test_auth_providers.cpp @@ -53,5 +53,53 @@ int main() { if (req.url != "https://example.com/api?foo=bar&key=val") return 1; } + // Edge cases: empty token returns false and does not inject header + { + kurlyk::Headers headers; + kurlyk::http::auth::BearerTokenAuthProvider bearer(""); + if (bearer.authorize(headers)) return 1; + if (headers.find("Authorization") != headers.end()) return 1; + } + + // Edge cases: empty ApiKey name or value returns false + { + kurlyk::Headers headers; + kurlyk::http::auth::ApiKeyAuthProvider api_key( + "", "val", kurlyk::http::auth::ApiKeyPlacement::HEADER); + if (api_key.authorize(headers)) return 1; + } + { + kurlyk::Headers headers; + kurlyk::http::auth::ApiKeyAuthProvider api_key( + "key", "", kurlyk::http::auth::ApiKeyPlacement::HEADER); + if (api_key.authorize(headers)) return 1; + } + + // Edge cases: empty ApiKey name or value in QUERY mode leaves URL unchanged + { + kurlyk::HttpRequest req; + req.url = "https://example.com/api"; + kurlyk::http::auth::ApiKeyAuthProvider api_key( + "", "val", kurlyk::http::auth::ApiKeyPlacement::QUERY); + if (api_key.authorize(req)) return 1; + if (req.url != "https://example.com/api") return 1; + } + { + kurlyk::HttpRequest req; + req.url = "https://example.com/api"; + kurlyk::http::auth::ApiKeyAuthProvider api_key( + "key", "", kurlyk::http::auth::ApiKeyPlacement::QUERY); + if (api_key.authorize(req)) return 1; + if (req.url != "https://example.com/api") return 1; + } + + // Edge case: ApiKeyAuthProvider QUERY placement with Headers interface returns false + { + kurlyk::Headers headers; + kurlyk::http::auth::ApiKeyAuthProvider api_key( + "key", "val", kurlyk::http::auth::ApiKeyPlacement::QUERY); + if (api_key.authorize(headers)) return 1; + } + return 0; } diff --git a/tests/auth/test_oauth_token_parse.cpp b/tests/auth/test_oauth_token_parse.cpp index 8702324..18fbb1f 100644 --- a/tests/auth/test_oauth_token_parse.cpp +++ b/tests/auth/test_oauth_token_parse.cpp @@ -52,5 +52,56 @@ int main() { if (result.error != kurlyk::AuthError::UnsupportedFlow) return 1; } + // Custom parser: simulates error response with invalid_grant + { + TestClient client3(config); + client3.set_token_parser([](const std::string& raw, + kurlyk::OAuthToken& token, + std::string& err) -> bool { + if (raw.find("invalid_grant") != std::string::npos) { + err = "invalid_grant"; + return false; + } + token.access_token = "ok"; + token.token_type = "Bearer"; + return true; + }); + kurlyk::AuthResult result; + if (client3.test_parse("invalid_grant", result)) return 1; + if (result.success) return 1; + if (result.error != kurlyk::AuthError::InvalidResponse) return 1; + if (result.error_message != "invalid_grant") return 1; + } + + // validate_state: empty strings return false + { + kurlyk::http::auth::OAuthPkceClient empty_state(config); + if (empty_state.validate_state("")) return 1; + } + + // validate_state: matching state returns true + { + kurlyk::OAuthConfig good_config; + good_config.client_id = "client"; + good_config.authorization_endpoint = "https://example.com/auth"; + good_config.redirect_uri = "https://example.com/cb"; + kurlyk::http::auth::OAuthPkceClient match_client(good_config); + std::string url = match_client.build_authorization_url(); + (void)url; + if (!match_client.validate_state(match_client.state())) return 1; + } + + // validate_state: mismatching state returns false + { + kurlyk::OAuthConfig good_config; + good_config.client_id = "client"; + good_config.authorization_endpoint = "https://example.com/auth"; + good_config.redirect_uri = "https://example.com/cb"; + kurlyk::http::auth::OAuthPkceClient mismatch_client(good_config); + std::string url = mismatch_client.build_authorization_url(); + (void)url; + if (mismatch_client.validate_state("wrong_state")) return 1; + } + return 0; } From b5a1cf450a3177ebdc9a05a3f9a83e278be7ae58 Mon Sep 17 00:00:00 2001 From: Aster Seker Date: Sun, 24 May 2026 01:17:30 +0300 Subject: [PATCH 2/4] fix(http): race condition in wait_requests and curl init failure handling wait_requests_by_group_id() could return immediately if group_request_count_unlocked() was zero between moving requests out of m_pending_requests and pushing the batch into m_active_request_batches. This caused the test to proceed before the callback fired, or the callback to be dropped entirely if curl_easy_init failed silently. - HttpBatchRequestHandler: fail all contexts immediately when curl_multi_init() or curl_easy_init() returns nullptr, instead of silently dropping the request context. - HttpRequestManager::process_pending_requests(): wrap batch creation in try/catch and fail all pending contexts on exception instead of silently losing them. - HttpRequestManager::wait_requests_by_group_id(): remove the "invoke_now" fast path; always register the waiter callback so it is delivered by notify_group_waiters_if_idle() after the next process() loop, closing the pending-to-active staging window. - http_client_wait_requests_test: add diagnostic prints for ok, in_flight and callback_count to make future CI failures observable. Directive: Always defer waiter callback to notify_group_waiters_if_idle() to avoid the pending-to-active staging race. Directive: Fail pending contexts immediately if HttpBatchRequestHandler or curl_multi_init() throws, instead of silently dropping them. Scope-risk: moderate (core request manager paths touched) Co-Authored-By: Claude Opus 4.7 --- include/kurlyk/http/HttpRequestManager.hpp | 39 +++++++----- .../HttpBatchRequestHandler.hpp | 61 ++++++++++++++++++- .../http_client_wait_requests_main.cpp | 5 ++ 3 files changed, 89 insertions(+), 16 deletions(-) diff --git a/include/kurlyk/http/HttpRequestManager.hpp b/include/kurlyk/http/HttpRequestManager.hpp index 39c491c..1cabfa3 100644 --- a/include/kurlyk/http/HttpRequestManager.hpp +++ b/include/kurlyk/http/HttpRequestManager.hpp @@ -204,18 +204,9 @@ namespace kurlyk { return; } - bool invoke_now = false; { std::lock_guard lock(m_mutex); - if (group_request_count_unlocked(group_id) == 0) { - invoke_now = true; - } else { - m_group_waiters[group_id].push_back(std::move(callback)); - } - } - - if (invoke_now && callback) { - callback(); + m_group_waiters[group_id].push_back(std::move(callback)); } } @@ -431,11 +422,29 @@ namespace kurlyk { // If there are ready requests, create a new HttpBatchRequestHandler to manage them. if (pending_request.empty()) return; -# if __cplusplus >= 201402L - m_active_request_batches.push_back(std::make_unique(pending_request)); -# else - m_active_request_batches.push_back(std::unique_ptr(new HttpBatchRequestHandler(pending_request))); -# endif + try { +# if __cplusplus >= 201402L + m_active_request_batches.push_back(std::make_unique(pending_request)); +# else + m_active_request_batches.push_back(std::unique_ptr(new HttpBatchRequestHandler(pending_request))); +# endif + } catch (...) { + for (auto& context : pending_request) { + if (!context || !context->callback) continue; +# if __cplusplus >= 201402L + auto response = std::make_unique(); +# else + auto response = std::unique_ptr(new HttpResponse()); +# endif + response->error_code = utils::make_error_code(utils::ClientError::AbortedDuringDestruction); + response->status_code = 499; // Client closed request + response->ready = true; + context->callback(std::move(response)); + context->complete(); + } + lock.unlock(); + return; + } } /// \brief Processes active requests, moving failed ones to the failed requests list for retrying. diff --git a/include/kurlyk/http/HttpRequestManager/HttpBatchRequestHandler.hpp b/include/kurlyk/http/HttpRequestManager/HttpBatchRequestHandler.hpp index 0e10eda..49c8750 100644 --- a/include/kurlyk/http/HttpRequestManager/HttpBatchRequestHandler.hpp +++ b/include/kurlyk/http/HttpRequestManager/HttpBatchRequestHandler.hpp @@ -16,18 +16,57 @@ namespace kurlyk { /// \param context_list List of unique pointers to HttpRequestContext objects. explicit HttpBatchRequestHandler(std::vector>& context_list) : m_multi_handle(curl_multi_init()) { + if (!m_multi_handle) { + // libcurl multi handle creation failed: fail all requests immediately. + for (auto& context : context_list) { + if (!context || !context->callback) continue; +# if __cplusplus >= 201402L + auto response = std::make_unique(); +# else + auto response = std::unique_ptr(new HttpResponse()); +# endif + response->error_code = utils::make_error_code(utils::ClientError::AbortedDuringDestruction); + response->status_code = 499; // Client closed request + response->ready = true; + context->callback(std::move(response)); + context->complete(); + context.reset(); + } + return; + } for (auto& context : context_list) { + if (!context) continue; # if __cplusplus >= 201402L auto handler = std::make_unique(std::move(context)); # else auto handler = std::unique_ptr(new HttpRequestHandler(std::move(context))); # endif CURL* curl = handler->get_curl(); - if (!curl) continue; + if (!curl) { + // curl_easy_init failed: deliver error immediately. + auto ctx = handler->get_request_context(); + if (ctx && ctx->callback) { +# if __cplusplus >= 201402L + auto response = std::make_unique(); +# else + auto response = std::unique_ptr(new HttpResponse()); +# endif + response->error_code = utils::make_error_code(utils::ClientError::AbortedDuringDestruction); + response->status_code = 499; // Client closed request + response->ready = true; + ctx->callback(std::move(response)); + ctx->complete(); + } + continue; + } curl_multi_add_handle(m_multi_handle, curl); m_handlers.push_back(std::move(handler)); } + // Ensure the source vector is empty after moving contexts into handlers or failing them. + for (auto& context : context_list) { + context.reset(); + } } /// \brief Cleans up the multi handle and removes all request handles. @@ -43,6 +82,26 @@ namespace kurlyk { /// \brief Processes the requests within the handler. /// \return True if all requests are completed, false otherwise. bool process() { + if (!m_multi_handle) { + // Multi handle was never created: fail all handlers immediately. + for (auto& handler : m_handlers) { + auto ctx = handler->get_request_context(); + if (ctx && ctx->callback) { +# if __cplusplus >= 201402L + auto response = std::make_unique(); +# else + auto response = std::unique_ptr(new HttpResponse()); +# endif + response->error_code = utils::make_error_code(utils::ClientError::AbortedDuringDestruction); + response->status_code = 499; // Client closed request + response->ready = true; + ctx->callback(std::move(response)); + ctx->complete(); + } + } + m_handlers.clear(); + return true; + } int still_running = 0; CURLMcode res = curl_multi_perform(m_multi_handle, &still_running); if (res != CURLM_OK) return false; diff --git a/tests/integration/http_client_wait_requests_main.cpp b/tests/integration/http_client_wait_requests_main.cpp index 4413177..367fd4d 100644 --- a/tests/integration/http_client_wait_requests_main.cpp +++ b/tests/integration/http_client_wait_requests_main.cpp @@ -96,7 +96,12 @@ int main() { }); require(ok, "request should be accepted"); + std::cerr << "[diag test1] ok=" << ok + << " in_flight=" << client->in_flight_requests() + << " callback_count=" << callback_count.load() + << std::endl; client->wait_requests(); + std::cerr << "[diag test1] after wait_requests callback_count=" << callback_count.load() << std::endl; require(callback_count.load() == 1, "wait_requests() must wait until callback is delivered"); require(client->in_flight_requests() == 0, "client group must be idle after wait_requests()"); From 4df21d3d8b77e16d5bd1f6dba1637bd55498d24f Mon Sep 17 00:00:00 2001 From: Aster Seker Date: Sun, 24 May 2026 01:58:15 +0300 Subject: [PATCH 3/4] fix(auth): address PR review comments on auth provider tests and docs - Wrap auth examples in if(KURLYK_AUTH_SUPPORT) in examples/CMakeLists.txt - Add integration test for HttpClient::set_auth_provider() injection behavior - Document that provider overwrites per-request Authorization headers - Document thread-safety contract for set_auth_provider() Co-Authored-By: Claude Opus 4.7 --- examples/CMakeLists.txt | 11 +- guides/auth-providers.md | 6 + include/kurlyk/http/HttpClient.hpp | 5 +- tests/integration/CMakeLists.txt | 9 ++ .../http_client_auth_provider_main.cpp | 146 ++++++++++++++++++ 5 files changed, 173 insertions(+), 4 deletions(-) create mode 100644 tests/integration/http_client_auth_provider_main.cpp diff --git a/examples/CMakeLists.txt b/examples/CMakeLists.txt index 5b6e812..8b4a314 100644 --- a/examples/CMakeLists.txt +++ b/examples/CMakeLists.txt @@ -19,11 +19,16 @@ set(KURLYK_EXAMPLE_SOURCES websocket_client_lifecycle_test.cpp websocket_echo_example.cpp websocket_independent_clients_example.cpp - bearer_token_auth_provider_example.cpp - api_key_auth_provider_example.cpp - simple_bearer_auth_example.cpp ) +if(KURLYK_AUTH_SUPPORT) + list(APPEND KURLYK_EXAMPLE_SOURCES + bearer_token_auth_provider_example.cpp + api_key_auth_provider_example.cpp + simple_bearer_auth_example.cpp + ) +endif() + if(KURLYK_OAUTH_SUPPORT) list(APPEND KURLYK_EXAMPLE_SOURCES openrouter_oauth_pkce_example.cpp diff --git a/guides/auth-providers.md b/guides/auth-providers.md index 6a651af..9f8aa8d 100644 --- a/guides/auth-providers.md +++ b/guides/auth-providers.md @@ -6,6 +6,8 @@ Injects an `Authorization: Bearer ` header, replacing any previous `Authorization` value. +When attached to an `HttpClient` via `set_auth_provider()`, the provider runs **after** per-request headers are merged. This means a provider can overwrite a header that was set manually on an individual request. + ```cpp kurlyk::http::auth::BearerTokenAuthProvider auth("my_api_key"); @@ -44,6 +46,10 @@ query_auth.authorize(request); Query parameters are percent-encoded automatically via `kurlyk::utils::percent_encode`. +## Thread safety + +`HttpClient::set_auth_provider()` is not internally synchronized. Set the provider before starting concurrent requests, or guard the call and subsequent submissions with external synchronization. + ## Implementing a custom provider Derive from `IAuthProvider` and implement `authorize(HttpRequest&)` and `authorize(Headers&)`. diff --git a/include/kurlyk/http/HttpClient.hpp b/include/kurlyk/http/HttpClient.hpp index efe480d..c8b6db6 100644 --- a/include/kurlyk/http/HttpClient.hpp +++ b/include/kurlyk/http/HttpClient.hpp @@ -193,7 +193,10 @@ namespace kurlyk { /// \brief Assigns an authentication provider to all requests created by this client. /// \param provider Shared pointer to an IAuthProvider implementation. /// \note The provider's `authorize()` is called on every request created by this client - /// before submission. Set to `nullptr` to disable. + /// after per-request headers are merged, so it can overwrite headers such as + /// `Authorization` set manually on the request. Set to `nullptr` to disable. + /// \note Thread-safety: this setter is not synchronized; call it only before concurrent + /// requests begin, or externally synchronize with request submission. void set_auth_provider(std::shared_ptr provider) { m_auth_provider = provider; } diff --git a/tests/integration/CMakeLists.txt b/tests/integration/CMakeLists.txt index bb66680..8f2a282 100644 --- a/tests/integration/CMakeLists.txt +++ b/tests/integration/CMakeLists.txt @@ -37,6 +37,10 @@ add_executable(http_client_wait_requests_test http_client_wait_requests_main.cpp ) +add_executable(http_client_auth_provider_test + http_client_auth_provider_main.cpp +) + add_executable(sequential_rate_limit_test sequential_rate_limit_main.cpp ) @@ -98,6 +102,8 @@ target_link_libraries(request_group_cancel_test PRIVATE kurlyk) target_link_libraries(http_client_rate_limit_api_test PRIVATE kurlyk) target_link_libraries(http_client_wait_requests_test PRIVATE kurlyk) target_include_directories(http_client_wait_requests_test PRIVATE "${CMAKE_CURRENT_SOURCE_DIR}/../../external/Simple-Web-Server") +target_link_libraries(http_client_auth_provider_test PRIVATE kurlyk) +target_include_directories(http_client_auth_provider_test PRIVATE "${CMAKE_CURRENT_SOURCE_DIR}/../../external/Simple-Web-Server") target_link_libraries(sequential_rate_limit_test PRIVATE kurlyk) target_link_libraries(rate_limit_partition_test PRIVATE kurlyk) target_link_libraries(http_local_test PRIVATE kurlyk) @@ -116,6 +122,7 @@ target_compile_features(rate_limit_lifetime_test PRIVATE cxx_std_17) target_compile_features(request_group_cancel_test PRIVATE cxx_std_17) target_compile_features(http_client_rate_limit_api_test PRIVATE cxx_std_17) target_compile_features(http_client_wait_requests_test PRIVATE cxx_std_17) +target_compile_features(http_client_auth_provider_test PRIVATE cxx_std_17) target_compile_features(sequential_rate_limit_test PRIVATE cxx_std_17) target_compile_features(rate_limit_partition_test PRIVATE cxx_std_17) target_compile_features(http_local_test PRIVATE cxx_std_17) @@ -137,6 +144,7 @@ copy_runtime_dlls(rate_limit_lifetime_test) copy_runtime_dlls(request_group_cancel_test) copy_runtime_dlls(http_client_rate_limit_api_test) copy_runtime_dlls(http_client_wait_requests_test) +copy_runtime_dlls(http_client_auth_provider_test) copy_runtime_dlls(sequential_rate_limit_test) copy_runtime_dlls(rate_limit_partition_test) copy_runtime_dlls(http_local_test) @@ -150,6 +158,7 @@ add_test(NAME rate_limit_lifetime_test COMMAND rate_limit_lifetime_test) add_test(NAME request_group_cancel_test COMMAND request_group_cancel_test) add_test(NAME http_client_rate_limit_api_test COMMAND http_client_rate_limit_api_test) add_test(NAME http_client_wait_requests_test COMMAND http_client_wait_requests_test) +add_test(NAME http_client_auth_provider_test COMMAND http_client_auth_provider_test) add_test(NAME sequential_rate_limit_test COMMAND sequential_rate_limit_test) add_test(NAME rate_limit_partition_test COMMAND rate_limit_partition_test) add_test(NAME http_local_test COMMAND http_local_test) diff --git a/tests/integration/http_client_auth_provider_main.cpp b/tests/integration/http_client_auth_provider_main.cpp new file mode 100644 index 0000000..1ef4f1a --- /dev/null +++ b/tests/integration/http_client_auth_provider_main.cpp @@ -0,0 +1,146 @@ +#define KURLYK_AUTO_INIT 0 +#include +#include + +#include +#include +#include +#include +#include +#include +#include + +namespace { + +using HttpServer = SimpleWeb::Server; + +void require(bool condition, const std::string& message) { + if (!condition) { + std::cerr << message << std::endl; + std::exit(1); + } +} + +void background_process(std::atomic& stop) { + while (!stop.load()) { + kurlyk::process(); + std::this_thread::sleep_for(std::chrono::milliseconds(5)); + } +} + +struct ProcessorGuard { + std::atomic stop{false}; + std::thread thread; + + ProcessorGuard() : thread([this]() { background_process(stop); }) {} + ~ProcessorGuard() { + stop.store(true); + if (thread.joinable()) thread.join(); + } +}; + +} // namespace + +int main() { + kurlyk::init(false); + + HttpServer server; + server.config.port = 0; + server.config.thread_pool_size = 2; + + std::string last_auth_header; + server.resource["^/echo-auth$"]["GET"] = [&last_auth_header]( + std::shared_ptr response, + std::shared_ptr request) { + auto it = request->header.find("Authorization"); + if (it != request->header.end()) { + last_auth_header = it->second; + } else { + last_auth_header.clear(); + } + *response << "HTTP/1.1 200 OK\r\nContent-Length: 2\r\n\r\nok"; + }; + + std::promise port_promise; + std::thread server_thread([&server, &port_promise]() { + server.start([&port_promise](unsigned short port) { + try { + port_promise.set_value(port); + } catch (...) {} + }); + }); + + const unsigned short port = port_promise.get_future().get(); + const std::string base_url = "http://127.0.0.1:" + std::to_string(port); + + { + ProcessorGuard pg; + + // --- Test 1: BearerTokenAuthProvider injected via HttpClient --- + { + last_auth_header.clear(); + auto client = std::make_unique(base_url); + client->set_auth_provider(std::make_shared( + "test-token-123")); + + std::atomic callback_done{false}; + bool ok = client->get("/echo-auth", kurlyk::QueryParams(), kurlyk::Headers(), + [&](kurlyk::HttpResponsePtr response) { + callback_done = true; + }); + require(ok, "request should be accepted"); + client->wait_requests(); + require(callback_done.load(), "callback must be delivered"); + require(last_auth_header == "Bearer test-token-123", + "Authorization header must be injected by BearerTokenAuthProvider"); + } + + // --- Test 2: per-request Authorization header overwritten by provider --- + { + last_auth_header.clear(); + auto client = std::make_unique(base_url); + client->set_auth_provider(std::make_shared( + "provider-token")); + + std::atomic callback_done{false}; + kurlyk::Headers headers; + headers.emplace("Authorization", "manual-token"); + bool ok = client->get("/echo-auth", kurlyk::QueryParams(), headers, + [&](kurlyk::HttpResponsePtr response) { + callback_done = true; + }); + require(ok, "request should be accepted"); + client->wait_requests(); + require(callback_done.load(), "callback must be delivered"); + require(last_auth_header == "Bearer provider-token", + "provider must overwrite per-request Authorization header"); + } + + // --- Test 3: set_auth_provider(nullptr) disables injection --- + { + last_auth_header.clear(); + auto client = std::make_unique(base_url); + client->set_auth_provider(std::make_shared( + "disabled-token")); + client->set_auth_provider(nullptr); + + std::atomic callback_done{false}; + bool ok = client->get("/echo-auth", kurlyk::QueryParams(), kurlyk::Headers(), + [&](kurlyk::HttpResponsePtr response) { + callback_done = true; + }); + require(ok, "request should be accepted"); + client->wait_requests(); + require(callback_done.load(), "callback must be delivered"); + require(last_auth_header.empty(), + "Authorization must be absent when provider is nullptr"); + } + } + + server.stop(); + server_thread.join(); + + kurlyk::deinit(); + std::cout << "HttpClient auth provider integration test passed" << std::endl; + return 0; +} From b1f63e118e917e09a20993f1782a2604fab8968b Mon Sep 17 00:00:00 2001 From: Aster Seker Date: Sun, 24 May 2026 02:14:33 +0300 Subject: [PATCH 4/4] fix(http): address reviewer feedback on request manager safety - Remove double lock.unlock() in process_pending_requests() catch block. - Restore immediate callback in wait_requests_by_group_id() for empty groups while keeping race-safe pattern (decide under lock, invoke outside). - Remove diagnostic cerr from http_client_wait_requests_main.cpp. - Guard curl_multi_cleanup with if(m_multi_handle) in destructor. Co-Authored-By: Claude Opus 4.7 --- include/kurlyk/http/HttpRequestManager.hpp | 12 ++++++++++-- .../HttpRequestManager/HttpBatchRequestHandler.hpp | 4 +++- tests/integration/http_client_wait_requests_main.cpp | 5 ----- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/include/kurlyk/http/HttpRequestManager.hpp b/include/kurlyk/http/HttpRequestManager.hpp index 1cabfa3..68aa7f6 100644 --- a/include/kurlyk/http/HttpRequestManager.hpp +++ b/include/kurlyk/http/HttpRequestManager.hpp @@ -204,9 +204,18 @@ namespace kurlyk { return; } + bool invoke_now = false; { std::lock_guard lock(m_mutex); - m_group_waiters[group_id].push_back(std::move(callback)); + if (group_request_count_unlocked(group_id) == 0) { + invoke_now = true; + } else { + m_group_waiters[group_id].push_back(std::move(callback)); + } + } + + if (invoke_now && callback) { + callback(); } } @@ -442,7 +451,6 @@ namespace kurlyk { context->callback(std::move(response)); context->complete(); } - lock.unlock(); return; } } diff --git a/include/kurlyk/http/HttpRequestManager/HttpBatchRequestHandler.hpp b/include/kurlyk/http/HttpRequestManager/HttpBatchRequestHandler.hpp index 49c8750..cafb49d 100644 --- a/include/kurlyk/http/HttpRequestManager/HttpBatchRequestHandler.hpp +++ b/include/kurlyk/http/HttpRequestManager/HttpBatchRequestHandler.hpp @@ -76,7 +76,9 @@ namespace kurlyk { if (!curl) continue; curl_multi_remove_handle(m_multi_handle, curl); } - curl_multi_cleanup(m_multi_handle); + if (m_multi_handle) { + curl_multi_cleanup(m_multi_handle); + } } /// \brief Processes the requests within the handler. diff --git a/tests/integration/http_client_wait_requests_main.cpp b/tests/integration/http_client_wait_requests_main.cpp index 367fd4d..4413177 100644 --- a/tests/integration/http_client_wait_requests_main.cpp +++ b/tests/integration/http_client_wait_requests_main.cpp @@ -96,12 +96,7 @@ int main() { }); require(ok, "request should be accepted"); - std::cerr << "[diag test1] ok=" << ok - << " in_flight=" << client->in_flight_requests() - << " callback_count=" << callback_count.load() - << std::endl; client->wait_requests(); - std::cerr << "[diag test1] after wait_requests callback_count=" << callback_count.load() << std::endl; require(callback_count.load() == 1, "wait_requests() must wait until callback is delivered"); require(client->in_flight_requests() == 0, "client group must be idle after wait_requests()");