feat: use thread-local curl handle for multi-concurrency support#214
feat: use thread-local curl handle for multi-concurrency support#214maxday wants to merge 10 commits into
Conversation
Replace the instance member CURL* with a thread_local static CURL* at namespace scope. This allows the runtime to be safely used from multiple threads, each getting their own curl handle without needing separate runtime instances.
Tests cover: - Runtime construction with thread-local curl handle - Multiple sequential runtime instances on the same thread - Concurrent runtime instances on different threads - Sequential requests on a single runtime instance Also fixes a bug from the original change: the destructor must not call curl_easy_cleanup on the thread_local handle, since the handle outlives any single runtime instance and is shared across all instances on the same thread.
Include the HTTP response body in the error log when posting the handler response fails, aiding debugging of Lambda runtime API errors. Add unit tests covering invocation_response, http::response, outcome, invocation_request, and version APIs.
Remove response body from the post failure log message to avoid potential breaking changes in log output format.
Align unit tests with actual API signatures: failure() takes 2 args, invocation_response constructor takes 3, get_header() returns outcome. Remove ExcludeHeaderFilter from .clang-tidy as it's unsupported by CI.
Disable performance-enum-size, misc-use-anonymous-namespace, and misc-include-cleaner which trigger on pre-existing code with newer clang-tidy versions in CI.
- Remove redundant inline specifier on in-class method - Add const to variables that are never modified - Use = default for trivial destructor - Make set_curl_post_result_options static (no instance members used)
- Add std::uint8_t base type to verbosity enum (performance-enum-size) - Move get_prefix to anonymous namespace (misc-use-anonymous-namespace) - Add direct <cstdarg> include in logging.cpp (misc-include-cleaner) - Remove redundant inline on in-class method (readability-redundant-inline-specifier) - Add const qualifiers to unmodified variables (misc-const-correctness) - Use = default for trivial destructor (modernize-use-equals-default) - Make set_curl_post_result_options static (readability-convert-member-functions-to-static)
No longer needed now that real unit tests are in place.
|
Added context: this is basically an application of the patch made to the java ric aws/aws-lambda-java-libs@a9c7fd1#diff-fbda7a3aa6d7793232f2725bf11132d28ac61aa7614047c43598ebffa8901ed3 |
| else() | ||
| message(STATUS "Unit tests skipped: Not in GitHub Actions environment") | ||
| # Build unit tests using the bundled gtest |
There was a problem hiding this comment.
why bother with the above actions specific branch if the bundled one works?
| */ | ||
|
|
||
| #include <cstdarg> | ||
| #include <cstdint> |
There was a problem hiding this comment.
I see in the commit history that there was some fighting with clang tidy, with options added at one point and later reverted. Are the changes in logging.h/logging.cpp still necessary to get the CI to pass? eg: Can the more recent clang-tidy recommendations land in a standalone PR to keep this PR diff a tiny bit cleaner
Trying to understand.. |
| inline bool has_header(char const* header) const; | ||
| inline lambda_runtime::outcome<std::string, bool> get_header(char const* header) const; | ||
| inline response_code get_response_code() const { return m_response_code; } | ||
| response_code get_response_code() const { return m_response_code; } |
There was a problem hiding this comment.
why was this change made?
Figure it was from a clang-tidy during development, but didn't audit all the runs
ah
/home/runner/work/aws-lambda-cpp/aws-lambda-cpp/include/aws/http/response.h:37:5: error: function 'get_response_code' has inline specifier but is implicitly inlined [readability-redundant-inline-specifier,-warnings-as-errors]
37 | inline response_code get_response_code() const { return m_response_code; }
| ^~~~~~
|
Wouldn’t it be better (from a correctness, maintainability standpoint) to have a runtime instance per thread? Was that solution considered? |
|
I keep thinking about this and the more I think about it, the more I realize how bad this solution is.
Please consider extracting the curl handle into a |
3 participants
Description of changes:
includes.
Test result - GitHub action is ✅
Test result - locally:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.