Observer pattern over FFI

Question

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.