3
\$\begingroup\$

I'm trying to write up a cross-platform rust library (one, that will be used in iOS/Mac/Android dev etc.) It is based largely on concept's and code from Mozilla. One of the requirements I have is to observe changes to struct properties in rust (similar to KVO in Objective-C). The following is a pretty big wall of code (I apologize for that) - being a beginner with Rust, I would appreciate any input on how to improve it. Thanks

use {
    ffi_support::{
        define_string_destructor, handle_map::ConcurrentHandleMap, rust_string_to_c, ExternError,
        FfiStr,
    },
    lazy_static::lazy_static,
    std::{
        collections::HashMap,
        ffi::c_void,
        os::raw::c_char,
        sync::{
            atomic::{AtomicUsize, Ordering},
            RwLock,
        },
    },
};

define_string_destructor!(observer_destroy_string);

static ID_GEN: AtomicUsize = AtomicUsize::new(1);

/*
I'm storing the callbacks outside the object being observed (as, I would like to 
generate this code using a macro, and, I don't see a way to add a property to an
object using a macro). 
While, I could add a variable in the struct to store the property, I figure a 
hashmap/id combination would be better than having multiple callback variables 
being user defined. 
*/
#[derive(Eq, PartialEq, Hash, Copy, Clone)]
struct Id(usize);

impl Id {
    fn new() -> Self {
        Id(ID_GEN.fetch_add(1, Ordering::SeqCst))
    }
}

// Object would need to implement this trait, to have their properties observable.
trait Observable {
    type KeyType;
    fn observable_key(&self) -> Self::KeyType;
}

// Sample observable struct. We are going to observe changes to `name`.
pub struct Person {
    id: Id,
    name: String,
}

impl Person {
    pub fn new(name: &str) -> Self {
        Person {
            id: Id::new(),
            name: name.to_string(),
        }
    }
}

impl Observable for Person {
    type KeyType = Id;

    fn observable_key(&self) -> Self::KeyType {
        self.id
    }
}

static OBSERVER_ID_GEN: AtomicUsize = AtomicUsize::new(1);

/*
When we add a callback, we get a observer id in return. This id is to be used to 
unregister a callback when we are no longer interested. Is there a good way to 
handle lifetime (esp over FFI)? Is it possible for me to defensively add checks 
on Person being deallocated, and an unregister call coming in later? Or, do I just 
have to be extra cautious with my code?
*/
#[derive(Eq, PartialEq, Hash, Copy, Clone)]
pub struct ObserverId(usize);

impl ObserverId {
    fn new() -> Self {
        ObserverId(OBSERVER_ID_GEN.fetch_add(1, Ordering::SeqCst))
    }
}

type NameCallbackCallbackType = Box<dyn Fn(&str, &str) + Sync + Send + 'static>;
type FfiCallbackDestructorType = Box<dyn Fn() + Sync + Send + 'static>;

/*
A wrapper to a callback, which holds an optional destructor for the callback 
(useful to deallocate Swift closures that are passed in).
*/
struct NameCallback {
    callback: NameCallbackCallbackType,
    ffi_destructor: Option<FfiCallbackDestructorType>,
}

/*
Without these I get compile errors (around NameCallback not being safe to send 
across threads).
*/
unsafe impl Send for NameCallback {}
unsafe impl Sync for NameCallback {}

impl Drop for NameCallback {
    fn drop(&mut self) {
        if let Some(ref ffi_destructor) = self.ffi_destructor {
            ffi_destructor()
        }
    }
}

struct NameCallbacks(HashMap<ObserverId, NameCallback>);

impl NameCallbacks {
    fn new() -> Self {
        NameCallbacks(HashMap::new())
    }

    fn insert(&mut self, callback: NameCallback) -> ObserverId {
        let observer_id = ObserverId::new();
        self.0.insert(observer_id, callback);
        observer_id
    }

    fn remove(&mut self, observer_id: ObserverId) {
        self.0.remove(&observer_id);
    }
}
type PersonObservableKeyType = <Person as Observable>::KeyType;

lazy_static! {
    static ref OBSERVERS_OF_NAME: RwLock<HashMap<PersonObservableKeyType, NameCallbacks>> =
        RwLock::new(HashMap::new());
}

impl Person {
    pub fn observe_name<F>(&mut self, f: F) -> ObserverId
    where
        F: Fn(&str, &str) + Sync + Send + 'static,
    {
        let mut map = OBSERVERS_OF_NAME.write().unwrap();
        let callbacks = map
            .entry(self.observable_key())
            .or_insert(NameCallbacks::new());
        callbacks.insert(NameCallback {
            callback: Box::new(f),
            ffi_destructor: None,
        })
    }

    pub fn unobserve_name(&mut self, observer_id: ObserverId) {
        let mut map = OBSERVERS_OF_NAME.write().unwrap();
        map.entry(self.observable_key())
            .and_modify(|e| e.remove(observer_id));
    }

    pub fn set_name(&mut self, name: &str) {
        let old_value = self.name.clone();
        self.name = name.to_string();
        if let Some(name_callbacks) = OBSERVERS_OF_NAME
            .read()
            .unwrap()
            .get(&self.observable_key())
        {
            for (_, name_callback) in name_callbacks.0.iter() {
                (name_callback.callback)(old_value.as_str(), self.name.as_str());
            }
        }
    }
}

lazy_static! {
    static ref ITEMS: ConcurrentHandleMap<Person> = ConcurrentHandleMap::new();
}

#[no_mangle]
pub extern "C" fn observer_extern_error_new() -> ExternError {
    ExternError::success()
}

#[no_mangle]
pub extern "C" fn observer_person_new(name: FfiStr, err: &mut ExternError) -> u64 {
    ITEMS.insert_with_output(err, || Person::new(name.as_str()))
}

#[derive(Copy, Clone)]
struct UserData(*mut c_void);

/* Similar to NameCallback, these are needed to avoid threading compile errors. 
*/
unsafe impl Send for UserData {}
unsafe impl Sync for UserData {}

#[no_mangle]
pub extern "C" fn observer_person_observe_name(
    h: u64,
    user_data: *mut c_void,
    callback: fn(*mut c_char, *mut c_char, *mut c_void),
    destructor: Option<fn(*mut c_void)>,
    err: &mut ExternError,
) -> u64 {
    let user_data = UserData(user_data);
    ITEMS.call_with_output_mut(err, h, |person| {
        let mut map = OBSERVERS_OF_NAME.write().unwrap();
        let callbacks = map
            .entry(person.observable_key())
            .or_insert(NameCallbacks::new());
        callbacks
            .insert(NameCallback {
                callback: Box::new(move |old_value, new_value| {
                    callback(
                        rust_string_to_c(old_value),
                        rust_string_to_c(new_value),
                        user_data.0,
                    )
                }),
                ffi_destructor: {
                    match destructor {
                        Some(destructor) => Some(Box::new(move || destructor(user_data.0))),
                        None => None,
                    }
                },
            })
            .0 as u64
    })
}

#[no_mangle]
pub extern "C" fn observer_person_unobserve_name(h: u64, observer_id: u64, err: &mut ExternError) {
    ITEMS.call_with_output_mut(err, h, |person| {
        person.unobserve_name(ObserverId(observer_id as usize));
    })
}

#[no_mangle]
pub extern "C" fn observer_person_set_name(h: u64, name: FfiStr, err: &mut ExternError) {
    ITEMS.call_with_output_mut(err, h, |person| {
        person.set_name(name.as_str());
    })
}

#[no_mangle]
pub extern "C" fn observer_person_get_name(h: u64, err: &mut ExternError) -> *mut c_char {
    ITEMS.call_with_output(err, h, |person| rust_string_to_c(person.name.clone()))
}

#[cfg(test)]
mod tests {
    use {super::*, ffi_support::FfiStr, std::ffi::CString};

    #[test]
    fn test_callback() {
        let mut person = Person::new("Bob");
        static mut SET_COUNT: u64 = 0;
        static mut CALLBACK1_RUN_COUNT: u64 = 0;
        static mut CALLBACK2_RUN_COUNT: u64 = 0;
        let observer1_id = person.observe_name(|old_value, new_value| unsafe {
            match SET_COUNT {
                0 => {
                    assert_eq!("Bob", old_value);
                    assert_eq!("Nancy", new_value);
                }
                1 => {
                    assert_eq!("Nancy", old_value);
                    assert_eq!("Frank", new_value);
                }
                2 => {
                    assert_eq!("Frank", old_value);
                    assert_eq!("Anthony", new_value);
                }
                _ => {}
            }
            CALLBACK1_RUN_COUNT += 1;
        });
        let observer2_id = person.observe_name(|old_value, new_value| unsafe {
            match SET_COUNT {
                0 => {
                    assert_eq!("Bob", old_value);
                    assert_eq!("Nancy", new_value);
                }
                1 => {
                    assert_eq!("Nancy", old_value);
                    assert_eq!("Frank", new_value);
                }
                2 => {
                    assert_eq!("Frank", old_value);
                    assert_eq!("Anthony", new_value);
                }
                _ => {}
            }
            CALLBACK2_RUN_COUNT += 1;
        });
        person.set_name("Nancy");
        unsafe {
            SET_COUNT += 1;
            assert_eq!(1, CALLBACK1_RUN_COUNT);
            assert_eq!(1, CALLBACK2_RUN_COUNT);
        }
        person.unobserve_name(observer1_id);
        person.set_name("Frank");
        unsafe {
            SET_COUNT += 1;
            assert_eq!(1, CALLBACK1_RUN_COUNT);
            assert_eq!(2, CALLBACK2_RUN_COUNT);
        }
        person.unobserve_name(observer2_id);
        person.set_name("Anthony");
        unsafe {
            SET_COUNT += 1;
            assert_eq!(1, CALLBACK1_RUN_COUNT);
            assert_eq!(2, CALLBACK2_RUN_COUNT);
        }
    }

    static mut SET_COUNT: u8 = 0;
    static mut CALLBACK_RUN_COUNT: u8 = 0;
    lazy_static! {
        static ref NAME1: CString = CString::new("Bob").unwrap();
        static ref NAME2: CString = CString::new("Nancy").unwrap();
        static ref NAME3: CString = CString::new("Frank").unwrap();
    }

    #[no_mangle]
    fn observer_name_callback(old_value: *mut c_char, new_value: *mut c_char, _: *mut c_void) {
        unsafe {
            match SET_COUNT {
                0 => {
                    assert!(libc::strcmp(NAME1.as_c_str().as_ptr(), old_value) == 0);
                    assert!(libc::strcmp(NAME2.as_c_str().as_ptr(), new_value) == 0);
                }
                1 => {
                    assert!(libc::strcmp(NAME2.as_c_str().as_ptr(), old_value) == 0);
                    assert!(libc::strcmp(NAME3.as_c_str().as_ptr(), new_value) == 0);
                }
                _ => {}
            }
            observer_destroy_string(old_value);
            observer_destroy_string(new_value);
            CALLBACK_RUN_COUNT += 1;
        }
    }

    #[test]
    fn test_c_callback() {
        let mut err = ExternError::default();
        let person_handle = observer_person_new(FfiStr::from_cstr(NAME1.as_c_str()), &mut err);
        let observer_id = observer_person_observe_name(
            person_handle,
            std::ptr::null_mut(),
            observer_name_callback,
            None,
            &mut err,
        );
        observer_person_set_name(person_handle, FfiStr::from_cstr(NAME2.as_c_str()), &mut err);
        unsafe {
            SET_COUNT += 1;
            assert_eq!(1, CALLBACK_RUN_COUNT);
        }
        observer_person_unobserve_name(person_handle, observer_id, &mut err);
        observer_person_set_name(person_handle, FfiStr::from_cstr(NAME3.as_c_str()), &mut err);
        unsafe {
            SET_COUNT += 1;
            assert_eq!(1, CALLBACK_RUN_COUNT);
        }
    }
}

While this works, I'm particularly concerned about the following lines

unsafe impl Send for NameCallback {}
unsafe impl Sync for NameCallback {}

I couldn't get the code to work without them (as I used to get compile errors about not being able to send NameCallback over threads - and, other such issues). Is there a safer way to do this? Or, would I be fine, as long as the callbacks are not called on two threads at the same time (should be unlikely in my use case I think).

Edit: Added comments in code to better explain intent.

Edit: With regards to this comment,

When we add a callback, we get a observer id in return. This id is to be used to unregister a callback when we are no longer interested. Is there a good way to handle lifetime (esp over FFI)? Is it possible for me to defensively add checks on Person being deallocated, and an unregister call coming in later? Or, do I just have to be extra cautious with my code?

I guess it shouldn't matter. If Person goes out of scope, the callback will not be called again. If the observer/closure is released without unregistering, it could be an issue. But, since, we are providing a mechanism to unregister, I'm not sure if we can do better than expect well behaved clients.

\$\endgroup\$
3
  • \$\begingroup\$ While there are many recommended ways to do use constructs, that is... not one. I would add a separate use statement for each line at the top. \$\endgroup\$
    – lights0123
    Commented May 7, 2020 at 20:07
  • \$\begingroup\$ @lights0123 Aesthetically, I prefer this style (I find it easier to read), and, it seems to have made it into rust a few years ago. The RFC rust-lang.github.io/rfcs/2128-use-nested-groups.html#drawbacks mentions grepping being an issue with this style. Are there any other disadvantages to this style? Thanks \$\endgroup\$
    – georgemp
    Commented May 9, 2020 at 13:22
  • \$\begingroup\$ there's no problems with it, it's just unconventional. \$\endgroup\$
    – lights0123
    Commented May 9, 2020 at 14:43

0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Browse other questions tagged or ask your own question.