diff --git a/examples/scan_keys.rs b/examples/scan_keys.rs index 92e28369..057a976a 100644 --- a/examples/scan_keys.rs +++ b/examples/scan_keys.rs @@ -17,8 +17,8 @@ fn scan_keys(ctx: &Context, _args: Vec) -> RedisResult { let cursor = KeysCursor::new(); let mut res = Vec::new(); - let scan_callback = |_ctx: &Context, key_name: RedisString, _key: Option<&RedisKey>| { - res.push(RedisValue::BulkRedisString(key_name)); + let scan_callback = |ctx: &Context, key_name: &RedisString, _key: Option<&RedisKey>| { + res.push(RedisValue::BulkRedisString(key_name.safe_clone(ctx))); }; while cursor.scan(ctx, &scan_callback) { diff --git a/src/context/key_cursor.rs b/src/context/key_cursor.rs index 3e0684aa..ccb61630 100644 --- a/src/context/key_cursor.rs +++ b/src/context/key_cursor.rs @@ -1,5 +1,6 @@ use std::{ ffi::c_void, + mem, ptr::{self}, }; @@ -24,7 +25,7 @@ use crate::{key::RedisKey, raw, RedisString}; /// fn example_scan_key_for_each(ctx: &Context) -> RedisResult { /// let key = ctx.open_key_with_flags("user:123", KeyFlags::NOEFFECTS | KeyFlags::NOEXPIRE | KeyFlags::ACCESS_EXPIRED ); /// let cursor = ScanKeyCursor::new(key); -/// +/// /// let res = RefCell::new(Vec::new()); /// cursor.for_each(|_key, field, value| { /// let mut res = res.borrow_mut(); @@ -92,11 +93,10 @@ impl ScanKeyCursor { let callback = unsafe { &mut *(data.cast::()) }; callback(&key, &field, &value); - // we're not the owner of field and value strings - field.take(); - value.take(); - - key.take(); // we're not the owner of the key either + // We don't own any of the passed in pointers, so we must ensure we don't run their destructors here + mem::forget(field); + mem::forget(value); + mem::forget(key); } // Safety: The c-side initialized the function ptr and it is is never changed, diff --git a/src/context/keys_cursor.rs b/src/context/keys_cursor.rs index aaa3f748..21918df3 100644 --- a/src/context/keys_cursor.rs +++ b/src/context/keys_cursor.rs @@ -3,13 +3,14 @@ use crate::key::RedisKey; use crate::raw; use crate::redismodule::RedisString; use std::ffi::c_void; +use std::mem; use std::ptr::NonNull; pub struct KeysCursor { inner_cursor: *mut raw::RedisModuleScanCursor, } -extern "C" fn scan_callback)>( +extern "C" fn scan_callback)>( ctx: *mut raw::RedisModuleCtx, key_name: *mut raw::RedisModuleString, key: *mut raw::RedisModuleKey, @@ -20,13 +21,17 @@ extern "C" fn scan_callback)>( let redis_key = if key.is_null() { None } else { - Some(RedisKey::from_raw_parts(ctx, key)) + // Safety: The returned `RedisKey` does not outlive this callbacks and so by necessity + // the pointers passed in as parameters are valid for its entire lifetime. + Some(unsafe { RedisKey::from_raw_parts(ctx, key) }) }; let callback = unsafe { &mut *(private_data.cast::()) }; - callback(&context, key_name, redis_key.as_ref()); + callback(&context, &key_name, redis_key.as_ref()); - // we are not the owner of the key, so we must take the underline *mut raw::RedisModuleKey so it will not be freed. - redis_key.map(|v| v.take()); + // We don't own any of the passed in pointers and have just created "temporary RAII types". + // We must ensure we don't run their destructors here. + mem::forget(redis_key); + mem::forget(key_name); } impl KeysCursor { @@ -35,7 +40,7 @@ impl KeysCursor { Self { inner_cursor } } - pub fn scan)>( + pub fn scan)>( &self, ctx: &Context, callback: &F, diff --git a/src/key.rs b/src/key.rs index c16f3d0c..f05c7f08 100644 --- a/src/key.rs +++ b/src/key.rs @@ -59,12 +59,6 @@ pub struct RedisKey { } impl RedisKey { - pub(crate) fn take(mut self) -> *mut raw::RedisModuleKey { - let res = self.key_inner; - self.key_inner = std::ptr::null_mut(); - res - } - pub fn open(ctx: *mut raw::RedisModuleCtx, key: &RedisString) -> Self { let key_inner = raw::open_key(ctx, key.inner, to_raw_mode(KeyMode::Read)); Self { ctx, key_inner } @@ -80,13 +74,29 @@ impl RedisKey { Self { ctx, key_inner } } - pub const fn from_raw_parts( + /// Construct a new `RedisKey` from a pointer to a redismodule context and a key. + /// + /// # Safety + /// + /// The caller must ensure: + /// 1. The `ctx` pointer remains valid for the lifetime of the `RedisKey`. + /// 2. The `key_inner` pointer remains valid for the lifetime of the `RedisKey`. + pub const unsafe fn from_raw_parts( ctx: *mut raw::RedisModuleCtx, key_inner: *mut raw::RedisModuleKey, ) -> Self { Self { ctx, key_inner } } + /// Decomposes a `RedisKey` into its raw components: `(redismodule context pointer, key pointer)`. + /// + /// After calling this function, the caller is responsible for cleaning up the raw key previously managed by the `RedisKey`. + /// The only way to do this safely is to convert the raw redismodule context and key pointers back into a `RedisKey` with + /// the [`from_raw_parts`][Self::from_raw_parts] function, allowing the destructor to perform the cleanup. + pub fn to_raw_parts(self) -> (*mut raw::RedisModuleCtx, *mut raw::RedisModuleKey) { + (self.ctx, self.key_inner) + } + /// # Panics /// /// Will panic if `RedisModule_ModuleTypeGetValue` is missing in redismodule.h