Repository: servo/core-foundation-rs (language: rust)

Issue:
CFRunLoop looks thread safe, but core-foundation-rs doesn't mark it as such
The core-foundation-rs `CFRunLoop` type does not appear to be marked as `Send` or `Sync`, and I suspect they should be.  

[Apple's Core Foundation docs appear to suggest those types are generally thread-safe][0] (emphasis mine):

> Core Foundation is sufficiently thread-safe that, if you program with care, you should not run into any problems related to competing threads. **It is thread-safe in the common cases, such as when you query, retain, release, and pass around immutable objects.** Even central shared objects that might be queried from more than one thread are reliably thread-safe.
>
> Like Cocoa, Core Foundation is not thread-safe when it comes to mutations to objects or their contents. (...)

Looking at another usage, `mozilla/authenticator-rs` passes a `CFRunLoop` between threads in a [`SendableRunLoop` type][1]: it [schedules a `CFRunLoopObserver` in a new thread][2], which then [sends back the current `CFRunLooop` over a `mpsc::channel`][3] and [picks it up from the current thread][4].

The C `__CFRunLoop` type also looks like it has [a mutex][6] around mutation operations ([example][7]), so it looks a lot like this should be safe to pass between threads... and you'd need to do this anyway if you want to schedule work on another thread.

Very few types in this crate ([currently four][5]) are marked as `Send` or `Sync`; and there's probably other types that could be marked that way too. Apple's documentation seems to suggest that Core Foundation types _could_ be non-thread-safe (just you should assume safe unless otherwise stated), so I don't think that implementing `Send + Sync` for `T: TCFType` is wise.

[0]: https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Multithreading/ThreadSafetySummary/ThreadSafetySummary.html#//apple_ref/doc/uid/10000057i-CH12-SW9
[1]: https://github.com/mozilla/authenticator-rs/blob/110b691ff105062352bcc9f00c36c77c42d4e45c/src/macos/iokit.rs#L60-L85
[2]: https://github.com/mozilla/authenticator-rs/blob/110b691ff105062352bcc9f00c36c77c42d4e45c/src/macos/transaction.rs#L45-L47
[3]: https://github.com/mozilla/authenticator-rs/blob/110b691ff105062352bcc9f00c36c77c42d4e45c/src/macos/transaction.rs#L78-L83
[4]: https://github.com/mozilla/authenticator-rs/blob/110b691ff105062352bcc9f00c36c77c42d4e45c/src/macos/transaction.rs#L67-L70
[5]: https://github.com/search?q=repo%3Aservo%2Fcore-foundation-rs%20%22impl%20Sync%22&type=code
[6]: https://github.com/apple-oss-distributions/CF/blob/dc54c6bb1c1e5e0b9486c1d26dd5bef110b20bf3/CFRunLoop.c#L637-L639
[7]: https://github.com/apple-oss-distributions/CF/blob/dc54c6bb1c1e5e0b9486c1d26dd5bef110b20bf3/CFRunLoop.c#L1263


Discussion / hints:

Fix patch:
diff --git a/core-foundation/src/runloop.rs b/core-foundation/src/runloop.rs
index be06f4ec7..fd1954d5f 100644
--- a/core-foundation/src/runloop.rs
+++ b/core-foundation/src/runloop.rs
@@ -26,6 +26,10 @@ declare_TCFType!(CFRunLoop, CFRunLoopRef);
 impl_TCFType!(CFRunLoop, CFRunLoopRef, CFRunLoopGetTypeID);
 impl_CFTypeDescription!(CFRunLoop);
 
+// https://github.com/servo/core-foundation-rs/issues/550
+unsafe impl Send for CFRunLoop {}
+unsafe impl Sync for CFRunLoop {}
+
 #[derive(Copy, Clone, Debug, PartialEq)]
 pub enum CFRunLoopRunResult {
     Finished = 1,
@@ -176,10 +180,14 @@ impl_TCFType!(CFRunLoopObserver, CFRunLoopObserverRef, CFRunLoopObserverGetTypeI
 #[cfg(test)]
 mod test {
     use super::*;
-    use date::{CFDate, CFAbsoluteTime};
+    use base::Boolean;
+    use date::{CFAbsoluteTime, CFDate};
     use std::mem;
     use std::os::raw::c_void;
+    use std::ptr::null_mut;
     use std::sync::mpsc;
+    use std::thread::spawn;
+    use std::time::Duration;
 
     #[test]
     fn wait_200_milliseconds() {
@@ -221,4 +229,62 @@ mod test {
         let _ = unsafe { (*info).elapsed_tx.send(elapsed) };
         CFRunLoop::get_current().stop();
     }
+
+    extern "C" fn observe(_: CFRunLoopObserverRef, _: CFRunLoopActivity, context: *mut c_void) {
+        let tx: &mpsc::Sender<CFRunLoop> = unsafe { &*(context as *const _) };
+        let _ = tx.send(CFRunLoop::get_current());
+    }
+
+    extern "C" fn observe_timer_popped(_: CFRunLoopTimerRef, _: *mut c_void) {
+        panic!("timer popped unexpectedly");
+    }
+
+    #[test]
+    fn observe_runloop() {
+        let (tx, rx) = mpsc::channel();
+        spawn(move || {
+            let mut context = CFRunLoopObserverContext {
+                version: 0,
+                info: &tx as *const _ as *mut c_void,
+                retain: None,
+                release: None,
+                copyDescription: None,
+            };
+
+            let observer = unsafe {
+                CFRunLoopObserver::wrap_under_create_rule(CFRunLoopObserverCreate(
+                    kCFAllocatorDefault,
+                    kCFRunLoopEntry,
+                    false as Boolean,
+                    0,
+                    observe,
+                    &mut context,
+                ))
+            };
+
+            let runloop = CFRunLoop::get_current();
+            runloop.add_observer(&observer, unsafe { kCFRunLoopDefaultMode });
+
+            let timer = CFRunLoopTimer::new(
+                CFDate::now().abs_time() + 1f64,
+                0f64,
+                0,
+                0,
+                observe_timer_popped,
+                null_mut(),
+            );
+            runloop.add_timer(&timer, unsafe { kCFRunLoopDefaultMode });
+
+            let result = unsafe {
+                CFRunLoop::run_in_mode(kCFRunLoopDefaultMode, Duration::from_secs(10), false)
+            };
+
+            assert_eq!(result, CFRunLoopRunResult::Stopped);
+
+            drop(tx);
+        });
+
+        let runloop: CFRunLoop = rx.recv().unwrap();
+        runloop.stop();
+    }
 }


Test patch: