in_lib: Adjust consuming mechanism to improve throughput#12130
in_lib: Adjust consuming mechanism to improve throughput#12130cosmo0920 wants to merge 3 commits into
Conversation
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
📝 WalkthroughWalkthroughThe Changesin_lib input handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant InputChannel
participant InLibCollector
participant Pipe
InputChannel->>InLibCollector: invoke collection
InLibCollector->>Pipe: read available bytes
Pipe-->>InLibCollector: bytes or terminal condition
InLibCollector->>InLibCollector: grow buffer and retain unconsumed data
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
plugins/in_lib/in_lib.c (1)
52-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueApply the mandated C brace style.
The changed control blocks use same-line opening braces, while the supplied guideline requires opening braces on the next line. As per coding guidelines, C control blocks must place opening braces on the next line.
Also applies to: 237-253
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/in_lib/in_lib.c` around lines 52 - 93, Update the control blocks in the affected read loop and the corresponding blocks around the referenced additional section to place each opening brace on the next line, including the while, if, else-if, and non-Windows conditional blocks. Preserve the existing logic and indentation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/in_lib/in_lib.c`:
- Around line 52-92: Update the file-descriptor setup for ctx->fd so the POSIX
pipe is also configured nonblocking, not only under FLB_SYSTEM_WINDOWS. Ensure
this setup occurs before the read loop in the surrounding initialization flow,
allowing the existing drain loop to continue until FLB_PIPE_WOULDBLOCK() on
non-Windows platforms.
- Around line 76-80: Update the error check in the read handling path to compare
errno directly with EPIPE rather than its negated value. Preserve the existing
-1 return for broken-pipe errors so they do not fall through to the successful
return path.
---
Nitpick comments:
In `@plugins/in_lib/in_lib.c`:
- Around line 52-93: Update the control blocks in the affected read loop and the
corresponding blocks around the referenced additional section to place each
opening brace on the next line, including the while, if, else-if, and
non-Windows conditional blocks. Preserve the existing logic and indentation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8329dfad-f7cd-477e-a56d-eec73fc2dea0
📒 Files selected for processing (2)
plugins/in_lib/in_lib.cplugins/in_lib/in_lib.h
![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: 09c41bf1b4
ℹ️ 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".
db39b14 to
5a573de
Compare
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
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)
plugins/in_lib/in_lib.c (2)
71-81: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPropagate all non-would-block read errors.
After handling
FLB_PIPE_WOULDBLOCK(), every otherread()failure should remain an error. Returning0for errors other thanEPIPEsilently treats failures such asEBADForEIOas successful no-data reads; return-1instead, unless a specific errno has an explicitly documented benign meaning.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/in_lib/in_lib.c` around lines 71 - 81, Update the bytes == -1 error path in the pipe-reading function to return -1 for every non-would-block read failure, including errors other than EPIPE. Preserve the existing FLB_PIPE_WOULDBLOCK() break behavior and perror/flb_pipe_error handling.
52-65: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftBound the retained partial payload, not only each read.
total_bytesresets on every callback, whilectx->buf_lenremains populated when decoding returnsFLB_ERR_JSON_PART. An unterminated or malformed stream can therefore growctx->buf_dataindefinitely across callbacks and exhaust memory. Enforce a maximum onctx->buf_len, cap each read to the remaining capacity, and apply an explicit oversize/drop error path.Also applies to: 100-105
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/in_lib/in_lib.c` around lines 52 - 65, Bound the retained partial payload using ctx->buf_len, not the per-callback total_bytes counter. In the read loop, limit each read to the remaining LIB_MAX_READ_SIZE capacity and reject or drop input when ctx->buf_len reaches or would exceed that limit. Ensure the oversize path reports the error and resets or discards the retained buffer so malformed unterminated streams cannot grow ctx->buf_data across callbacks.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@plugins/in_lib/in_lib.c`:
- Around line 71-81: Update the bytes == -1 error path in the pipe-reading
function to return -1 for every non-would-block read failure, including errors
other than EPIPE. Preserve the existing FLB_PIPE_WOULDBLOCK() break behavior and
perror/flb_pipe_error handling.
- Around line 52-65: Bound the retained partial payload using ctx->buf_len, not
the per-callback total_bytes counter. In the read loop, limit each read to the
remaining LIB_MAX_READ_SIZE capacity and reject or drop input when ctx->buf_len
reaches or would exceed that limit. Ensure the oversize path reports the error
and resets or discards the retained buffer so malformed unterminated streams
cannot grow ctx->buf_data across callbacks.
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
cosmo0920
changed the title
1 participant
Otherwise, Fluent Bit tests can fail as below on Windows:
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