downstream: add coroutine-based event dispatch#12146
Conversation
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (21)
🚧 Files skipped from review as they are similar to previous changes (20)
📝 WalkthroughWalkthroughAdds downstream-owned event coroutines and deferred connection teardown for Forward TLS connections. Engine and worker dispatch resume these coroutines, asynchronous I/O propagates network and restoration errors, and integration tests cover partial records, shutdown, protocol errors, pause handling, and worker timeouts. ChangesTLS event coroutine lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant TLSClient
participant fw_conn_add
participant Downstream
participant TLSIO
participant Output
TLSClient->>fw_conn_add: establish TLS connection
fw_conn_add->>Downstream: register event callback
Downstream->>TLSIO: resume asynchronous read/write
TLSIO-->>Downstream: deliver data or net_error
Downstream->>Output: forward decoded records
Downstream-->>TLSClient: close or wake timed-out connection
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c08d3a6339
ℹ️ 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".
| if (flb_stream_is_thread_safe(&stream->base)) { | ||
| resume_pending_event_coroutines(stream); |
There was a problem hiding this comment.
Avoid resuming worker coroutines from the engine thread
When a timed-out downstream belongs to a threaded input or downstream worker, this branch resumes its connection coroutine directly from whichever thread called flb_downstream_conn_timeouts_stream(). That function is also reached by the main engine timer via flb_downstream_conn_timeouts(&ctx->downstreams), and worker/threaded downstreams are inserted into that global list during setup, so a Forward TLS worker timeout can resume a coroutine and touch its worker event-loop state from the engine thread instead of the owning worker/input thread. Please only do the direct resume when the caller is the stream's owner, or wake the owner event loop instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/in_forward/fw_conn.c (1)
253-258: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the required Allman brace style in edited control blocks.
plugins/in_forward/fw_conn.c#L253-L258: moveif/elseopening braces to their own lines.plugins/in_forward/fw_conn.c#L360-L362: move theifopening brace to its own line.src/flb_downstream_worker.c#L213-L225: update theif/elseblocks.src/flb_engine.c#L1295-L1300: update theif/elseblocks.src/flb_input_thread.c#L462-L467: update theif/elseblocks.src/tls/flb_tls.c#L414-L417,#L429-L434,#L533-L538,#L549-L554,#L579-L584: update each error branch.Proposed pattern
- if (condition) { + if (condition) + {As per coding guidelines, C control-block opening braces must be on the next line.
🤖 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_forward/fw_conn.c` around lines 253 - 258, Apply Allman brace formatting to the edited control blocks: place every if and else opening brace on its own line at plugins/in_forward/fw_conn.c lines 253-258 and 360-362; src/flb_downstream_worker.c lines 213-225; src/flb_engine.c lines 1295-1300; src/flb_input_thread.c lines 462-467; and each listed error branch in src/tls/flb_tls.c lines 414-417, 429-434, 533-538, 549-554, and 579-584. Preserve all existing control flow and behavior.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.
Nitpick comments:
In `@plugins/in_forward/fw_conn.c`:
- Around line 253-258: Apply Allman brace formatting to the edited control
blocks: place every if and else opening brace on its own line at
plugins/in_forward/fw_conn.c lines 253-258 and 360-362;
src/flb_downstream_worker.c lines 213-225; src/flb_engine.c lines 1295-1300;
src/flb_input_thread.c lines 462-467; and each listed error branch in
src/tls/flb_tls.c lines 414-417, 429-434, 533-538, 549-554, and 579-584.
Preserve all existing control flow and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5bf89f17-3a58-413a-abb0-45e6851bc91b
📒 Files selected for processing (13)
include/fluent-bit/flb_connection.hinclude/fluent-bit/flb_downstream.hplugins/in_forward/fw_conn.csrc/flb_connection.csrc/flb_downstream.csrc/flb_downstream_worker.csrc/flb_engine.csrc/flb_input_thread.csrc/tls/flb_tls.ctests/integration/scenarios/in_forward/config/in_forward_tls_pause.yamltests/integration/scenarios/in_forward/config/in_forward_tls_threaded.yamltests/integration/scenarios/in_forward/config/in_forward_tls_workers_timeout.yamltests/integration/scenarios/in_forward/tests/test_in_forward_001.py
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
8078710 to
ede95a2
Compare
Purpose
Add coroutine-based event dispatch to the downstream core and use Forward as
the first end-to-end consumer for both TLS and plain stream connections.
This PR is based on
master(cb7256c28), replaces #11547, and fixes #11551.Instead of bounding TLS retry loops in plugin code, it fixes the downstream
execution model: callbacks can suspend on read/write readiness and resume when
the connection has data, reaches EOF, fails, times out, or is cancelled.
Design
core abstraction.
WANT_READ,WANT_WRITE, and retry sentinels.restores its original type, mask, priority, and handler after completion.
unwind the wrapper, event, TLS session, and coroutine safely.
callback returns, preventing use-after-free during backpressure pause.
intentionally outside this interface.
There is no retry cap when
net.io_timeoutis zero. The coroutine is dormantuntil descriptor readiness or cancellation, so indefinite waits do not spin.
Configured idle timeouts wake and terminate the pending operation.
Forward integration
Forward TLS and plain TCP now use the downstream dispatch interface. Unrelated
server inputs remain unchanged and can migrate separately after this core path
is proven.
The plain-TCP event ownership changes internally, but its protocol behavior and
configuration remain compatible.
Coroutine stack safety
The default coroutine stack now has a 64 KiB minimum while preserving explicit
FLB_CORO_STACK_SIZEoverrides and larger platform defaults.The former glibc-derived default could be only 24 KiB. That is insufficient
headroom for the full parser/processor ingestion path now reachable from a
downstream callback; historical yyjson parser frames alone have approached
38 KiB. The same safe default applies consistently to upstream, downstream,
input, output, and scheduler coroutines.
On systems that previously selected 24 KiB, the cost is approximately 40 KiB
of additional virtual stack reservation per coroutine.
Integration coverage
The focused Forward matrix contains 24 downstream-coroutine scenarios, each
run normally and under strict Valgrind:
The timeout and pause tests assert that the connection wrapper is dropped
before process shutdown, so they cannot pass by leaking until teardown.
Result across the final matrix: 24 passed.
Result across the final matrix: 24 passed with strict Valgrind.
Core validation
flb-it-network,flb-it-downstream_worker, andflb-it-upstream_tlspassed.
flb-rt-in_forwardcould not bind port 24224 because the hostfluentd.serviceowns it; that service was not disrupted.The full PR range passes commit-prefix lint, DCO, and
git diff --check.Windows x86, x64, and ARM64 builds are covered by PR CI because a local MSVC
toolchain is unavailable.
Compatibility
net.io_timeout: 0retains indefinite, readiness-driven waiting.Related
Summary by CodeRabbit