lib: Recreate the event loop after daemonizing on BSD platforms. #11171
Conversation
yuichiro-naito
requested review from
cosmo0920,
edsiper and
niedbalski
as code owners
WalkthroughIntroduces exported APIs Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Lib as libfluent-bit
participant OS as Kernel (BSD)
rect `#DDEFFF`
Note over App,Lib: Startup
App->>Lib: flb_create()
Lib->>Lib: flb_event_loop_create(ctx)
Lib->>OS: create kqueue / event channel
end
rect `#FFEBD9`
Note over App,Lib: Daemonize (BSD)
App->>Lib: flb_main_run(daemonize)
Lib->>Lib: flb_event_loop_destroy(ctx)
Lib->>OS: fork()
OS->>OS: child created (kqueue not inherited)
end
rect `#E6FFE7`
Note over Lib,App: Post-fork recovery
Lib->>Lib: flb_event_loop_create(ctx)
Lib->>OS: recreate kqueue / event channel
Lib->>App: resume startup / signal handling
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
include/fluent-bit/flb_lib.h(1 hunks)src/flb_lib.c(2 hunks)src/fluent-bit.c(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
include/fluent-bit/flb_lib.h (1)
src/flb_lib.c (2)
flb_create_event_loop(143-180)flb_destroy_event_loop(182-205)
src/flb_lib.c (4)
lib/monkey/mk_core/mk_event.c (4)
mk_event_loop_create(61-88)mk_event_channel_create(181-190)mk_event_channel_destroy(193-202)mk_event_loop_destroy(91-96)src/flb_config.c (1)
flb_config_exit(488-672)include/fluent-bit/flb_mem.h (2)
flb_free(126-128)flb_calloc(84-96)lib/monkey/include/monkey/mk_core/mk_event.h (1)
MK_EVENT_ZERO(135-138)
src/fluent-bit.c (2)
src/flb_lib.c (2)
flb_destroy_event_loop(182-205)flb_create_event_loop(143-180)src/flb_utils.c (2)
flb_utils_set_daemon(178-209)flb_utils_error(69-158)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/flb_lib.c (3)
143-187: Good improvements to error handling; consider resettingconfig->ch_evlon failure.The function now properly addresses most critical issues from the previous review:
- NULL validation for
ctxis present.- Error paths correctly clean up allocated resources via goto labels.
- No longer calls
flb_stop()orflb_destroy()during initialization.- Pointers are set to NULL after cleanup.
However,
config->ch_evlis assigned on line 158 but is not reset to NULL in the error paths (lines 183-186). If this function returns an error,config->ch_evlwill point to freed memory, which could be problematic if any code (other than the immediate caller's cleanup) attempts to use it.Consider this adjustment:
error_1: mk_event_loop_destroy(ctx->event_loop); ctx->event_loop = NULL; + config->ch_evl = NULL; return FLB_LIB_ERROR;
163-163: Consider removing or adjusting theperrorcall.
flb_calloccan return NULL without settingerrno(particularly in the OSSFUZZ failure-injection path), soperror("calloc")may print a misleading error message.You can either remove it or replace it with a direct error message:
ctx->event_channel = flb_calloc(1, sizeof(struct mk_event)); if (!ctx->event_channel) { - perror("calloc"); + flb_error("[lib] failed to allocate event_channel"); goto error_1; }
189-218: Good defensive NULL checks; minor consistency improvements possible.The function now properly checks for NULL before freeing/destroying and sets pointers to NULL afterward, addressing the main concerns from the previous review.
A few optional refinements for consistency and defensive programming:
- Line 208 uses
free()instead offlb_free(), which is inconsistent with the rest of the codebase.- Lines 205-206 close file descriptors without checking if they're valid or resetting them afterward, which could be improved for robustness.
Optional improvements:
if (ret != 0) { /* make sure to close file descriptors */ - close(config->ch_notif[0]); - close(config->ch_notif[1]); + if (config->ch_notif[0] >= 0) { + close(config->ch_notif[0]); + config->ch_notif[0] = -1; + } + if (config->ch_notif[1] >= 0) { + close(config->ch_notif[1]); + config->ch_notif[1] = -1; + } } - free(ctx->event_channel); + flb_free(ctx->event_channel); ctx->event_channel = NULL;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/flb_lib.c(2 hunks)src/fluent-bit.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/fluent-bit.c
🧰 Additional context used
🧬 Code graph analysis (1)
src/flb_lib.c (3)
lib/monkey/mk_core/mk_event.c (4)
mk_event_loop_create(61-88)mk_event_channel_create(181-190)mk_event_loop_destroy(91-96)mk_event_channel_destroy(193-202)include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)lib/monkey/include/monkey/mk_core/mk_event.h (1)
MK_EVENT_ZERO(135-138)
🔇 Additional comments (1)
src/flb_lib.c (1)
264-269: LGTM! Clean integration of the new event loop creation API.The refactoring successfully delegates event loop setup to
flb_create_event_loop()and handles failures appropriately by cleaning up the config and context before returning NULL.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
The patches are really good but we need to add Signed-off line on each of commit. |
cosmo0920
changed the title
yuichiro-naito
force-pushed
the
fix/stop_daemon
branch
from
6aa2773 to
403a863
Compare
|
I rebased my patch to master and squashed 2 commits into 1 commit. |
yuichiro-naito
force-pushed
the
fix/stop_daemon
branch
from
403a863 to
971f916
Compare
|
I updated the category in the commit message to 'lib:'. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
include/fluent-bit/flb_lib.h (1)
52-53: LGTM! Consider adding API documentation for these lifecycle functions.The new public APIs are well-designed and correctly solve the BSD kqueue inheritance issue. The function signatures are consistent with the existing API, and the implementations (from the code snippets) show proper error handling and resource cleanup.
However, since these functions expose event loop lifecycle management as part of the public API, brief documentation would help library users understand:
- When these functions should be called (e.g., around fork on BSD platforms)
- Return value semantics (0 for success, FLB_LIB_ERROR/-1 for failure)
- Relationship with
flb_create()/flb_destroy()(typically not needed unless forking)This suggestion is optional since the existing header style generally omits function documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
include/fluent-bit/flb_lib.h(1 hunks)src/flb_lib.c(2 hunks)src/fluent-bit.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/flb_lib.c
- src/fluent-bit.c
🧰 Additional context used
🧬 Code graph analysis (1)
include/fluent-bit/flb_lib.h (1)
src/flb_lib.c (2)
flb_create_event_loop(143-187)flb_destroy_event_loop(189-218)
This file contains 'flb_event_loop_*' functions. Signed-off-by: Yuichiro NAITO <naito.yuichiro@gmail.com>
yuichiro-naito
force-pushed
the
fix/stop_daemon
branch
from
2a09157 to
21f3c59
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/flb_event_loop.c (2)
30-33: Add NULL check forctx->configbefore dereferencing.Line 33 dereferences
ctx->configwithout verifying it's not NULL. Whileflb_create()setsctx->configbefore calling this function, defensive programming would add validation here for safety when the function is called from other contexts (e.g., BSD daemonization path afterflb_event_loop_destroy).if (ctx == NULL) return FLB_LIB_ERROR; + if (ctx->config == NULL) + return FLB_LIB_ERROR; + config = ctx->config;
79-97: Consider clearingconfig->ch_evlfor consistency.In
flb_event_loop_create(),config->ch_evlis assigned the event loop pointer (line 40). For symmetry and to avoid a dangling pointer, consider clearing it in the destroy path.if (ctx->event_loop != NULL) { mk_event_loop_destroy(ctx->event_loop); ctx->event_loop = NULL; + config->ch_evl = NULL; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
include/fluent-bit/flb_lib.h(1 hunks)src/CMakeLists.txt(1 hunks)src/flb_event_loop.c(1 hunks)src/flb_lib.c(1 hunks)src/fluent-bit.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/CMakeLists.txt
- include/fluent-bit/flb_lib.h
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-21T06:23:29.770Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11171
File: include/fluent-bit/flb_lib.h:52-53
Timestamp: 2025-11-21T06:23:29.770Z
Learning: In Fluent Bit core (fluent/fluent-bit repository), function descriptions/documentation are not required for newly added functions in header files.
Applied to files:
src/flb_event_loop.csrc/fluent-bit.c
📚 Learning: 2025-09-04T12:35:22.872Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10825
File: plugins/out_s3/s3.c:1339-1344
Timestamp: 2025-09-04T12:35:22.872Z
Learning: In the Fluent Bit S3 plugin, the user prefers to maintain current retry_limit behavior without special handling for FLB_OUT_RETRY_UNLIMITED (-1), as there's no documentation indicating -1 should be used for infinite retries and consistency with current logic is preferred.
Applied to files:
src/fluent-bit.c
📚 Learning: 2025-09-04T12:35:36.904Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10825
File: plugins/out_s3/s3.c:3275-3282
Timestamp: 2025-09-04T12:35:36.904Z
Learning: The out_s3 plugin intentionally uses a simple numeric comparison for retry_limit (chunk->failures >= ctx->ins->retry_limit) rather than the standard Fluent Bit pattern that checks for FLB_OUT_RETRY_UNLIMITED (-1). The maintainer wants to keep this current behavior for consistency within the plugin.
Applied to files:
src/fluent-bit.c
📚 Learning: 2025-09-04T12:32:46.030Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10825
File: plugins/out_s3/s3.c:0-0
Timestamp: 2025-09-04T12:32:46.030Z
Learning: In Fluent Bit plugins, avoid creating plugin-local config_map entries that duplicate core functionality. The core already provides Retry_Limit parsed into ins->retry_limit, so plugins should use that directly rather than defining their own retry_limit configuration option.
Applied to files:
src/fluent-bit.c
🧬 Code graph analysis (2)
src/flb_lib.c (1)
src/flb_event_loop.c (1)
flb_event_loop_create(25-69)
src/fluent-bit.c (2)
src/flb_event_loop.c (2)
flb_event_loop_destroy(71-100)flb_event_loop_create(25-69)src/flb_utils.c (2)
flb_utils_set_daemon(181-212)flb_utils_error(69-161)
🔇 Additional comments (3)
src/flb_lib.c (1)
187-192: Clean refactoring of event loop creation.The delegation to
flb_event_loop_create(ctx)properly encapsulates the event loop setup logic. Error handling correctly cleans upctx->configandctxbefore returning NULL, maintaining the original behavior.src/flb_event_loop.c (1)
25-69: Event loop creation implementation looks solid.The function properly:
- Uses goto-based cleanup for error paths
- Frees allocated resources and nulls pointers on failure
- Returns appropriate error codes
The error handling pattern is clean and prevents resource leaks.
src/fluent-bit.c (1)
1418-1429: BSD daemonization fix is correct and verified.The ch_evl synchronization is properly handled:
- Parent process exits at line 191 of
flb_utils.cimmediately after fork- Child process continues and reassigns
config->ch_evlinflb_event_loop_create()before any use- No dangling pointer issue exists because the parent terminates before it could reference the destroyed event loop
Set NULL for the 'ch_evl' member after destroying. Signed-off-by: Yuichiro NAITO <naito.yuichiro@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/flb_event_loop.c (1)
36-36: Consider defining a named constant for the event loop size.The magic number
256for the event loop size reduces readability. Consider defining a constant likeFLB_EVENT_LOOP_SIZEor similar to make the code more maintainable.Example:
+#define FLB_EVENT_LOOP_SIZE 256 + int flb_event_loop_create(flb_ctx_t *ctx) { ... - ctx->event_loop = mk_event_loop_create(256); + ctx->event_loop = mk_event_loop_create(FLB_EVENT_LOOP_SIZE);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/flb_event_loop.c(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-21T06:23:29.770Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11171
File: include/fluent-bit/flb_lib.h:52-53
Timestamp: 2025-11-21T06:23:29.770Z
Learning: In Fluent Bit core (fluent/fluent-bit repository), function descriptions/documentation are not required for newly added functions in header files.
Applied to files:
src/flb_event_loop.c
🔇 Additional comments (3)
src/flb_event_loop.c (3)
1-24: LGTM! Includes are appropriate.The header includes all necessary dependencies: flb_lib.h for context types, flb_mem.h for memory functions, and unistd.h for close().
85-89: Excellent defensive cleanup for the BSD kqueue issue.The manual
close()calls whenmk_event_channel_destroyfails are essential for the BSD fork scenario. When the kqueue becomes invalid after daemonizing,mk_event_channel_destroywill fail but the file descriptors still need to be closed to prevent leaks.
71-101: Consider propagating destroy failures to the caller.The function always returns
0(success) even whenmk_event_channel_destroyfails (line 85). While best-effort cleanup is often appropriate for destroy functions, returning success might hide issues from callers who expect to be notified of cleanup problems. Consider whether callers need to know about partial failures.If error propagation is desired, you could:
int flb_event_loop_destroy(flb_ctx_t *ctx) { int ret; + int failed = 0; struct flb_config *config; if (ctx == NULL || ctx->config == NULL) return 0; config = ctx->config; if (ctx->event_channel != NULL) { ret = mk_event_channel_destroy(config->ch_evl, config->ch_notif[0], config->ch_notif[1], ctx->event_channel); if (ret != 0) { /* make sure to close file descriptors */ close(config->ch_notif[0]); close(config->ch_notif[1]); + failed = 1; } flb_free(ctx->event_channel); ctx->event_channel = NULL; } if (ctx->event_loop != NULL) { mk_event_loop_destroy(ctx->event_loop); ctx->event_loop = NULL; config->ch_evl = NULL; } - return 0; + return failed ? FLB_LIB_ERROR : 0; }
Signed-off-by: Yuichiro NAITO <naito.yuichiro@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/flb_event_loop.c (1)
80-90: Optionally logmk_event_channel_destroyfailures for easier debuggingIn
flb_event_loop_destroy, ifmk_event_channel_destroyfails you close the FDs directly, which is good for resource safety, but the failure is silent. Given how tricky kqueue/daemonization issues can be, a log here would help operational debugging without changing behavior:if (ctx->event_channel != NULL) { ret = mk_event_channel_destroy(config->ch_evl, config->ch_notif[0], config->ch_notif[1], ctx->event_channel); if (ret != 0) { - /* make sure to close file descriptors */ + flb_error("[lib] could not destroy notification channels, closing fds directly"); + /* make sure to close file descriptors */ close(config->ch_notif[0]); close(config->ch_notif[1]); }Purely a diagnostics improvement; feel free to skip if you want to keep teardown quieter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/flb_event_loop.c(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-21T06:23:29.770Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11171
File: include/fluent-bit/flb_lib.h:52-53
Timestamp: 2025-11-21T06:23:29.770Z
Learning: In Fluent Bit core (fluent/fluent-bit repository), function descriptions/documentation are not required for newly added functions in header files.
Applied to files:
src/flb_event_loop.c
Signed-off-by: Yuichiro NAITO <naito.yuichiro@gmail.com>
Because of compatibility for Windows. Signed-off-by: Yuichiro NAITO <naito.yuichiro@gmail.com>
Signed-off-by: Yuichiro NAITO <naito.yuichiro@gmail.com>
|
I decided to update the monkey library to plug descriptor leaks in an error case. |
3 participants
Recreate the event loop after daemonizing on BSD platforms.
Because kqueue(2) is not inherited by the child process.
Before reopening, we need to close event channel sockets that were initially created, so as not to leak the sockets. However, the libmonkey interface
mk_event_channel_destroytries to delete sockets from the kqueue. It will fail after daemonizing. We have to callmk_event_channel_destroybefore daemonizing.Fixes #11170
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
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.