tests: runtime: stabilize http client chunked test#11723
tests: runtime: stabilize http client chunked test#11723
Conversation
📝 WalkthroughWalkthroughAdded Fluent Bit network/DNS setup to the HTTP client test context, disabled upstream async mode, introduced server-thread mutex/cond synchronization to signal readiness before accept(), hardened server read/close behavior, and added proper init/cleanup of synchronization primitives in the chunked-response test. Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant ServerThread as Server Thread
participant ClientCtx as HTTP Client Context
participant NetDNS as Net/DNS
Test->>ServerThread: start thread
ServerThread->>ServerThread: init mutex/cv, set accepting=false
ServerThread-->>Test: signal "ready_to_accept"
Test->>ClientCtx: create client context
ClientCtx->>NetDNS: flb_net_ctx_init / flb_net_dns_ctx_init / flb_net_dns_ctx_set
ClientCtx->>ClientCtx: flb_stream_disable_async_mode
Test->>ServerThread: connect to server (after waiting)
ServerThread->>ServerThread: accept() -> connection
ServerThread->>ServerThread: read request (if read() <= 0 -> close & exit)
ServerThread->>ClientCtx: send chunked response
ClientCtx->>Test: receive/process response
Test->>ClientCtx: destroy client context (flb_net_dns_lookup_context_cleanup, flb_net_dns_ctx_set(NULL))
ServerThread->>ServerThread: cleanup mutex/cv
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
edsiper
force-pushed
the
fix-http-client-test
branch
from
7c4bb36 to
33cef76
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/runtime/http_client_chunked.c (2)
305-330:⚠️ Potential issue | 🟠 MajorKeep trailer parsing asserted in this chunked-response test.
The server still sends
Trailer: Expires, X-Trace, but the test now stops at payload verification. That turns this into a payload reassembly check only, so a regression in chunked trailer parsing can slip through unnoticed. If the old header assertions were the flaky part, I'd split them into a separate focused trailer test rather than dropping them.As per coding guidelines,
tests/**/*.{c,cpp,py}: Add or update tests for behavior changes, especially protocol parsing and encoder/decoder paths; Validate both success and failure paths in tests: invalid payloads, boundary sizes, null/missing fields.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/runtime/http_client_chunked.c` around lines 305 - 330, The test currently only verifies payload reassembly after calling flb_http_client and flb_http_do (using client and client->resp) and drops assertions that the chunked response trailers were parsed; restore assertions that validate the trailers (e.g., check client->resp.trailer or client->resp.headers for "Expires" and "X-Trace" and their values) so the test covers trailer parsing as well as payload reassembly, or if those header checks were flaky, move them into a new focused test that exercises trailer parsing separately while keeping this test focused on payload reassembly.
191-219:⚠️ Potential issue | 🟠 MajorAdd DNS context cleanup to all failure paths after line 184.
After
flb_net_dns_ctx_set(&ctx->dns_ctx)on line 184, the DNS context is active and must be cleaned up on every error return. Currently:
- Line 188 has
flb_net_dns_ctx_set(NULL)but missingflb_net_dns_lookup_context_cleanup()- Lines 197–202 (upstream_create failure) are missing both cleanup calls
- Lines 209–214 (upstream_conn_get failure) are missing both cleanup calls
Production code (flb_engine.c, flb_input_thread.c, flb_output_thread.c) consistently pairs both functions. Add them to all three failure paths:
Suggested fix
ctx->config = flb_config_init(); if (!TEST_CHECK(ctx->config != NULL)) { + flb_net_dns_lookup_context_cleanup(&ctx->dns_ctx); flb_net_dns_ctx_set(NULL); mk_event_loop_destroy(ctx->evl); flb_free(ctx); return NULL; } ctx->u = flb_upstream_create(ctx->config, "127.0.0.1", port, 0, NULL); if (!TEST_CHECK(ctx->u != NULL)) { + flb_net_dns_lookup_context_cleanup(&ctx->dns_ctx); + flb_net_dns_ctx_set(NULL); flb_config_exit(ctx->config); mk_event_loop_destroy(ctx->evl); flb_free(ctx); return NULL; } ctx->u_conn = flb_upstream_conn_get(ctx->u); if (!TEST_CHECK(ctx->u_conn != NULL)) { flb_upstream_destroy(ctx->u); + flb_net_dns_lookup_context_cleanup(&ctx->dns_ctx); + flb_net_dns_ctx_set(NULL); flb_config_exit(ctx->config); mk_event_loop_destroy(ctx->evl); flb_free(ctx); return NULL; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/runtime/http_client_chunked.c` around lines 191 - 219, The DNS lookup context set by flb_net_dns_ctx_set(&ctx->dns_ctx) must be cleaned up on every early return; add calls to flb_net_dns_lookup_context_cleanup() followed by flb_net_dns_ctx_set(NULL) in the failure branches for TEST_CHECK(ctx->config != NULL), the flb_upstream_create failure (ctx->u == NULL), and the flb_upstream_conn_get failure (ctx->u_conn == NULL), ensuring you perform the cleanup before destroying the event loop and freeing ctx to mirror the pattern used in flb_engine.c/flb_input_thread.c.
🧹 Nitpick comments (1)
tests/runtime/http_client_chunked.c (1)
282-287: Route thepthread_create()failure through the shared teardown.If Line 282 fails, the function returns after closing the socket but skips destroying
server.cvandserver.lock. Reusingcleanup:here would keep the new init/destroy pairs balanced and avoid another special-case exit.Suggested fix
ret = pthread_create(&server.thread, NULL, chunked_server_thread, &server); TEST_CHECK(ret == 0); if (ret != 0) { close(server.listen_fd); - return; + server.listen_fd = -1; + goto cleanup; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/runtime/http_client_chunked.c` around lines 282 - 287, The pthread_create() failure handling in chunked_server_thread setup closes server.listen_fd and returns early, skipping destruction of server.cv and server.lock; change the early-return path (the block after pthread_create(&server.thread, NULL, chunked_server_thread, &server)) to route through the existing cleanup: label instead of returning directly — e.g., set an error/ret value if needed and goto cleanup so the teardown that destroys server.cv and server.lock always runs; reference pthread_create, server.thread, chunked_server_thread, server.cv, server.lock and the cleanup: label when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/runtime/http_client_chunked.c`:
- Around line 305-330: The test currently only verifies payload reassembly after
calling flb_http_client and flb_http_do (using client and client->resp) and
drops assertions that the chunked response trailers were parsed; restore
assertions that validate the trailers (e.g., check client->resp.trailer or
client->resp.headers for "Expires" and "X-Trace" and their values) so the test
covers trailer parsing as well as payload reassembly, or if those header checks
were flaky, move them into a new focused test that exercises trailer parsing
separately while keeping this test focused on payload reassembly.
- Around line 191-219: The DNS lookup context set by
flb_net_dns_ctx_set(&ctx->dns_ctx) must be cleaned up on every early return; add
calls to flb_net_dns_lookup_context_cleanup() followed by
flb_net_dns_ctx_set(NULL) in the failure branches for TEST_CHECK(ctx->config !=
NULL), the flb_upstream_create failure (ctx->u == NULL), and the
flb_upstream_conn_get failure (ctx->u_conn == NULL), ensuring you perform the
cleanup before destroying the event loop and freeing ctx to mirror the pattern
used in flb_engine.c/flb_input_thread.c.
---
Nitpick comments:
In `@tests/runtime/http_client_chunked.c`:
- Around line 282-287: The pthread_create() failure handling in
chunked_server_thread setup closes server.listen_fd and returns early, skipping
destruction of server.cv and server.lock; change the early-return path (the
block after pthread_create(&server.thread, NULL, chunked_server_thread,
&server)) to route through the existing cleanup: label instead of returning
directly — e.g., set an error/ret value if needed and goto cleanup so the
teardown that destroys server.cv and server.lock always runs; reference
pthread_create, server.thread, chunked_server_thread, server.cv, server.lock and
the cleanup: label when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 49c63fef-fbc2-45e8-b578-9d3ffd4acf13
📒 Files selected for processing (1)
tests/runtime/http_client_chunked.c
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c4bb36229
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| flb_net_dns_ctx_init(); | ||
| flb_net_ctx_init(&ctx->dns_ctx); | ||
| flb_net_dns_ctx_set(&ctx->dns_ctx); |
There was a problem hiding this comment.
After flb_net_dns_ctx_set(&ctx->dns_ctx) stores a thread-local pointer, the later return NULL branches for upstream creation/connection failure exit after flb_free(ctx) without unsetting or cleaning that DNS context. If either branch is hit (e.g., allocation/socket setup failure), the thread-local DNS pointer becomes dangling and can be dereferenced by later network operations in the same test thread, causing follow-on crashes or flaky failures.
Useful? React with 👍 / 👎.
| cleanup: | ||
| if (value != NULL) { | ||
| TEST_CHECK(strcmp(value, "tomorrow") == 0); | ||
| flb_sds_destroy(value); | ||
| } |
There was a problem hiding this comment.
This cleanup-label move removed the previous checks for X-Trace and Expires trailer values, so the test now succeeds even when chunked trailer parsing/regression occurs. Because this test specifically serves runtime coverage for chunked responses, dropping trailer validation weakens its signal and can let trailer-handling bugs slip through CI unnoticed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/runtime/http_client_chunked.c (2)
273-276:⚠️ Potential issue | 🟡 MinorClean up mutex/cond on pthread_create failure
If
pthread_createfails, the return path at Line 275 skipspthread_cond_destroyandpthread_mutex_destroyfor objects initialized at Lines 259-260.Proposed fix
ret = pthread_create(&server.thread, NULL, chunked_server_thread, &server); TEST_CHECK(ret == 0); if (ret != 0) { close(server.listen_fd); + pthread_cond_destroy(&server.cv); + pthread_mutex_destroy(&server.lock); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/runtime/http_client_chunked.c` around lines 273 - 276, The failure path after pthread_create currently returns after closing server.listen_fd but skips destroying the pthread_cond_t and pthread_mutex_t that were initialized earlier; update the error path (the branch where pthread_create returns non-zero) to call pthread_cond_destroy on the condition variable and pthread_mutex_destroy on the mutex (the pthread_cond_t and pthread_mutex_t initialized just before the pthread_create call) before closing server.listen_fd and returning so resources are always cleaned up on pthread_create failure.
182-211:⚠️ Potential issue | 🔴 CriticalReset DNS context on all create() failure exits after Line 182
After
flb_net_dns_ctx_set(&ctx->dns_ctx), the failure branches at Line 195 and Line 205 freectxwithout unsetting/cleaning DNS context. This can leave stale global DNS state pointing to freed memory for subsequent tests.Proposed fix
static struct runtime_http_client_ctx *runtime_http_client_ctx_create(int port) { struct runtime_http_client_ctx *ctx; + int dns_ctx_set; ctx = flb_calloc(1, sizeof(struct runtime_http_client_ctx)); @@ + dns_ctx_set = FLB_FALSE; flb_engine_evl_init(); flb_engine_evl_set(ctx->evl); flb_net_dns_ctx_init(); flb_net_ctx_init(&ctx->dns_ctx); flb_net_dns_ctx_set(&ctx->dns_ctx); + dns_ctx_set = FLB_TRUE; @@ if (!TEST_CHECK(ctx->config != NULL)) { - flb_net_dns_ctx_set(NULL); + flb_net_dns_lookup_context_cleanup(&ctx->dns_ctx); + flb_net_dns_ctx_set(NULL); mk_event_loop_destroy(ctx->evl); flb_free(ctx); return NULL; } @@ if (!TEST_CHECK(ctx->u != NULL)) { + if (dns_ctx_set) { + flb_net_dns_lookup_context_cleanup(&ctx->dns_ctx); + flb_net_dns_ctx_set(NULL); + } flb_config_exit(ctx->config); mk_event_loop_destroy(ctx->evl); flb_free(ctx); return NULL; } @@ if (!TEST_CHECK(ctx->u_conn != NULL)) { flb_upstream_destroy(ctx->u); + if (dns_ctx_set) { + flb_net_dns_lookup_context_cleanup(&ctx->dns_ctx); + flb_net_dns_ctx_set(NULL); + } flb_config_exit(ctx->config); mk_event_loop_destroy(ctx->evl); flb_free(ctx); return NULL; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/runtime/http_client_chunked.c` around lines 182 - 211, After calling flb_net_dns_ctx_set(&ctx->dns_ctx) you must unset the global DNS context on every failure path that frees ctx; update the error branches that follow flb_config_init (where ctx is freed), flb_upstream_create failure, and flb_upstream_conn_get failure to call flb_net_dns_ctx_set(NULL) before destroying/cleaning and returning. Locate the branches that call flb_config_exit, flb_upstream_destroy, mk_event_loop_destroy, flb_free(ctx) and insert flb_net_dns_ctx_set(NULL) prior to those cleanup steps so the global DNS pointer is never left pointing at freed memory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/runtime/http_client_chunked.c`:
- Around line 273-276: The failure path after pthread_create currently returns
after closing server.listen_fd but skips destroying the pthread_cond_t and
pthread_mutex_t that were initialized earlier; update the error path (the branch
where pthread_create returns non-zero) to call pthread_cond_destroy on the
condition variable and pthread_mutex_destroy on the mutex (the pthread_cond_t
and pthread_mutex_t initialized just before the pthread_create call) before
closing server.listen_fd and returning so resources are always cleaned up on
pthread_create failure.
- Around line 182-211: After calling flb_net_dns_ctx_set(&ctx->dns_ctx) you must
unset the global DNS context on every failure path that frees ctx; update the
error branches that follow flb_config_init (where ctx is freed),
flb_upstream_create failure, and flb_upstream_conn_get failure to call
flb_net_dns_ctx_set(NULL) before destroying/cleaning and returning. Locate the
branches that call flb_config_exit, flb_upstream_destroy, mk_event_loop_destroy,
flb_free(ctx) and insert flb_net_dns_ctx_set(NULL) prior to those cleanup steps
so the global DNS pointer is never left pointing at freed memory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0f176ff0-149c-4c5b-900c-a3b9337d6255
📒 Files selected for processing (1)
tests/runtime/http_client_chunked.c
|
this was already fixed in master. |
1 participant
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit