Fix unsound unsafe impl Send + Sync for Display (issue #2)#4
Conversation
Replace the unsound `unsafe impl Sync for Display` with an internal `Mutex<SendableDevice>`. Concurrent calls to `set_mode` and `redraw` are now serialized by the lock, making `Display` genuinely thread-safe. `Display` gains `Send + Sync` automatically from `Mutex<SendableDevice>` without any explicit `unsafe impl` on `Display` itself. The remaining `unsafe impl Send for SendableDevice` is isolated in a small newtype with a safety comment explaining why `HidDevice` is safe to send across threads. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
cceckman
left a comment
cceckman
left a comment
There was a problem hiding this comment.
Thanks for taking a stab at this!
I've not had a chance to test this yet; in fact, I had some issues earlier (before this PR) getting a response, but that might be on my end. I'll take another stab at it when I get a chance.
In the mean time, I think we might be OK with just the Mutex; it looks like HidDevice is already Send?
| /// Safety: The C hidapi library serializes concurrent access to a device handle | ||
| /// internally, so it is safe to send an HidDevice to another thread. | ||
| struct SendableDevice(HidDevice); | ||
| unsafe impl Send for SendableDevice {} |
There was a problem hiding this comment.
It looks like HidDevice should already be Send.
The inner device has its own unsafe impl:
and it's a restriction on HidDeviceBackend:
https://github.com/ruabmbua/hidapi-rs/blob/7c8358fd275b394df588cc7ad513693b46d4d908/src/lib.rs#L121
So I think HidDevice should already be Send. If we remove this unsafe impl, does everything still compile?
In general, the unsafe code should only be in the crate that does the unsafe->safe bridge, FFI and whatnot. (In this case,hidapi.) It tells us what guarantees are provided.
| /// Safety: The C hidapi library serializes concurrent access to a device handle | ||
| /// internally, so it is safe to send an HidDevice to another thread. |
There was a problem hiding this comment.
I didn't see this in the C code or the docs, but I'm less of an expert there; do you have a link to where this gets implemented / how it's accomplished?
If it's internally synchronized, it should implement Sync as well. But given the hidapi crate doesn't implement Sync for HidDevice, I'm suspicious that it doesn't actually have internal synchronization.
(Even if it does implement Sync, we might want a Mutex on our accessor to synchronize the Glider-level transactions.)
| #[pyclass(frozen)] | ||
| pub struct Display { | ||
| device: HidDevice, | ||
| device: Mutex<SendableDevice>, |
There was a problem hiding this comment.
| device: Mutex<SendableDevice>, | |
| // The Glider API has a request/response protocol. We want to make sure we capture the response for a given request, so we lock the device for each transaction. | |
| device: Mutex<SendableDevice>, |
2 participants
Addresses #2 - unsafe impl Send + Sync are not safe
Replace the unsound
unsafe impl Sync for Displaywith an internalMutex<SendableDevice>. Concurrent calls toset_modeandredraware now serialized by the lock, makingDisplaygenuinely thread-safe.DisplaygainsSend + Syncautomatically fromMutex<SendableDevice>without any explicitunsafe implonDisplayitself.The remaining
unsafe impl Send for SendableDeviceis isolated in a small newtype with a safety comment explaining whyHidDeviceis safe to send across threads.