-
Notifications
You must be signed in to change notification settings - Fork 496
[client] refactor http_curl, add unit tests #6617
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Vitalii Koshura <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the HTTP CURL implementation by converting global state into a singleton class and replaces legacy C-style callbacks with C++ lambdas. It also adds comprehensive unit tests for the new HTTP_CURL class.
- Converted global variables and functions into the
HTTP_CURLsingleton class - Replaced C-style callback functions with lambda expressions
- Added unit tests covering singleton behavior, thread safety, and HTTP protocol flags
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit-tests/client/test_http_curl.cpp | New unit test file with tests for HTTP_CURL singleton methods and thread-safe trace ID generation |
| tests/unit-tests/client/CMakeLists.txt | New CMake configuration for client unit tests, includes all necessary client source files and dependencies |
| tests/unit-tests/CMakeLists.txt | Updated to add client subdirectory and required dependencies (CURL, ZLIB, OpenSSL, X11) |
| tests/executeUnitTests.sh | Updated test execution script to include client tests for both Darwin and other platforms |
| client/main.cpp | Removed explicit curl_init() and curl_cleanup() calls as they're now managed by HTTP_CURL singleton |
| client/http_curl.h | Added HTTP_CURL singleton class definition with thread-safe atomic counters, removed global function declarations |
| client/http_curl.cpp | Refactored to use HTTP_CURL singleton, converted callback functions to lambdas, improved type safety with static_cast |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
client/http_curl.cpp
Outdated
| } | ||
| HTTP_CURL::HTTP_CURL() : | ||
| curlMulti(curl_init()), | ||
| user_agent_string(std::move(get_user_agent_string())) { |
Copilot
AI
Oct 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling the member function get_user_agent_string() during initialization is unsafe because it accesses the user_agent_string member variable before initialization completes. Move the initialization logic to the constructor body or ensure get_user_agent_string() doesn't reference uninitialized members.
| user_agent_string(std::move(get_user_agent_string())) { | |
| user_agent_string() { | |
| user_agent_string = get_user_agent_string(); |
client/http_curl.cpp
Outdated
| curl_easy_setopt(curlEasy, CURLOPT_DEBUGDATA, this ); | ||
| curl_easy_setopt(curlEasy, CURLOPT_DEBUGFUNCTION, | ||
| [](CURL*, curl_infotype type, char *data, size_t, HTTP_OP* phop) { | ||
| if (!log_flags.http_debug) return; |
Copilot
AI
Oct 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lambda has return type deduced from first return statement (line 693), which returns void, but line 707 attempts to return without a value which is inconsistent. The return type should be explicitly specified as 'int' and line 707 should return 0 to match the original function signature expected by CURLOPT_DEBUGFUNCTION.
client/http_curl.cpp
Outdated
| "[http] [ID#%u] %s %s\n", phop->trace_id, desc, line.c_str() | ||
| ); | ||
| } | ||
| return; |
Copilot
AI
Oct 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This return statement is unreachable because all switch cases either break or return, and the code after the switch is also a return. Remove this redundant return statement.
|
|
||
| set (CMAKE_CXX_STANDARD 17) | ||
| set (CMAKE_CXX_FLAGS "-g -Wall -Wextra -Werror --coverage") | ||
| #set (CMAKE_CXX_FLAGS "-g -Wall -Wextra -Werror --coverage") |
Copilot
AI
Oct 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commented-out line with -Werror should be removed rather than kept as a comment. If -Werror needs to be disabled temporarily, document the reason in a comment.
| #set (CMAKE_CXX_FLAGS "-g -Wall -Wextra -Werror --coverage") | |
| # -Werror is currently omitted to allow compilation with warnings during development and CI. |
client/http_curl.h
Outdated
| HTTP_CURL(HTTP_CURL const&); // must be private and unimplemented | ||
| void operator=(HTTP_CURL const&); // must be private and unimplemented |
Copilot
AI
Oct 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In C++11 and later, use '= delete' syntax instead of declaring copy constructor and assignment operator as private and unimplemented. Change to 'HTTP_CURL(HTTP_CURL const&) = delete;' and 'void operator=(HTTP_CURL const&) = delete;'
| HTTP_CURL(HTTP_CURL const&); // must be private and unimplemented | |
| void operator=(HTTP_CURL const&); // must be private and unimplemented | |
| HTTP_CURL(HTTP_CURL const&) = delete; | |
| void operator=(HTTP_CURL const&) = delete; |
Signed-off-by: Vitalii Koshura <[email protected]>
No description provided.