feat(eventlistener) support eventlistener interface#1
feat(eventlistener) support eventlistener interface#1ruojieranyishen wants to merge 4 commits intoarana-db:masterfrom
Conversation
WalkthroughAdds Rust-side RocksDB event listener support: new Changes
Sequence DiagramsequenceDiagram
participant User as User code
participant EL as new_event_listener (Rust)
participant Handle as EventListenerHandle
participant Opts as Options
participant FFI as RocksDB FFI
participant DB as RocksDB
User->>EL: call new_event_listener(listener)
EL->>EL: box listener, build C callbacks & destructor
EL->>Handle: return EventListenerHandle
User->>Opts: add_event_listener(handle)
Opts->>FFI: rocksdb_options_add_eventlistener(opts, raw_ptr)
User->>DB: open DB with Options (listener attached)
Note over DB,FFI: later, a flush occurs
FFI->>FFI: create flush_job_info
FFI->>Handle: call on_flush_begin callback(ctx, flush_job_info)
Handle->>EL: translate pointers -> FlushJobInfo
EL->>User: invoke EventListener::on_flush_begin(&FlushJobInfo)
FFI->>Handle: call on_flush_completed callback(ctx, flush_job_info)
EL->>User: invoke EventListener::on_flush_completed(&FlushJobInfo)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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: 1
🧹 Nitpick comments (3)
src/db_options.rs (1)
1853-1880: API signature exposes raw pointer - consider a safer wrapper.The public API takes a raw
*mut ffi::rocksdb_eventlistener_tpointer, which is unusual compared to other methods in this file. The docstring example also shows a different API (EventListener::new(...)) than what's actually implemented.Consider either:
- Accept the
EventListenertrait object directly and callnew_event_listenerinternally- Or document that users must call
new_event_listener()firstAdditionally, other similar registrations (e.g.,
set_env,set_row_cache) store references inoutliveto ensure proper lifetime management. The event listener may need similar treatment to prevent use-after-free if the listener is dropped before the DB.- pub fn add_event_listener(&mut self, listener: *mut ffi::rocksdb_eventlistener_t) { + pub fn add_event_listener<E: EventListener + 'static>(&mut self, listener: E) { + let listener_ptr = crate::new_event_listener(listener); unsafe { - ffi::rocksdb_options_add_eventlistener(self.inner, listener); + ffi::rocksdb_options_add_eventlistener(self.inner, listener_ptr); } }src/event_listener.rs (2)
26-28:FlushJobInfoholds a raw pointer with unclear lifetime.The
FlushJobInfostruct holds a raw pointer toffi::rocksdb_flushjobinfo_twhich is only valid during the callback execution. If a user stores theFlushJobInforeference or clones data incorrectly, it could lead to use-after-free.Consider documenting this limitation or making
FlushJobInfonon-copyable/non-cloneable explicitly, or using a lifetime parameter to tie it to the callback scope.+/// Wrapper around flush job information from RocksDB. +/// +/// # Safety +/// This struct is only valid during the callback invocation. +/// Do not store references to this struct beyond the callback scope. pub struct FlushJobInfo { pub(crate) inner: *const ffi::rocksdb_flushjobinfo_t, }
177-195: Document ownership semantics fornew_event_listener.The function returns a raw pointer where ownership is implicitly transferred. Consider adding documentation to clarify that:
- The returned pointer should be passed to
Options::add_event_listener- RocksDB takes ownership and will call the destructor when appropriate
- The caller should not manually free this pointer
+/// Creates a new RocksDB event listener from a Rust implementation. +/// +/// # Ownership +/// The returned pointer is owned by the caller and should be passed to +/// `Options::add_event_listener`. RocksDB will take ownership and +/// automatically destroy the listener when the database is closed. +/// Do not manually free the returned pointer. pub fn new_event_listener<E: EventListener>(e: E) -> *mut ffi::rocksdb_eventlistener_t {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/db_options.rs(1 hunks)src/event_listener.rs(1 hunks)src/lib.rs(2 hunks)tests/test_event_listener.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/event_listener.rs (2)
src/ffi_util.rs (1)
from_raw_parts(219-221)tests/test_event_listener.rs (1)
on_flush_begin(28-31)
src/lib.rs (1)
src/event_listener.rs (1)
new_event_listener(177-195)
tests/test_event_listener.rs (1)
src/event_listener.rs (8)
new_event_listener(177-195)on_flush_begin(75-75)on_flush_begin(91-101)on_flush_begin(183-183)file_path(40-47)on_flush_completed(76-76)on_flush_completed(103-113)on_flush_completed(184-184)
🔇 Additional comments (3)
src/event_listener.rs (1)
87-89: LGTM!The destructor correctly reconstructs the
Boxfrom the raw pointer to properly drop the listener.src/lib.rs (1)
97-97: LGTM!The module declaration and public re-exports follow the existing patterns in the crate and properly expose the new event listener API.
Also applies to: 135-135
tests/test_event_listener.rs (1)
38-63: LGTM!The test properly exercises the event listener functionality by:
- Creating a listener with shared atomic counters
- Registering it with the DB options
- Triggering a flush and verifying both callbacks are invoked
Consider adding a test case that verifies behavior with multiple flushes or edge cases in the future.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/event_listener.rs (1)
94-116: Callbacks should catch panics to prevent undefined behavior.As noted in the comment at lines 74-76 and previously flagged, if the user's
EventListenerimplementation panics, the panic will unwind through the C/FFI boundary, which is undefined behavior in Rust. The code still lacks panic guards.
🧹 Nitpick comments (2)
src/event_listener.rs (2)
118-176: Consider passing NULL for unimplemented callbacks or add FIXME markers.These stub functions are registered but perform no actions. Since the corresponding trait methods are commented out (lines 80-87), users cannot implement them. Registering no-op callbacks could be misleading.
Options:
- Pass
Noneinstead ofSome(stub_fn)innew_event_listenerfor unimplemented callbacks- Add
FIXMEorHACKcomments to indicate these are placeholders awaiting proper implementation🔎 Option 1: Pass None for unimplemented callbacks
pub fn new_event_listener<E: EventListener>(e: E) -> *mut ffi::rocksdb_eventlistener_t { let p = Box::new(e); unsafe { ffi::rocksdb_eventlistener_create( Box::into_raw(p) as *mut c_void, Some(destructor::<E>), Some(on_flush_begin::<E>), Some(on_flush_completed::<E>), - Some(on_compaction_begin::<E>), - Some(on_compaction_completed::<E>), - Some(on_subcompaction_begin::<E>), - Some(on_subcompaction_completed::<E>), - Some(on_external_file_ingested::<E>), - Some(on_background_error::<E>), - Some(on_stall_conditions_changed::<E>), - Some(on_memtable_sealed::<E>), + None, // on_compaction_begin - TODO + None, // on_compaction_completed - TODO + None, // on_subcompaction_begin - TODO + None, // on_subcompaction_completed - TODO + None, // on_external_file_ingested - TODO + None, // on_background_error - TODO + None, // on_stall_conditions_changed - TODO + None, // on_memtable_sealed - TODO ) } }Then you can remove the stub functions (lines 118-176).
1-198: Runcargo fmtto address formatting issues.Rustfmt reports formatting differences at multiple locations. Please run
cargo fmtto automatically fix these style issues.Based on static analysis hints.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/event_listener.rs(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Rustfmt
src/event_listener.rs
[warning] 196-196:
Diff in /home/runner/work/rust-rocksdb/rust-rocksdb/src/event_listener.rs
[warning] 174-174:
Diff in /home/runner/work/rust-rocksdb/rust-rocksdb/src/event_listener.rs
[warning] 68-68:
Diff in /home/runner/work/rust-rocksdb/rust-rocksdb/src/event_listener.rs
[warning] 62-62:
Diff in /home/runner/work/rust-rocksdb/rust-rocksdb/src/event_listener.rs
[warning] 56-56:
Diff in /home/runner/work/rust-rocksdb/rust-rocksdb/src/event_listener.rs
[warning] 47-47:
Diff in /home/runner/work/rust-rocksdb/rust-rocksdb/src/event_listener.rs
[warning] 12-12:
Diff in /home/runner/work/rust-rocksdb/rust-rocksdb/src/event_listener.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Security audit
- GitHub Check: Linux
- GitHub Check: macOS
- GitHub Check: Windows
- GitHub Check: Doctests
- GitHub Check: Rustdoc-check
- GitHub Check: Clippy
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/event_listener.rs (2)
25-63: Critical: Use-after-free hazard with FlushJobInfo.This issue was previously identified in past reviews. The
FlushJobInfostruct holds a raw pointer that is only valid during the RocksDB event callback. Users can store this struct beyond the callback scope, leading to dangling pointer access when methods likecf_name()orfile_path()are called later.The past review suggested three mitigation strategies:
- Add lifetime parameters to tie the struct to the callback scope
- Document prominently that
FlushJobInfomust not outlive the callback- Eagerly copy all data in the callback wrapper instead of passing the raw pointer
Please implement one of these approaches to prevent memory safety issues.
95-137: Major: Callbacks lack panic handling to prevent undefined behavior.This issue was identified in previous reviews. If a user's
EventListenerimplementation panics, the panic will unwind through the C/FFI boundary, which is undefined behavior in Rust.The WARNING comment at lines 95-97 acknowledges this issue but states "Not fixing this issue for now." As suggested in past reviews, wrap the callback bodies with
std::panic::catch_unwindto prevent UB:🔎 Example panic handling for on_flush_begin
unsafe extern "C" fn on_flush_begin<E: EventListener>( ctx: *mut c_void, _db: *mut ffi::rocksdb_t, flush_job_info: *const ffi::rocksdb_flushjobinfo_t, ) { let _ = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { let listener = &*(ctx as *const E); let info = FlushJobInfo { inner: flush_job_info, }; listener.on_flush_begin(&info); })); }Apply similar wrapping to
on_flush_completedand the destructor.
🧹 Nitpick comments (1)
src/event_listener.rs (1)
139-197: Stub callbacks registered but not implemented.Eight event callbacks are registered with RocksDB but contain only TODO placeholders:
- Compaction events (begin/completed, subcompaction begin/completed)
- External file ingestion
- Background errors
- Stall condition changes
- Memtable sealing
While not a bug (RocksDB will simply invoke no-op functions), consider:
- Uncommenting the corresponding trait methods in
EventListener(lines 101-108)- Implementing the callback bodies to invoke those trait methods
- Creating wrapper types like
FlushJobInfofor other event info structsThis would provide a complete event listener API for users.
Do you want me to generate the implementation pattern for the remaining callbacks?
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/db_options.rs(2 hunks)src/event_listener.rs(1 hunks)src/lib.rs(2 hunks)tests/test_event_listener.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib.rs
- tests/test_event_listener.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/event_listener.rs (2)
src/ffi_util.rs (1)
from_raw_parts(219-221)tests/test_event_listener.rs (2)
on_flush_begin(28-31)on_flush_completed(33-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: macOS
- GitHub Check: Doctests
- GitHub Check: Linux
- GitHub Check: Windows
- GitHub Check: Rustdoc-check
- GitHub Check: Clippy
🔇 Additional comments (4)
src/db_options.rs (2)
33-33: LGTM! Import correctly adds EventListenerHandle.The import is properly scoped and necessary for the new
add_event_listenermethod.
1854-1893: LGTM! Ownership transfer to RocksDB is correctly implemented.The method properly transfers ownership of the event listener to RocksDB by:
- Calling
into_raw()which sets theownedflag to false- Passing the raw pointer to the FFI layer
- Preventing Rust's Drop from destroying the listener
The documentation and example are comprehensive and helpful.
src/event_listener.rs (2)
65-94: LGTM! EventListenerHandle ownership management is well-designed.The handle correctly manages ownership with the
ownedflag:
from_rawassumes ownership (owned=true)into_rawtransfers ownership (owned=false)Droponly destroys if ownedThe
SendandSyncimplementations are appropriate for this use case.
199-218: LGTM! Factory function correctly constructs the event listener.The
new_event_listenerfunction properly:
- Boxes the user's listener implementation for a stable pointer
- Registers all callbacks (both implemented and stub) with RocksDB
- Wraps the FFI pointer in a safe
EventListenerHandleThe implementation is sound and provides a safe interface for users.
GROK 代码审查报告（feat(eventlistener) support eventlistener interface）关键要点
该 PR 为 rust-rocksdb 添加了 RocksDB EventListener 支持（目前仅 flush 事件），通过 FFI 桥接 C API。功能基本可用，但存在安全隐患，不建议直接合并到生产分支，需修复后再考虑。 1. Bug（潜在运行时问题）
2. 逻辑错误（设计或功能逻辑问题）
3. 语法不当（代码风格、规范问题）
总体建议
类别 | 文件/行号 | 问题描述 | 修复建议 | 优先级
-- | -- | -- | -- | --
Bug | event_listener.rs:80-120 | FFI 回调 panic UB | 包裹 catch_unwind | 高
Bug | event_listener.rs:20-40 | FlushJobInfo use-after-free 风险 | 加强文档或 copy 数据 | 高
Bug | event_listener.rs:200-210 | 未实现回调注册 stub | 传 None | 中
逻辑错误 | db_options.rs add_event_listener | 所有权转移复杂易错 | 内部 Box 管理 | 中
|
|
GPT 审查报告
PR 里自己也写了 WARNING：如果用户实现的 EventListener 在回调里 panic，会发生 Rust unwind 穿过 C/FFI 边界，这是未定义行为（UB），可能直接把进程搞挂或者出现诡异内存问题。现在的实现确实没有 catch_unwind。 建议修复方式： 在每个 extern "C" 回调里用 std::panic::catch_unwind 包起来； panic 后至少要吞掉（记录日志/计数），避免传播到 C； （可选）把 panic 信息写到某个全局 hook 或 callback error handler。
回调里直接把 flush_job_info 塞进 FlushJobInfo { inner: flush_job_info }，后续 cf_name()/file_path() 会无条件调用 ffi::rocksdb_flushjobinfo_* 并对返回指针做 slice。 如果 RocksDB 在某些异常路径传了 NULL（我不敢断言一定不会），这会立刻 UB。 建议： 回调入口对 flush_job_info.is_null() 做 guard； 或者把 FlushJobInfo 设计成 Option / 在方法里遇到 null 返回空字符串/Result。
这里逻辑是：C 返回 ptr + size，然后 from_raw_parts(ptr as *const u8, size)。 如果 C 在某些情况下返回 ptr = NULL, size = 0，理论上 from_raw_parts(NULL, 0) 是允许的；但如果是 ptr = NULL, size > 0 就直接 UB。 建议： 若 size > 0 && ptr.is_null() 则返回空/报错； 或者统一把空指针当成空字符串处理。
EventListenerHandle 被 unsafe impl Send + Sync 了。 从 Rust 侧看你约束了 E: Send + Sync，但 C 侧 rocksdb_eventlistener_t 本身是否能跨线程销毁/访问，这里并没有证明。RocksDB 的 event listener 回调通常可能发生在后台线程，这点倒是符合，但handle 的 Drop（destroy）在哪个线程发生不可控。 建议（保守做法）： 不给 EventListenerHandle 自动标 Send/Sync，或者至少在文档里明确线程模型； 若要保留，最好给出安全性说明：为什么 destroy 和 C 对象线程安全。
add_event_listener 消费 EventListenerHandle，内部 into_raw() 把 owned=false，避免 Rust Drop 再 destroy，然后把 ptr 交给 rocksdb_options_add_eventlistener。这个逻辑本身是合理的（避免 double free）。 但这里有两个“坑”需要在文档写清楚： 一旦 add 进去，listener 生命周期由 RocksDB/C++ 管，Rust 侧不再能手动释放； 如果用户创建了 handle 但没 add，就由 Rust Drop 释放（这是当前行为）。 否则用户容易误以为 handle 还要保存在某处来“保持活着”。
你把 compaction / ingestion / background_error / stall / memtable 等回调都传进去了，但目前函数体是 TODO 空实现。 这不一定是 bug，但会误导使用者：他们以为实现 trait 对应方法会被调用，但实际上 trait 里也注释掉了这些方法，只剩 flush 两个默认实现。 建议： 要么别把这些 TODO callback 注册进去（传 None），避免未来 ABI/行为不一致； 要么把 trait 方法补齐（即使默认空实现），并写清楚哪些 RocksDB 版本/特性可用。 结论 这个 PR 能跑通 flush 监听的主路径，但最大的问题是 #1：panic 跨 FFI 的 UB，属于必须修的安全缺陷。 其次是空指针/返回指针防御（#2/rust-rocksdb#3），以及 Send/Sync 的安全性说明（rust-rocksdb#4）。 直接给你一版可直接贴进 PR 的补丁级写法，目标只有两件事：
我按 “最小侵入、不改现有 API” 的原则来写。 一、FFI 回调统一加
|
2 participants
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.