Garbage value in callback closure

1

I'm writing rust wrappers on mosquitto mqtt library. Callbacks has been bit of a problem but I'm able to tackle them by passing closure as user_data.

    //  Registered callback is called when the broker sends a CONNACK message in response
    // to a connection. Will be called even incase of failure. All your sub/pub stuff
    // should ideally be done in this callback when connection is successful

    pub fn onconnect_callback<F>(&self, callback: F)
        where F: Fn(i32)
    {

        // Convert the rust closure into void* to be used as user_data. This will
        // be passed to callback automatically by the library
        let cb = &callback as *const _ as *mut libc::c_void;


        unsafe {
            // Set our closure as user data
            bindings::mosquitto_user_data_set(self.mosquitto, cb);
            // Register callback
            bindings::mosquitto_connect_callback_set(self.mosquitto, Some(onconnect_wrapper::<F>));
        }

        // Registered callback. user data is our closure
        unsafe extern "C" fn onconnect_wrapper<F>(mqtt: *mut bindings::Struct_mosquitto,
                                                  closure: *mut libc::c_void,
                                                  val: libc::c_int)
            where F: Fn(i32)
        {
            let closure = closure as *mut F;
            println!("rc = {:?}", val as i32);
            (*closure)(val as i32);

        }
    }

But I'm getting garbage value while capturing the variable.

    let i = 100;

    client.onconnect_callback(|a: i32|{
        println!("i = {:?}", i);
        println!("@@@ On connect callback {}@@@", a)
        });
    
    match client.connect("localhost"){
        Ok(_) => println!("Connection successful --> {:?}", client),
        Err(n) => panic!("Connection error = {:?}", n)
    }

OUTPUT:

i = 734146560
@@@ On connect callback 0@@@

What am I doing wrong here ?

2

Now, I can't say for certain, but I can imagine a couple of scenarios:

  1. The userdata isn't properly set or fetches, resulting in an invalid environment.
  2. The compiler thinks that callback is only valid for the lifetime of onconnect_callback, so the memory location of i is recycled for other values.

The first scenario can easily be tested by sending some completely known value and checking for it. The second scenario would probably require you to make F: 'static as well, and prefix the closure with move.

By the way, don't you have to make sure callback isn't dropped in ongoing_callback (as in forget(callback))?

1 Like
3

@ogeon Yeah. If closure is a kind on non-copy type, Maybe this is the problem. (Closure is destroyed by the time c callback is invoked)

UPDATE:

I passed the closure by reference and the value is fine now.

pub fn onconnect_callback<F>(&self, callback: & F)
        where F: FnMut(i32)

and

let closure = |a: i32|{
                            println!("i = {:?}", i);
                            println!("@@@ On connect callback {}@@@", a)
                          };

    client.onconnect_callback(&closure);

OUTPUT

i = 100
@@@ On connect callback 0@@@

But passing closure by reference doesn't look very nice to me. Is there any other alternative that you can think of ?

4

I wouldn't trust the above implementation to be correct, since closures will usually contain references to their environments and not the actual values. My suspicion lies on a combination of that and scenario 2.

5

It's still a bit fishy. I don't know how the closure is handled throughout the program flow and how long it's supposed to be alive, but I'm assuming that it will have to outlive onconnect_callback. This makes the reference variant work temporarily because of the order of destruction, but not in a longer term. What about this, for example:

{
    let closure = |a: i32|{
        println!("i = {:?}", i);
        println!("@@@ On connect callback {}@@@", a)
    };

    client.onconnect_callback(&closure);
    //Closure destroyed here
}

I think the safest bet, as far as I know, is to require the closure to be 'static, meaning that it's ok for it to exist "for ever", and use forget(...) to prevent its destruction. You may also want to require Sync and Send if there is a risk of multi-threading.

1 Like
6

I've tried the 'static + box method suggested by you and Vladimir in the SOF link below and it works.

http://stackoverflow.com/questions/34247879/rust-closure-as-callback-for-c-bindings-receiving-garbage-value-in-captured-vari?noredirect=1#comment56254788_34247879

But there is still a problem because user data is being set by a function

     // Set our closure as user data
     bindings::mosquitto_user_data_set(self.mosquitto, cb);

My library has multiple callback functions. By the time a callback is invoked, user data is being overwritten setting a different closure as user data causing invocation of wrong closure.

To fix that, I'm storing all the callbacks in the struct (below) and passing pointer to the struct as user data.

        pub struct Client<'b, 'c, 'd> {
        pub id: String,
        pub user_name: Option<&'b str>,
        pub password: Option<&'c str>,
        pub host: Option<&'d str>,
        pub keep_alive: i32,
        pub clean_session: bool,
        pub icallbacks: HashMap<String, Box<Fn(i32)>>,
        pub scallbacks: HashMap<String, Box<Fn(&str)>>,
        pub mosquitto: *mut bindings::Struct_mosquitto,
    }

Rust callback wrapper will just update the callback fields of the struct as below.

        pub fn onconnect_callback<F>(&mut self, callback:  F)
            where F: Fn(i32), F: 'static
        {
            self.icallbacks.insert("on_connect".to_string(), Box::new(callback));
            //setting client object as userdata. Setting 'callback' as userdata is buggy because by the
            //time the actual callback is invoked, other callbacks like 'on_subscribe' callback is overwriting
            //the userdata and wrong closure is getting invoked for on_connect callback
            let cb = self as *const _ as *mut libc::c_void;
            unsafe {
                // Set our closure as user data
                bindings::mosquitto_user_data_set(self.mosquitto, cb);
                // Register callback
                bindings::mosquitto_connect_callback_set(self.mosquitto, Some(onconnect_wrapper));
            }

            // Registered callback. user data is our closure
            unsafe extern "C" fn onconnect_wrapper(mqtt: *mut bindings::Struct_mosquitto,
                                                      closure: *mut libc::c_void,
                                                      val: libc::c_int)
            {
                let client: &mut Client = mem::transmute(closure);
                match client.icallbacks.get("on_connect") {
                    Some(cb) => cb(val as i32),
                    _ => panic!("No callback found"),
                }
            }
        }

Just want to know if there are any mistakes in this approach.

7

That's why you have to use std::mem::forget. The closure will otherwise be dropped and any reference to local variables will disappear with it.

8

I've tried forget. I think that is not the problem.

Let's say I have 2 callback set rust functions

onconnect_callback(&mut self, callback: F) (1)

In this wrapper definitions, I'll convert the callback closure to void pointer and pass it to user_data_set binding.

bindings::mosquitto_user_data_set(self.mosquitto, cb);

Now user data is pointer to this closure and let's say its value is 0x1234 and I've made sure that closure is not destroyed.

When C callback is invoked, User data, which is pointer to callback closure (0x1234) should have been passed.

BUT by the time the C callback is invoked, let's say I've called one more rust callback set wrapper

onsubscribe_callback(&mut self, callback: F)  (2)

This calls the user_data_set as well to set its closure as user data and user data pointer is now overwritten.

Now user data is pointing to 0x5678. If Actual on connect C call back is invoked by the library at this point, it's user data would be 0x5678 which is pointing to wrong closure.

9

How do I enable rust syntax highlighting.

10

Oh, so you can only set one callback? Then I would use a similar method to what you are already doing with the HashMap. You could, however, replace the string with an enum to leverage the type safety a bit.

Surround your code with ```.

11

Yes. Just one callback/user data.

Thank you very much for the help. :slight_smile: