Issue with dropping a channel, ManuallyDrop and FFI

Folks!
long time I don't hang out here :slight_smile:

Anyhow I got a small problem and I would like your input on it and on the pattern.

In Rust code I create a structure that hold sender of a channel, this structure is then managed by C code via FFI. Eventually I get back the raw pointer from the C code and I need to send to the channel.

This code works (or at least seems to work).

#[repr(C)]
pub struct RedisKey {
    pub key: *mut rm::ffi::RedisModuleKey, // this is the raw pointer
}

impl RedisKey {
    // this is how we create the structure
    pub fn new(key_name: &str, ctx: &Context) -> Self {
        let key_name = rm::RMString::new(ctx, key_name);
        let key =
            rm::OpenKey(ctx, &key_name, rm::ffi::REDISMODULE_WRITE);
        RedisKey { key }
    }
    // this is how we access the sender
    pub fn get_channel(
        &self,
    ) -> Result<&Sender<Command>, RediSQLError> {
        match self.key_type() {
            KeyTypes::RediSQL => unsafe {
                // we get the raw pointer
                let dbkey = rm::ffi::RedisModule_ModuleTypeGetValue
                    .unwrap()(self.key)
                    as *mut DBKey;
                Ok(&(*dbkey).tx) // just return the sender here
            },
            // business logic here
            KeyTypes::Empty => Err(RediSQLError::empty_key()),
            _ => Err(RediSQLError::no_redisql_key()),
        }
    }
}

The problem of this code is that eventually I need to remember to don't drop anything
So I moved into this other code:



#[repr(C)]
pub struct RedisKey {
    pub key: *mut rm::ffi::RedisModuleKey,
}

// let's use this structure that `Deref` to DBKey,
// so I don't have to remember anything anymore!
pub struct UndroppableDBKey {
    dbkey: std::mem::ManuallyDrop<DBKey>,
}

impl std::ops::Deref for UndroppableDBKey {
    type Target = DBKey;
    fn deref(&self) -> &Self::Target {
        &self.dbkey
    }
}

impl RedisKey {
    pub fn new(key_name: &str, ctx: &Context) -> Self {
        .... as before ....
    }
    pub fn get_channel(
        &self,
    ) -> Result<Sender<Command>, RediSQLError> {
        // use the method below to get the undroppable version
        let dbkey = self.get_dbkey()?;
        // the second time I use this Sender the channel is closed.
        Ok(dbkey.tx.clone())
    }
    pub fn get_dbkey(
        &self,
    ) -> Result<UndroppableDBKey, RediSQLError> {
        match self.key_type() {
            // similar to above, we get the raw pointer here
            KeyTypes::RediSQL => {
                let dbkey = unsafe {
                    rm::ffi::RedisModule_ModuleTypeGetValue.unwrap()(
                        self.key,
                    ) as *mut DBKey
                };
                // eventually we read the pointer, get the structure and
                // push the new structure in ManuallyDrop
                Ok(UndroppableDBKey {
                    dbkey: std::mem::ManuallyDrop::new(unsafe {
                        dbkey.read()
                    }),
                })
            }
            KeyTypes::Empty => Err(RediSQLError::empty_key()),
            _ => Err(RediSQLError::no_redisql_key()),
        }
    }
}

The second version does not work, it works once and then the Sender is closed, the error is something like: sending on a closed channel.

Unfortunately I don't understand what I am doing wrong and why?

Do you guys have ideas?

dbkey.read() this probably the problem. Your creating a fake Copy like structure rather than using the original (by-ref or move or clone.)
Sender is internally mutable, a mutation does not change the original.

Hummm, I am not following.

In my mental model raw.read() takes a *mut T and create a T.

Then I put this T inside ManuallyDrop and that's it. In this way T is never dropped, and I am pretty sure about this.

What is interesting is that in the not-working function I return a whole Sender<> while in the working function I return a reference to it.

So what are you suggesting is that the whole Sender I am returning is dropping and this broke the channel.

Is that a possibility?

There is no drop only mutation.

This code does not compile;

    struct NotCopy;
    let orig = NotCopy;
    let _a_move = orig;
    let _b_move = orig;

There is no new instance created. Once you call read you have to discard the original memory.

I can kinda grasp what you are point to, but I am not sure I understand it yet.

Ok, I am understading that the code above does not compile, but I cannot connect it to my problem.

Humm, if I don't drop the new value created, the original memory can stay as it is, can't it?

No, only Copy types can be read from multiple times.
Example changed to pointer;

    struct NotCopy;
    let orig: *mut NotCopy = Box::into_raw(Box::new(NotCopy)); // ignore the leaked memory (is a different issue)
    
    // first is fine
    let _a_move = unsafe { std::ptr::read(orig) };
    // since type is not copy have to discard orig (or write back to it)
    
    // not allowed since type not Copy
    let _b_move = unsafe { std::ptr::read(orig) };
1 Like

I am still confuse! From

Reads the value from src without moving it. This leaves the memory in src unchanged.

I understand it is not allowed, hence the unsafe around, but I don't understand why it is not working. The physical memory should stay the same. I don't understand what mutate the the Sender in my case.

Something must happen somewhere that invalidate/close the Sender, what? Why?

What channel are you using? The std mpsc, crossbeam's mpmc, something else?

The std::mpsc :slight_smile:

In that case you could just clone the Sender right?

Just checking: Is it possible the receiver was dropped?

(Take into account I don't understand why your trying to change to ManuallyDrop but only trying to address why it is failing from doing so.)

Note: This may be a general rule rather than for all cases, [not a compiler dev.] If you don't know the internal of a structure then should abide by it.
In your case your structure is composed of Sender.
"src must be valid for reads" after the first read it becomes invalid.
"without moving it" might be a poor choice of description; the memory still exits but until you write back should not be read from.

In terms of the mutation;
new creates a sender with OneShot
clone changes that to Shared
More going on to give error, but it is down to the original memory being unchanged.

Yeah but the sender is referenced by this other object, that I don't want to forget to forget... So I wrapped it into the ManuallyDrop, but the thing does not work...

No the receiver is well alive.

I would need to read the code of the sender!

Fortunately I was able to work around the issue, hopefully without leaking anything.