Impl Send+Sync for CFRunLoopTimer, CFRunLoopSource, CFRunLoopObserver

[kevinmehall]

Following #610, the same reasoning from #550 applies to the remaining CFRunLoop types.

[waywardmonkeys]

I didn't see a clear reference in the Apple docs about the thread safety of these APIs, apart from that you can register them in a single runloop but not multiple.

@madsmtm What do you think of this? It looks like you looked into it some before in the issue that you linked.

[madsmtm]

CFRunLoopSource is thread safe to access and signal from other threads than the one that created it, that's kinda the entire point of it.

I don't dare say anything about the others, though I will reference this piece of documentation and that CoreFoundation is open source, then you have the same to work with as I do ;).

[madsmtm]

Though actually, all of these carry callbacks that the user sets, and those callbacks may not be Send + Sync, so even though the underlying types themselves are thread safe, I'm not sure I'd make them Send + Sync in Rust.

[waywardmonkeys]

@kevinmehall What is your goal with this change? (I am away from home all day today running errands)

[kevinmehall]

In nusb, I'm launching a dedicated background thread for a CFRunLoop to handle IOKit events whose callbacks wake async tasks. Since the initial reference to the CFRunLoop can only be obtained on its own thread by CFRunLoop::get_current(), and it needs a CFRunLoopSource added before it can run, I'm sending the CFRunLoopSource in the thread::spawn closure, which sends the CFRunLoop back before running the loop.

https://github.com/kevinmehall/nusb/blob/main/src/platform/macos_iokit/events.rs

I ended up doing this by making newtype wrappers around these types that I could put Send + Sync impls on. Like winit, I actually also only need CFRunLoopSource; the others were included in this PR just because they seemed to have the same properties.

Good point on the callbacks -- Looking at the safe functions in this crate that accept callbacks that end up in these types (CFRunLoopTimer::new, CFFileDescriptor::new), the callbacks are not closures but function pointers that can't carry data and are inherently Send + Sync, along with a raw pointer for context. Doing anything with that context pointer requires unsafe code in the callback, so technically it might be fine to say that unsafe block has the responsibility to ensure that the data behind the context pointer is safe to access there. That game of trying to argue that one particular unsafe block carries the responsibility for a bunch of safe code outside it to enforce a global property isn't ideal, though.

I'm fine with needing to write unsafe for this, but my newtype wrappers also feel like the wrong way to go about it.

Also, I think making CFRunLoop itself Send+Sync in #610, and the existence of CFRunLoop::get_main before that, is equivalent in power, because if the thread that constructs the sources can obtain another thread's CFRunLoop reference, it can call add_source and the callbacks will be called on that other thread. So probably all of these should be Send + Sync or none of them, but the status quo is inconsistent.

[madsmtm]

Good points overall!

Perhaps a solution would be to make CFRunLoopAddSource/CFRunLoopAddObserver/CFRunLoopAddTimer unsafe, with the safety requirement that the thing we're adding has callbacks that are thread-safe on the thread that's going to run the runloop?

This way, e.g. CFRunLoopObserver by itself can be Send + Sync, regardless of whether the data pointed to by the observer's info pointer is Send + Sync (given that the retain/release/... methods in CFRunLoopObserverContext are thread-safe, but that seems like a reasonable requirement to impose).