feat: add dns_cache_timeout, max_concurrent_streams, and modernize li…#165
feat: add dns_cache_timeout, max_concurrent_streams, and modernize li…#165
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds DNS cache timeout and HTTP/2 max concurrent streams configuration options, modernizes the libevent integration to use the 2.x API, and improves resource cleanup. The PR also adds coverage build support and fixes curl type warnings for better type safety.
- Added
dns_cache_timeoutrequest option with support for disabling (0), forever caching (-1), or custom timeout in seconds - Added
max_concurrent_streamspool option for controlling HTTP/2 stream limits - Migrated from deprecated libevent 1.x API to modern 2.x API (
event_base_new,bufferevent_socket_new,bufferevent_setcb) - Added proper cleanup code for bufferevents, curl handles, and event base on exit
- Fixed curl type warnings by using
1Lfor boolean curl options andlongfor response codes - Added COVERAGE=1 build flag support with coverage target rules
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/katipo_SUITE.erl | Adds test cases for dns_cache_timeout option with different values and max_concurrent_streams pool option |
| src/katipo.erl | Adds dns_cache_timeout to request record and max_concurrent_streams to pool options type, implements validation and command line argument support |
| c_src/katipo.c | Migrates to libevent 2.x API, adds DNS cache timeout and max concurrent streams support, fixes curl type warnings, adds cleanup code |
| c_src/Makefile | Adds COVERAGE=1 build flag support with coverage HTML generation and cleanup targets |
| README.md | Documents dns_cache_timeout request option and max_concurrent_streams pool option with links to curl documentation |
| .gitignore | Adds C coverage artifacts to gitignore (gcda, gcno, gcov, coverage.info) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…bevent - Add dns_cache_timeout option (0=disable, -1=forever, positive=seconds) - Add max_concurrent_streams pool option for HTTP/2 stream limits - Modernize libevent to 2.x API (event_base_new, bufferevent_socket_new) - Add proper cleanup on exit (bufferevent_free, curl_*_cleanup, event_base_free) - Fix curl type warnings (1L for CURLOPT_POST/NOBODY, long response_code) - Fix multi_timer_cb signature for curl type checking - Add COVERAGE=1 and SANITIZE=1 build flags to Makefile
b52e502 to
f7f4b81
Compare
faa1b5d to
5a8fd7d
Compare
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
8297715 to
eec1e19
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
8ce6131 to
0510a4f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Added explicit initialization - Changed to result.index (was sending garbage bytes) - Duplicate #include <getopt.h> - Redundant memset after calloc - Missing NULL check after realloc - Unused multi parameter in multi_timer_cb
3055906 to
bbe2e37
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 8 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| userpwd => userpwd(), | ||
| ca_cache_timeout => integer(), | ||
| pipewait => boolean()}. |
There was a problem hiding this comment.
The new option dns_cache_timeout is missing from the opts() type specification. It should be added as "dns_cache_timeout => integer()" to match the implementation and the ca_cache_timeout and pipewait options that are already present.
| #if LIBCURL_VERSION_NUM >= 0x074300 /* 7.67.0 */ | ||
| case 's': | ||
| curl_multi_setopt(global.multi, CURLMOPT_MAX_CONCURRENT_STREAMS, | ||
| atoi(optarg)); | ||
| break; |
There was a problem hiding this comment.
The new 's' option for max-concurrent-streams is not included in the getopt_long short options string on line 1353. The string "pac:" should be updated to include 's' with a colon since it requires an argument (e.g., "pacs:"). Without this, the short option form will not work and getopt_long will treat 's' as an unknown option.
| if (CURLSHE_OK != curl_share_setopt(global.shobject, CURLSHOPT_SHARE, CURL_LOCK_DATA_DNS)) { | ||
| errx(2, "curl_share_setopt DNS failed"); |
There was a problem hiding this comment.
DNS sharing is enabled in the global share object but the share object is only attached to individual curl easy handles when lock_data_ssl_session is true (line 899). This means DNS sharing won't work unless lock_data_ssl_session is enabled. The CURLOPT_SHARE should be set unconditionally, or the DNS sharing setup should be conditional on lock_data_ssl_session being enabled somewhere.
| userpwd => userpwd(), | ||
| ca_cache_timeout => integer(), | ||
| pipewait => boolean()}. |
There was a problem hiding this comment.
The new option dns_cache_timeout is missing from both the request() and opts() type specifications. It should be added as "dns_cache_timeout => integer()" to both type definitions to match the implementation and the ca_cache_timeout and pipewait options that are already present.
e19d906 to
688ba41
Compare
…bevent