lib: monkey: prevent libevent timer socket deadlock - #12241
Conversation
Make the timer notification socket nonblocking so its callback cannot stall the event-loop thread when notifications accumulate. Treat a full socket as an already-pending notification while preserving fatal socket error handling. Add a Windows regression test that fills the notification socket and verifies the timer remains active after backpressure. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: zshuang0316 <zshuang0316@163.com>
📝 WalkthroughWalkthroughThe event implementation now uses nonblocking timeout-pipe writes with platform-specific would-block handling. Windows tests cover pipe backpressure, timeout event delivery, and cleanup. ChangesTimeout notification handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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
🤖 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 `@lib/monkey/mk_core/mk_event_libevent.c`:
- Around line 321-326: Update the control-block formatting so opening braces
appear on the next line: change each affected if block in
lib/monkey/mk_core/mk_event_libevent.c at lines 321-326 and 359-364, and the do
block in lib/monkey/test/event_timeout.c at lines 109-111. Preserve all existing
control flow and behavior.
- Around line 319-324: Update the timeout notification send path around
mk_event_libevent_socketpair and the send call to handle partial writes: retry
until sizeof(val) bytes are queued, preserving progress across would-block
results as needed. Close or otherwise clean up the pipe on non-would-block send
errors, and ensure the consumer’s recv(MSG_WAITALL) cannot block on an
incomplete notification, reusing the framing behavior from
_mk_event_channel_create where appropriate.
🪄 Autofix
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 Plus
Run ID: 5fcb5419-704b-4ed7-a867-6418d784df36
📒 Files selected for processing (2)
lib/monkey/mk_core/mk_event_libevent.clib/monkey/test/event_timeout.c
| ret = send(ev_map->pipe[1], (char *) &val, sizeof(uint64_t), 0); | ||
|
|
||
| if (ret == -1) { | ||
| if (evutil_socket_geterror(fd) != ERR(ECONNABORTED)) { | ||
| error = evutil_socket_geterror(ev_map->pipe[1]); | ||
| if (MK_EVENT_LIBEVENT_WOULD_BLOCK(error)) { | ||
| return; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline lib/monkey/mk_core/mk_event_libevent.c --items all --type function
rg -n -C 16 '\bmk_event_libevent_socketpair\s*\(' \
lib/monkey/mk_core/mk_event_libevent.c
rg -n -C 8 'SOCK_(STREAM|DGRAM|SEQPACKET)|socketpair|accept|connect' \
lib/monkey/mk_core/mk_event_libevent.c
rg -n -C 6 'recv\s*\(|MSG_WAITALL|read\s*\(' \
lib/monkey/mk_server/mk_server.cRepository: fluent/fluent-bit
Length of output: 10223
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '312,400p' lib/monkey/mk_core/mk_event_libevent.c
sed -n '450,578p' lib/monkey/mk_server/mk_server.c
rg -n -C 4 'mk_win32_socketpair' lib
fd -i 'mk_win32_socketpair'Repository: fluent/fluent-bit
Length of output: 15382
Handle partial timeout notifications before calling the event handler.
mk_event_libevent_socketpair() creates SOCK_STREAM endpoints on Unix and SOCK_STREAM loopback AF_UNIX/loopback pairs on Windows. A positive ret less than sizeof(val) means the consumer can still block in recv(..., MSG_WAITALL). Retry the send() result until all bytes are queued, close on non-would-block errors, and/or add the same framing handling that the _mk_event_channel_create() consumers use.
🤖 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 `@lib/monkey/mk_core/mk_event_libevent.c` around lines 319 - 324, Update the
timeout notification send path around mk_event_libevent_socketpair and the send
call to handle partial writes: retry until sizeof(val) bytes are queued,
preserving progress across would-block results as needed. Close or otherwise
clean up the pipe on non-would-block send errors, and ensure the consumer’s
recv(MSG_WAITALL) cannot block on an incomplete notification, reusing the
framing behavior from _mk_event_channel_create where appropriate.
| if (ret == -1) { | ||
| if (evutil_socket_geterror(fd) != ERR(ECONNABORTED)) { | ||
| error = evutil_socket_geterror(ev_map->pipe[1]); | ||
| if (MK_EVENT_LIBEVENT_WOULD_BLOCK(error)) { | ||
| return; | ||
| } | ||
| if (error != ERR(ECONNABORTED)) { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the required brace style in the changed control blocks.
lib/monkey/mk_core/mk_event_libevent.c#L321-L326: Put eachifopening brace on the next line.lib/monkey/mk_core/mk_event_libevent.c#L359-L364: Put theifopening brace on the next line.lib/monkey/test/event_timeout.c#L109-L111: Put thedoopening brace on the next line.
As per coding guidelines, **/*.{c,h,cc,cpp,cxx} requires control-block opening braces on the next line.
📍 Affects 2 files
lib/monkey/mk_core/mk_event_libevent.c#L321-L326(this comment)lib/monkey/mk_core/mk_event_libevent.c#L359-L364lib/monkey/test/event_timeout.c#L109-L111
🤖 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 `@lib/monkey/mk_core/mk_event_libevent.c` around lines 321 - 326, Update the
control-block formatting so opening braces appear on the next line: change each
affected if block in lib/monkey/mk_core/mk_event_libevent.c at lines 321-326 and
359-364, and the do block in lib/monkey/test/event_timeout.c at lines 109-111.
Preserve all existing control flow and behavior.
Source: Coding guidelines
![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: 54862875a3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| fd = mk_event_timeout_create(evl, 0, 1000, &ev); | ||
| TEST_ASSERT(fd >= 0); | ||
|
|
||
| event_map = ev.data; |
There was a problem hiding this comment.
On the libevent backend, ev.data is overwritten by _mk_event_add() with the read-side event map, whose pipe[] fields are zeroed and never populated with the timer write socket. In the Windows-only backpressure scenario this makes the new setsockopt()/send() calls operate on socket 0 (or fail before reaching the wait) instead of the timer notification socket, so the regression either breaks CI or does not exercise the deadlock path it was added to cover.
Useful? React with 👍 / 👎.
|
Could you send you PR into the monkey HTTP server repo instead of this repo? |
2 participants
Problem
On Windows, the libevent timer callback writes notifications to a socket pair from the event-loop thread. When the socket buffer fills, the blocking
send()waits for the same event loop to drain the read side, deadlocking Fluent Bit. WPR captured the engine thread blocked incb_timeout()throughWSPSend()andSockWaitForSingleObject().Change
EAGAIN/EWOULDBLOCKas an already-pending timer notificationCompatibility
Normal timer delivery is unchanged. Under backpressure, redundant notifications are coalesced instead of blocking the engine thread.
Testing
Local Windows build environment was unavailable; GitHub Actions CI will run the Windows regression coverage.
Summary by CodeRabbit
Bug Fixes
Tests