io: http_client: Implement batching io write#11501
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a public Changes
Sequence DiagramsequenceDiagram
participant HTTP as HTTP Client
participant IO as I/O Layer
participant Conn as Connection
HTTP->>IO: flb_io_net_writev(connection, iov[], iovcnt, &out_len)
Note over IO: validate inputs\ncheck iovcnt vs IOV_MAX\ncompute total size (overflow checks)
IO->>IO: allocate temporary buffer\nconcatenate iov data into buffer
IO->>Conn: flb_io_net_write(connection, buffer, total_len)
Conn-->>IO: bytes_written / error
IO->>IO: free buffer
IO-->>HTTP: return result, out_len
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 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 |
![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: e5b09d6a8a
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/fluent-bit/flb_io.h`:
- Around line 23-36: The Windows build fails because the iovec alias is only
defined when a preexisting iovec macro is present; move or add an unconditional
alias so that after defining struct flb_iovec (inside the FLB_SYSTEM_WINDOWS
guard) you always map the symbol iovec to flb_iovec (i.e., ensure iovec is
defined/aliased to flb_iovec regardless of whether a prior iovec macro exists)
so uses of const struct iovec *iov compile on MSVC; update the code around the
FLB_SYSTEM_WINDOWS block to `#undef` iovec if needed and then `#define` iovec
flb_iovec unconditionally after the struct definition.
In `@src/flb_http_client.c`:
- Around line 1494-1509: The current code hard-fails when
flb_io_net_writev(c->u_conn, io_vector, 2, &bytes_header) returns -1; change
this to try a graceful fallback: on flb_io_net_writev failure, log the error
(flb_errno()) but then attempt two separate writes using the underlying
single-write API (e.g., call flb_io_net_write(c->u_conn, c->header_buf,
c->header_len, &bytes_written) and then flb_io_net_write(c->u_conn, c->body_buf,
c->body_len, &bytes_written) or equivalent), and only return FLB_HTTP_ERROR if
those fallback writes fail; keep existing success path when writev succeeds and
preserve bytes_header/bytes_written handling.
- Around line 1511-1513: The subtraction bytes_body = bytes_header -
c->header_len can underflow because bytes_header and c->header_len are size_t;
before doing the subtraction (in the block that updates bytes_body and
bytes_header) add a guard that verifies bytes_header >= c->header_len and handle
the else-case (e.g., set bytes_body = 0 and keep bytes_header unchanged or clamp
to 0 and log/return an error); update references to bytes_body and bytes_header
accordingly so no unsigned wraparound occurs when partial writes happen.
cosmo0920
force-pushed
the
cosmo0920-implement-batching-io_write
branch
from
e5b09d6 to
39a6a71
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/flb_io.c`:
- Around line 735-738: The check allows iov[index].iov_base to be NULL when
iov[index].iov_len == 0 but later still calls memcpy with that NULL source;
update the code path around the memcpy call to skip calling memcpy when
iov[index].iov_len == 0 (i.e., only call memcpy when iov[index].iov_len > 0),
ensuring you keep the existing validation that sets errno = EINVAL when iov_len
> 0 and iov_base == NULL; refer to the iov[index].iov_len / iov[index].iov_base
checks and the memcpy invocation to locate and change the logic.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
include/fluent-bit/flb_io.hsrc/flb_io.c
🚧 Files skipped from review as they are similar to previous changes (1)
- include/fluent-bit/flb_io.h
cosmo0920
force-pushed
the
cosmo0920-implement-batching-io_write
branch
from
39a6a71 to
fc849bf
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/flb_http_client.c (1)
1494-1538:⚠️ Potential issue | 🟠 MajorGuard against unsigned underflow in byte accounting.
At line 1536,
bytes_body = bytes_header - c->header_lenperforms subtraction onsize_tvalues. If the underlying write completes only partially (a valid scenario in network I/O),bytes_headercould be less thanc->header_len, causing unsigned wraparound to a huge value.Proposed defensive check
else { + if (bytes_header < c->header_len) { + flb_error("[http_client] partial write: total=%zu header_len=%zu", + bytes_header, c->header_len); + return FLB_HTTP_ERROR; + } bytes_body = bytes_header - c->header_len; bytes_header = c->header_len; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flb_http_client.c` around lines 1494 - 1538, The subtraction bytes_body = bytes_header - c->header_len can underflow because bytes_header may be less than c->header_len after a partial write from flb_io_net_writev; modify the post-write branch that currently sets bytes_body and bytes_header to first check if bytes_header >= c->header_len and only then compute bytes_body = bytes_header - c->header_len and set bytes_header = c->header_len, otherwise treat it as a partial-header write by setting bytes_body = 0 and leaving bytes_header as the actual bytes_header (or handle as appropriate), so no unsigned underflow occurs (refer to flb_io_net_writev, bytes_header, bytes_body, and c->header_len).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/fluent-bit/flb_io.h`:
- Around line 69-72: The function declaration flb_io_net_writev currently uses
"const struct flb_iovec *iov" which will not compile on POSIX because flb_iovec
is a typedef (no struct tag); change the declaration to use the typedef name
"const flb_iovec *iov" and remove the "struct" keyword from all uses of
flb_iovec (including the implementation in src/flb_io.c where flb_io_net_writev
is defined and callers such as the io_vector[] in src/flb_http_client.c) so
every reference uses the typedef type name flb_iovec rather than struct
flb_iovec.
In `@src/flb_http_client.c`:
- Line 1495: The declaration uses "struct flb_iovec io_vector[2];" which fails
on POSIX because flb_iovec is a typedef alias to struct iovec; change the
declaration to use the typedef name directly (flb_iovec io_vector[2];) and
remove the "struct" keyword anywhere "struct flb_iovec" appears (e.g., local
variable io_vector in flb_http_client.c and any other occurrences referenced by
the same header) so the code compiles on POSIX.
---
Duplicate comments:
In `@src/flb_http_client.c`:
- Around line 1494-1538: The subtraction bytes_body = bytes_header -
c->header_len can underflow because bytes_header may be less than c->header_len
after a partial write from flb_io_net_writev; modify the post-write branch that
currently sets bytes_body and bytes_header to first check if bytes_header >=
c->header_len and only then compute bytes_body = bytes_header - c->header_len
and set bytes_header = c->header_len, otherwise treat it as a partial-header
write by setting bytes_body = 0 and leaving bytes_header as the actual
bytes_header (or handle as appropriate), so no unsigned underflow occurs (refer
to flb_io_net_writev, bytes_header, bytes_body, and c->header_len).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
include/fluent-bit/flb_io.hsrc/flb_http_client.csrc/flb_io.c
🚧 Files skipped from review as they are similar to previous changes (1)
- src/flb_io.c
cosmo0920
force-pushed
the
cosmo0920-implement-batching-io_write
branch
from
fc849bf to
85cbf67
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/flb_io.c (1)
714-718:⚠️ Potential issue | 🟠 MajorAdd missing
connectionnull-guard in input validation.Line 714 validates several inputs but not
connection. A nullconnectioncan flow intoflb_io_net_write()and dereference-crash.💡 Suggested fix
- if (iov == NULL || iovcnt <= 0 || out_len == NULL) { + if (connection == NULL || iov == NULL || iovcnt <= 0 || out_len == NULL) { errno = EINVAL; return -1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flb_io.c` around lines 714 - 718, Add a null-guard for the connection pointer in the input validation so a NULL connection cannot be passed into flb_io_net_write() and cause a dereference crash: in the validation block that currently checks iov, iovcnt and out_len (around the start of the function in flb_io.c) include connection == NULL (or a dedicated if that sets errno = EINVAL and returns -1) so the function returns early on a NULL connection with the same error handling path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/flb_io.c`:
- Around line 714-718: Add a null-guard for the connection pointer in the input
validation so a NULL connection cannot be passed into flb_io_net_write() and
cause a dereference crash: in the validation block that currently checks iov,
iovcnt and out_len (around the start of the function in flb_io.c) include
connection == NULL (or a dedicated if that sets errno = EINVAL and returns -1)
so the function returns early on a NULL connection with the same error handling
path.
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
cosmo0920
force-pushed
the
cosmo0920-implement-batching-io_write
branch
from
85cbf67 to
0534987
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is internal component change and optimization.
When sending HTTP request, we need to send header and body pair if body length is greater than zero.
However, this can be concatenated with one request if possible.
So, we can concatenate with memcpy to reduce calling of write syscall.
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
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
New Features
Bug Fixes