Merge branch 'fix/event-callback-lifecycle' into 'master'

Manage event callback lifetimes

See merge request videolan/vlc-rs!28

src/core.rs

@@ -323,31 +323,102 @@ pub struct EventManager<'a> {
     pub(crate) _phantomdata: ::std::marker::PhantomData<&'a sys::libvlc_event_manager_t>,
 }
 
-impl<'a> EventManager<'a> {
+type EventCallback = dyn Fn(Event, VLCObject) + Send + 'static;
-    pub fn detach(&self, event_type: EventType, registered_callback: *mut c_void) {
+type EventCallbackBox = Box<EventCallback>;
-        unsafe { sys::libvlc_event_detach(self.ptr, event_type as i32, Some(event_manager_callback), registered_callback) }
-    }
 
-    pub fn attach<F>(&self, event_type: EventType, callback: F) -> Result<*mut c_void, ()>
+struct EventCallbackAllocation {
+    raw: *mut c_void,
+}
+
+impl EventCallbackAllocation {
+    fn new<F>(callback: F) -> EventCallbackAllocation
         where F: Fn(Event, VLCObject) + Send + 'static
     {
-        // Explicit type annotation is needed
+        let callback: Box<EventCallbackBox> = Box::new(Box::new(callback));
-        let callback: Box<Box<dyn Fn(Event, VLCObject) + Send + 'static>> =
+        EventCallbackAllocation { raw: Box::into_raw(callback) as *mut c_void }
-            Box::new(Box::new(callback));
+    }
 
-        let raw = Box::into_raw(callback) as *mut c_void;
+    fn raw(&self) -> *mut c_void {
+        self.raw
+    }
 
-        let result = unsafe{
+    unsafe fn reclaim(self) {
+        drop(Box::from_raw(self.raw as *mut EventCallbackBox));
+    }
+}
+
+fn attach_event_callback<F, A>(callback: F, attach: A) -> Result<EventCallbackAllocation, ()>
+    where F: Fn(Event, VLCObject) + Send + 'static,
+          A: FnOnce(*mut c_void) -> i32
+{
+    let callback = EventCallbackAllocation::new(callback);
+
+    if attach(callback.raw()) == 0 {
+        Ok(callback)
+    }else{
+        unsafe { callback.reclaim(); }
+        Err(())
+    }
+}
+
+fn release_event_callback<D>(callback: &mut Option<EventCallbackAllocation>, detach: D)
+    where D: FnOnce(*mut c_void)
+{
+    if let Some(callback) = callback.take() {
+        detach(callback.raw());
+        unsafe { callback.reclaim(); }
+    }
+}
+
+/// Owns an event callback registration.
+///
+/// Dropping this value detaches the callback and reclaims its allocation.
+pub struct EventSubscription<'a> {
+    event_manager: *mut sys::libvlc_event_manager_t,
+    event_type: EventType,
+    callback: Option<EventCallbackAllocation>,
+    _phantomdata: ::std::marker::PhantomData<&'a sys::libvlc_event_manager_t>,
+}
+
+impl<'a> EventSubscription<'a> {
+    /// Detach the callback now. Dropping the subscription has the same effect.
+    pub fn detach(mut self) {
+        self.release();
+    }
+
+    fn release(&mut self) {
+        let event_manager = self.event_manager;
+        let event_type = self.event_type;
+
+        release_event_callback(&mut self.callback, |callback| unsafe {
+            sys::libvlc_event_detach(
+                event_manager, event_type as i32, Some(event_manager_callback), callback);
+        });
+    }
+}
+
+impl<'a> Drop for EventSubscription<'a> {
+    fn drop(&mut self) {
+        self.release();
+    }
+}
+
+impl<'a> EventManager<'a> {
+    pub fn attach<F>(&self, event_type: EventType, callback: F) -> Result<EventSubscription<'a>, ()>
+        where F: Fn(Event, VLCObject) + Send + 'static
+    {
+        let callback = attach_event_callback(callback, |raw| unsafe {
             sys::libvlc_event_attach(
                 self.ptr, event_type as i32, Some(event_manager_callback),
                 raw)
-        };
+        })?;
 
-        if result == 0 {
+        Ok(EventSubscription{
-            Ok(raw)
+            event_manager: self.ptr,
-        }else{
+            event_type,
-            Err(())
+            callback: Some(callback),
-        }
+            _phantomdata: ::std::marker::PhantomData,
+        })
     }
 
     /// Returns raw pointer
@@ -357,11 +428,66 @@ impl<'a> EventManager<'a> {
 }
 
 unsafe extern "C" fn event_manager_callback(pe: *const sys::libvlc_event_t, data: *mut c_void) {
-    let f: &Box<dyn Fn(Event, VLCObject) + Send + 'static> = ::std::mem::transmute(data);
+    let f: &EventCallbackBox = ::std::mem::transmute(data);
 
     f(conv_event(pe), VLCObject{ ptr: (*pe).p_obj });
 }
 
+#[cfg(test)]
+mod event_callback_tests {
+    use super::{attach_event_callback, release_event_callback, Event, VLCObject};
+    use std::sync::atomic::{AtomicUsize, Ordering};
+    use std::sync::{Arc, Mutex};
+
+    struct DropCounter(Arc<AtomicUsize>);
+
+    impl Drop for DropCounter {
+        fn drop(&mut self) {
+            self.0.fetch_add(1, Ordering::SeqCst);
+        }
+    }
+
+    struct DropRecorder(Arc<Mutex<Vec<&'static str>>>);
+
+    impl Drop for DropRecorder {
+        fn drop(&mut self) {
+            self.0.lock().unwrap().push("drop");
+        }
+    }
+
+    #[test]
+    fn failed_attachment_reclaims_the_callback() {
+        let dropped = Arc::new(AtomicUsize::new(0));
+        let drop_counter = DropCounter(dropped.clone());
+
+        let result = attach_event_callback(
+            move |_: Event, _: VLCObject| { let _ = &drop_counter; },
+            |_| -1,
+        );
+
+        assert!(result.is_err());
+        assert_eq!(dropped.load(Ordering::SeqCst), 1);
+    }
+
+    #[test]
+    fn release_detaches_before_reclaiming_the_callback() {
+        let events = Arc::new(Mutex::new(Vec::new()));
+        let drop_recorder = DropRecorder(events.clone());
+        let mut callback = Some(attach_event_callback(
+            move |_: Event, _: VLCObject| {
+                let _ = &drop_recorder;
+            },
+            |_| 0,
+        ).unwrap());
+
+        release_event_callback(&mut callback, |_| events.lock().unwrap().push("detach"));
+        release_event_callback(&mut callback, |_| panic!("callback detached twice"));
+
+        assert!(callback.is_none());
+        assert_eq!(*events.lock().unwrap(), vec!["detach", "drop"]);
+    }
+}
+
 // Convert c-style libvlc_event_t to Event
 fn conv_event(pe: *const sys::libvlc_event_t) -> Event {
     let event_type: EventType = (unsafe{ (*pe).type_ } as u32).into();
@@ -579,4 +705,3 @@ impl Log {
         self.ptr
     }
 }
-