gh-libusb/hidapi

Watch

Fork 0

mac: deliver hotplug ENUMERATE pass asynchronously on the event context #824

Open

Youw wants to merge 10 commits from hotplug-async-enumerate-mac into connection-callback

Youw commented

2026-07-13 17:40:54 -05:00

(Migrated from github.com)

Implements the hotplug contract documented in hidapi/hidapi.h (#790) for the mac backend, and fixes the self-join hang of #794.

What changed

Asynchronous HID_API_HOTPLUG_ENUMERATE pass. hid_hotplug_register_callback() no longer invokes the callback synchronously on the calling thread. Instead it takes a deep-copied, registration-time snapshot of the matching entries of the internal device cache (under the hotplug mutex) and stores it on the callback record (replay). The snapshot is delivered on the hotplug event thread, one device per invocation (device->next always NULL), exactly once, and never from within the register call itself.
Delivery vehicle: a second CFRunLoopSource (replay_source), the same pattern as the existing stop source. Register signals it and wakes the event thread's run loop; its perform routine flushes all pending replays under the mutex with mutex_in_use set, honoring a non-zero callback return (deregisters the callback and drops the rest of its pass). CFRunLoopPerformBlock was avoided on purpose: it relies on the blocks extension, which is a portability risk for the plain-C/C++ builds.
Pass-before-live ordering. Run loop source ordering vs. the IOHID source is not guaranteed, so hid_internal_invoke_callbacks() (reached from both the IOHID connect and disconnect callbacks) flushes a callback's pending replay before dispatching any live event to it. A callback registered with ENUMERATE for both event types therefore never sees a DEVICE_LEFT for a device whose arrival it has not been told about first.
Self-join fix (#794). When a callback deregisters the last callback in response to an event, hid_internal_hotplug_cleanup() is reached on the event thread itself and used to pthread_join() its own thread - a permanent hang. Cleanup now detects this with pthread_equal(pthread_self(), ...): it still requests the stop (state + stop source + wake) but defers the join. The join is performed with the hotplug mutex released by a dedicated collector (hid_internal_hotplug_collect_thread(), serialized through the join_done condition variable) from register/deregister/hid_exit(); when the last callback is removed from the event thread itself and nobody is joining, the thread detaches and releases its own resources in its epilogue.
Implicit hid_init(). Register now initializes the library implicitly, per contract. The hotplug IOHIDManager remains fully independent of hid_mgr.
hid_exit() leak. Hotplug teardown was guarded by if (hid_mgr), so hotplug-only usage never stopped the event thread or freed the callback list. The teardown now runs unconditionally. The run_loop_mode CFString (previously leaked) is released there as well.
Error strings and out-param. All register/deregister failure paths set the global error (arg validation, allocation, thread creation, event-thread setup, snapshot failure). On any register failure *callback_handle (when non-NULL) is set to 0; on success it is written before any event can be delivered. (when called from an application thread; per hidapi.h, a call made from within a hotplug callback reports only -1 and leaves hid_error(NULL) untouched)
Deregister hardening. Added the missing callback_handle <= 0 early rejection (parity with the other backends); an unknown/stale handle stays a safe no-op returning -1. Deregister and hid_exit() free undelivered replay entries, so a deregistered callback never fires again, including its pending synthetic events.
device->next == NULL for every invocation. The IOHID connect callback previously passed chained hid_device_info entries (multi-usage devices) straight into the callbacks; each entry is now detached for the duration of its delivery.
Event-thread startup failures (manager or run loop source creation) are now reported back through the startup barrier via startup_ok, which the registrant publishes under the mutex, unwound cleanly, and turned into a -1 with an error string. pthread_create failure is likewise handled. dup_wcs() got a missing malloc NULL-check so snapshot OOM fails cleanly instead of crashing.

Design decisions / deviations

Snapshot OOM fails the whole registration (rather than delivering a partial initial pass), keeping the exactly-once guarantee trivially intact. The callback is safely detached because the mutex has been held since before it became visible.
The tail hid_internal_hotplug_cleanup() call in register's success path was dropped: with no synchronous invocation left, the list is guaranteed non-empty there and the call could never do anything.
A callback that registers a new ENUMERATE callback from within an event may have the new callback's pass flushed within the same dispatch cycle (still after register returns, still on the event context) - permitted by the contract ("may fire before or after it returns").

Known limitations

~~A cross-thread deadlock window predates this PR and is left as is: cleanup joins the event thread while holding the hotplug mutex.~~ Fixed in round 1 (the join moved to a dedicated collector that runs with the mutex released) and hardened in round 2 (condition variable instead of a spin).
~~A callback registered during a disconnect dispatch may observe that DEVICE_LEFT without a prior arrival.~~ Fixed in round 1: each dispatch is frozen at the tail callback present when it started, so a callback registered from within a callback never receives the in-flight event.
The implicit hid_init() schedules the global hid_mgr on the registering thread's run loop; see the round-2 notes below.

Addresses #793 for the mac backend.
Fixes: #794

Assisted-by: claude-code:claude-fable-5
Assisted-by: claude-code:claude-opus-5
Assisted-by: claude-code:claude-fable-5-1
Assisted-by: codex-cli:gpt-6-astra

Review round 1

Reviewed independently by two reviewers against the #790 contract; all confirmed findings are addressed in 1506abe:

Join-under-mutex deadlock (blocker, both reviewers) — the new replay run-loop source made a long-latent pattern reachable from a plain register-with-ENUMERATE + deregister sequence: pthread_join ran with the hotplug mutex held while the event thread blocked on that mutex inside the replay perform. The join moved out of hid_internal_hotplug_cleanup() into a dedicated collector invoked from the public entry points with the mutex released, serializing concurrent joiners.
Startup burst drained in the wrong run-loop mode (both reviewers) — process_pending_events() ran the default mode while the hotplug manager is scheduled in the private mode, so the device cache was empty when the first registrant took its snapshot (the ENUMERATE mechanism never engaged) and pre-connected devices surfaced as live arrivals even without the flag. The burst is now drained in the private mode before the startup barrier is released. Corrected in round 2: a timed 1 ms pump is a timing fence, not a completion fence — a slower burst still leaked pre-connected devices as live arrivals, and letting removals be delivered during that window turned the unconditional dispatch in the disconnect callback into a hard hang. The snapshot boundary is now synchronous.
Initialization and error-reporting races — the one-time setup (implicit hid_init + hotplug mutex creation) was serialized by a static bootstrap mutex (two racing first registrations could double-initialize the mutex and leak a second IOHIDManager), and all mutations of the global error string by a static mutex (concurrent thread-safe failures could double-free it).
Corrected in round 2 — two claims made here were false: the bootstrap mutex did not serialize hid_exit() against register/deregister (register released it before taking the hotplug mutex; deregister never took it at all), and it introduced an ABBA deadlock with register-from-callback; and the writer-only error mutex did not make hid_error(NULL) — which reads the pointer unlocked — safe against another thread. Both are superseded below; the bootstrap mutex is gone (pthread_once + an in-mutex exiting guard) and the hotplug mutex is now never destroyed.
Mid-dispatch registration ordering — each dispatch is frozen at the tail callback present at dispatch start, and arriving cache entries are appended before callbacks are invoked, so a callback registered from within a callback never receives the in-flight event and its snapshot covers that arrival exactly once (previously it could observe DEVICE_LEFT without a prior arrival).
Also: IOHIDManagerOpen result checked (registration fails instead of returning an inert handle); pthread_barrier_init checked and the shim's error checks fixed (pthread errors are positive, not negative); deregistering an already-deregistered handle returns -1; handle exhaustion fails registration instead of wrapping; the unsolicited run-loop exit is published under the mutex; mutex_in_use save/restore discipline made consistent.

Review round 2

Reviewed independently by two fresh reviewers. They found that two of the round-1 fixes were themselves hangs, and that the round-1 commit message/PR text advertised two guarantees it did not actually deliver (corrected in the round-1 section above). All findings are addressed in 93086d9.

Blockers

ABBA deadlock introduced by the round-1 bootstrap mutex. Two lock orders existed for (bootstrap, hotplug): hid_exit() took bootstrap в†’ hotplug, while a hid_hotplug_register_callback() from inside a callback — which the contract explicitly sanctions and promises "cannot deadlock" — already holds the hotplug mutex and then took bootstrap. A bootstrap lock ordered outside the hotplug mutex is fundamentally incompatible with register-from-callback, so it is gone: the one-time setup now uses pthread_once(), and the hid_exit() teardown is guarded from inside the hotplug mutex by an exiting flag. The single global lock order is stated in a comment block above the context: hid_hotplug_context.mutex (outermost) в†’ { global_error_mutex, startup_barrier } (leaves); pthread_join() is only ever called with the hotplug mutex released, pthread_cond_wait() only at recursion level 1.
Removal during the startup drain hung the first registration. The thread_state > 0 guard in the disconnect callback wrapped only its explicit lock/unlock — the hid_internal_invoke_callbacks() call inside the loop was unconditional and locks the hotplug mutex. Since round 1 made the private-mode drain actually deliver events, unplugging a device during the initial enumeration made the event thread block on the mutex held by the first registrant, which was parked at the startup barrier: both threads stuck forever, with the hotplug mutex held. The dispatch is now hoisted under the same guard (at startup the callback list is provably empty, so skipping the dispatch is semantically free), and hid_internal_invoke_callbacks() + the replay perform carry a defensive early-return for the same reason.

Majors

The "drain" was a timing fence, not a completion fence. CFRunLoopRunInMode(mode, 0.001, FALSE) returns on timeout, so an initial matching burst slower than ~1 ms (very possible: create_device_info() does IORegistry queries per device) left the registrant snapshotting only the processed prefix, and the remaining pre-connected devices were then delivered as live arrivals — even to a callback registered without HID_API_HOTPLUG_ENUMERATE. The snapshot boundary is now a real one: the cache is completed synchronously with IOHIDManagerCopyDevices() (the same call, and the same synchronous behavior, hid_enumerate() relies on) before the startup barrier, and the matching burst the manager replays once the run loop runs is deduplicated by io_service_t against the cache, so a device that was connected at snapshot time can never surface as a live arrival, no matter how slow the burst is. The 1 ms pump is kept ahead of the snapshot as best-effort insurance only (it can only add devices to the cache, never move one from the snapshot to the live events) — nothing depends on it completing, and it now also honors kCFRunLoopRunStopped.
hid_exit() vs register/deregister (the false round-1 claim). The bootstrap mutex did not serialize them: register released it before touching the hotplug mutex, and deregister never took it at all (it read mutex_ready unsynchronized, then locked). hid_exit() could therefore pthread_mutex_destroy() the hotplug mutex between another thread's check and its lock. Mirroring the decision taken on the windows backend: the hotplug mutex is never destroyed (it lives for the process lifetime, mutex_ready is never reset), and an explicit exiting state, set under the hotplug mutex at the top of the teardown, is observed by register (fails with -1 and an error string) and deregister (-1, no-op). exiting stays set across the whole of hid_exit(), so the implicit hid_init() in register — now performed under the hotplug mutex — cannot race the destruction of hid_mgr either.
Global error string vs the event thread (the other false round-1 claim). The round-1 mutex is writer-only, while hid_error(NULL) returns the raw pointer unlocked, so it never made hid_error(NULL) safe against an internal thread. Audited: no HIDAPI-internal event context in mac/hid.c writes the global error (the event thread, the IOKit callbacks and the replay perform report failures through internal state only; create_device_info() and the cache builder never touch it). The only writes reachable from the event thread are those an application's own callback initiates by calling register/deregister there — legitimized by the header amendment documenting that hid_error(NULL) must be serialized against register/deregister. The writer-side mutex is kept for those app-thread writes.
Join spin could starve the thread it waits for. The join_in_progress + sched_yield() spin re-acquired the hotplug mutex every iteration while the joiner sat in pthread_join() waiting for an event thread that needs that same mutex to finish its in-flight dispatch (Darwin mutexes are not FIFO-fair). Replaced with a condition variable (join_done), broadcast by the collector after clearing join_in_progress/thread_needs_join. The join protocol itself (no double join, no thread-handle reuse, no barrier re-init race) is unchanged.
Lifecycle state was racy. thread_state was written by the event thread without the mutex and read unlocked at the IOKit callback entries and in the loop condition, while also being written under the mutex elsewhere. Now all lifecycle state (thread_state, thread_needs_join, join_in_progress, exiting) is read and written exclusively under the hotplug mutex — the event thread no longer writes thread_state at all before the barrier; it reports its result in startup_ok, which the registrant (holding the mutex) publishes. The one documented exception is the event thread's startup phase, where the registrant holds the mutex parked at the barrier and the thread therefore has exclusive access — and must not take the mutex. The startup guard used by the IOKit callbacks is a separate, event-thread-private flag, so it is not a racy read of shared state.

Minors

hid_internal_hotplug_init() now validates every pthread_mutexattr_init/settype/mutex_init/cond_init call, unwinds on failure and leaves the hotplug API unavailable; register/deregister fail with a retrievable error instead of locking an invalid — or worse, non-recursive — mutex.
A registration can no longer succeed against an event thread that died on its own (unsolicited run-loop exit): a stopped thread with callbacks still registered is detected under the mutex and the registration fails, instead of returning 0 for a callback that would receive neither its ENUMERATE pass nor any live event.
Deregistering the last callback from inside a callback (including by returning non-zero) no longer leaves a zombie thread, two leaked CFRunLoopSources and the run loop behind until some later API call: the event thread now detaches and releases itself in an epilogue when nobody is joining it (the decision is taken under the mutex on both sides, so there is no double join and no join of a detached thread).
The implicit hid_init() from register schedules the global hid_mgr on the registering thread's run loop, which hid_enumerate()/hid_open() on another thread will not service. Nothing depends on it (the manager answers IOHIDManagerCopyDevices() synchronously), and changing init_hid_manager()'s run-loop scheduling would alter the non-hotplug enumeration path, so the behavior is documented at the call site instead: applications wanting the classic behavior should call hid_init() explicitly, from the thread they use HIDAPI on, before registering.
Nits: register clears the stale global error on success; the thread-failed-to-start unwind is now symmetric with the snapshot-failure one (it frees a partially built cache); hid_internal_copy_device_info() documents that it must be updated when struct hid_device_info gains a field, and the hid_device_info / hid_device_info_ex allocation asymmetry (a copy must never be fed back to match_ref_to_info()); the manager is closed on the new post-IOHIDManagerOpen failure path.

Verification

Manual only for the concurrency work — macOS has no hotplug hardware coverage in CI. Full re-read of the file; a written global lock-order statement audited against every acquisition; brace/paren balance and lock/unlock + CFRetain/CFRelease pairing checked on every path including all failure unwinds; explicit race walkthroughs for hid_exit() vs a callback that registers, removal during startup, a slow initial burst, hid_exit() racing deregister, last-callback self-deregistration, and concurrent joiners. C and C++ portability builds stay clean (no designated initializers, no intв†’enum).

Final state (after review)

Built on the #794 self-join fix and restructured further in review: an interim bootstrap mutex (which introduced an ABBA deadlock against register-from-callback) was removed in favor of pthread_once + an in-mutex exiting flag; the device cache is built synchronously from IOHIDManagerCopyDevices when the event thread starts (before it goes live), and every registration's ENUMERATE snapshot is a deep copy of that live cache taken under the hotplug mutex; device identity is io_service_t with a MACH_PORT_NULL guard (devices that report no backing io_service_t are excluded from hotplug (cache and events) because they cannot be matched for dedupe or removal; hid_enumerate() still lists them with an empty path); and internal-event-thread writes to the global error string are suppressed via the captured event-thread id.

Verification. Every fix commit on this branch was adversarially delta-verified by an independent reviewer (Codex on a separate plan, or a fresh model) tasked to find what the fix itself broke — a loop that caught a defect in nearly every fix commit. A per-backend white-box harness injects fake device events into the real implementation and runs under ASan/UBSan/LSan and TSan (TSan proven live). The hotplug-integration branch — all four final backends merged with the test suite (#826) — is CI-green on every platform, with HotplugAPI_darwin executing per push (the device-backed Hotplug_darwin still self-skips).

Implements device->next == NULL (#791, #792) and the ENUMERATE return-value (#793) contract for darwin. Part of hotplug support (#238); contract documented in #790.

Assisted-by: claude-code:claude-opus-4-8

Implements the hotplug contract documented in `hidapi/hidapi.h` (#790) for the mac backend, and fixes the self-join hang of #794. ## What changed - **Asynchronous `HID_API_HOTPLUG_ENUMERATE` pass.** `hid_hotplug_register_callback()` no longer invokes the callback synchronously on the calling thread. Instead it takes a deep-copied, registration-time snapshot of the matching entries of the internal device cache (under the hotplug mutex) and stores it on the callback record (`replay`). The snapshot is delivered on the hotplug event thread, one device per invocation (`device->next` always `NULL`), exactly once, and never from within the register call itself. - **Delivery vehicle: a second `CFRunLoopSource`** (`replay_source`), the same pattern as the existing stop source. Register signals it and wakes the event thread's run loop; its perform routine flushes all pending replays under the mutex with `mutex_in_use` set, honoring a non-zero callback return (deregisters the callback and drops the rest of its pass). `CFRunLoopPerformBlock` was avoided on purpose: it relies on the blocks extension, which is a portability risk for the plain-C/C++ builds. - **Pass-before-live ordering.** Run loop source ordering vs. the IOHID source is not guaranteed, so `hid_internal_invoke_callbacks()` (reached from both the IOHID connect and disconnect callbacks) flushes a callback's pending replay before dispatching any live event to it. A callback registered with `ENUMERATE` for both event types therefore never sees a `DEVICE_LEFT` for a device whose arrival it has not been told about first. - **Self-join fix (#794).** When a callback deregisters the last callback in response to an event, `hid_internal_hotplug_cleanup()` is reached on the event thread itself and used to `pthread_join()` its own thread - a permanent hang. Cleanup now detects this with `pthread_equal(pthread_self(), ...)`: it still requests the stop (state + stop source + wake) but defers the join. The join is performed with the hotplug mutex released by a dedicated collector (hid_internal_hotplug_collect_thread(), serialized through the join_done condition variable) from register/deregister/hid_exit(); when the last callback is removed from the event thread itself and nobody is joining, the thread detaches and releases its own resources in its epilogue. - **Implicit `hid_init()`.** Register now initializes the library implicitly, per contract. The hotplug IOHIDManager remains fully independent of `hid_mgr`. - **`hid_exit()` leak.** Hotplug teardown was guarded by `if (hid_mgr)`, so hotplug-only usage never stopped the event thread or freed the callback list. The teardown now runs unconditionally. The `run_loop_mode` CFString (previously leaked) is released there as well. - **Error strings and out-param.** All register/deregister failure paths set the global error (arg validation, allocation, thread creation, event-thread setup, snapshot failure). On any register failure `*callback_handle` (when non-NULL) is set to 0; on success it is written before any event can be delivered. (when called from an application thread; per hidapi.h, a call made from within a hotplug callback reports only -1 and leaves hid_error(NULL) untouched) - **Deregister hardening.** Added the missing `callback_handle <= 0` early rejection (parity with the other backends); an unknown/stale handle stays a safe no-op returning -1. Deregister and `hid_exit()` free undelivered replay entries, so a deregistered callback never fires again, including its pending synthetic events. - **`device->next == NULL` for every invocation.** The IOHID connect callback previously passed chained `hid_device_info` entries (multi-usage devices) straight into the callbacks; each entry is now detached for the duration of its delivery. - Event-thread startup failures (manager or run loop source creation) are now reported back through the startup barrier via startup_ok, which the registrant publishes under the mutex, unwound cleanly, and turned into a -1 with an error string. `pthread_create` failure is likewise handled. `dup_wcs()` got a missing malloc NULL-check so snapshot OOM fails cleanly instead of crashing. ## Design decisions / deviations - **Snapshot OOM fails the whole registration** (rather than delivering a partial initial pass), keeping the exactly-once guarantee trivially intact. The callback is safely detached because the mutex has been held since before it became visible. - The tail `hid_internal_hotplug_cleanup()` call in register's success path was dropped: with no synchronous invocation left, the list is guaranteed non-empty there and the call could never do anything. - A callback that registers a new `ENUMERATE` callback from within an event may have the new callback's pass flushed within the same dispatch cycle (still after register returns, still on the event context) - permitted by the contract ("may fire before or after it returns"). ## Known limitations - ~~A cross-thread deadlock window predates this PR and is left as is: cleanup joins the event thread while holding the hotplug mutex.~~ **Fixed in round 1** (the join moved to a dedicated collector that runs with the mutex released) and hardened in round 2 (condition variable instead of a spin). - ~~A callback registered during a disconnect dispatch may observe that `DEVICE_LEFT` without a prior arrival.~~ **Fixed in round 1**: each dispatch is frozen at the tail callback present when it started, so a callback registered from within a callback never receives the in-flight event. - The implicit `hid_init()` schedules the global `hid_mgr` on the *registering* thread's run loop; see the round-2 notes below. Addresses #793 for the mac backend. Fixes: #794 Assisted-by: claude-code:claude-fable-5 Assisted-by: claude-code:claude-opus-5 Assisted-by: claude-code:claude-fable-5-1 Assisted-by: codex-cli:gpt-6-astra ## Review round 1 Reviewed independently by two reviewers against the #790 contract; all confirmed findings are addressed in `1506abe`: - **Join-under-mutex deadlock** (blocker, both reviewers) — the new replay run-loop source made a long-latent pattern reachable from a plain register-with-ENUMERATE + deregister sequence: `pthread_join` ran with the hotplug mutex held while the event thread blocked on that mutex inside the replay perform. The join moved out of `hid_internal_hotplug_cleanup()` into a dedicated collector invoked from the public entry points with the mutex released, serializing concurrent joiners. - **Startup burst drained in the wrong run-loop mode** (both reviewers) — `process_pending_events()` ran the *default* mode while the hotplug manager is scheduled in the private mode, so the device cache was empty when the first registrant took its snapshot (the ENUMERATE mechanism never engaged) and pre-connected devices surfaced as *live* arrivals even without the flag. The burst is now drained in the private mode before the startup barrier is released. **Corrected in round 2:** a timed 1 ms pump is a timing fence, not a completion fence — a slower burst still leaked pre-connected devices as live arrivals, and letting removals be delivered during that window turned the unconditional dispatch in the disconnect callback into a hard hang. The snapshot boundary is now synchronous. - **Initialization and error-reporting races** — the one-time setup (implicit `hid_init` + hotplug mutex creation) was serialized by a static bootstrap mutex (two racing first registrations could double-initialize the mutex and leak a second IOHIDManager), and all *mutations* of the global error string by a static mutex (concurrent thread-safe failures could double-free it). **Corrected in round 2 — two claims made here were false:** the bootstrap mutex did *not* serialize `hid_exit()` against register/deregister (register released it before taking the hotplug mutex; deregister never took it at all), and it introduced an ABBA deadlock with register-from-callback; and the writer-only error mutex did *not* make `hid_error(NULL)` — which reads the pointer unlocked — safe against another thread. Both are superseded below; the bootstrap mutex is gone (`pthread_once` + an in-mutex `exiting` guard) and the hotplug mutex is now never destroyed. - **Mid-dispatch registration ordering** — each dispatch is frozen at the tail callback present at dispatch start, and arriving cache entries are appended before callbacks are invoked, so a callback registered from within a callback never receives the in-flight event and its snapshot covers that arrival exactly once (previously it could observe `DEVICE_LEFT` without a prior arrival). - Also: `IOHIDManagerOpen` result checked (registration fails instead of returning an inert handle); `pthread_barrier_init` checked and the shim's error checks fixed (pthread errors are positive, not negative); deregistering an already-deregistered handle returns -1; handle exhaustion fails registration instead of wrapping; the unsolicited run-loop exit is published under the mutex; `mutex_in_use` save/restore discipline made consistent. ## Review round 2 Reviewed independently by two fresh reviewers. They found that **two of the round-1 fixes were themselves hangs**, and that the round-1 commit message/PR text advertised two guarantees it did not actually deliver (corrected in the round-1 section above). All findings are addressed in `93086d9`. ### Blockers - **ABBA deadlock introduced by the round-1 bootstrap mutex.** Two lock orders existed for (bootstrap, hotplug): `hid_exit()` took bootstrap в†’ hotplug, while a `hid_hotplug_register_callback()` *from inside a callback* — which the contract explicitly sanctions and promises "cannot deadlock" — already holds the hotplug mutex and then took bootstrap. A bootstrap lock ordered *outside* the hotplug mutex is fundamentally incompatible with register-from-callback, so it is gone: the one-time setup now uses `pthread_once()`, and the `hid_exit()` teardown is guarded from *inside* the hotplug mutex by an `exiting` flag. The single global lock order is stated in a comment block above the context: `hid_hotplug_context.mutex` (outermost) в†’ `{ global_error_mutex, startup_barrier }` (leaves); `pthread_join()` is only ever called with the hotplug mutex released, `pthread_cond_wait()` only at recursion level 1. - **Removal during the startup drain hung the first registration.** The `thread_state > 0` guard in the disconnect callback wrapped only its explicit lock/unlock — the `hid_internal_invoke_callbacks()` call inside the loop was unconditional and locks the hotplug mutex. Since round 1 made the private-mode drain actually deliver events, unplugging a device during the initial enumeration made the event thread block on the mutex held by the first registrant, which was parked at the startup barrier: both threads stuck forever, with the hotplug mutex held. The dispatch is now hoisted under the same guard (at startup the callback list is provably empty, so skipping the dispatch is semantically free), and `hid_internal_invoke_callbacks()` + the replay perform carry a defensive early-return for the same reason. ### Majors - **The "drain" was a timing fence, not a completion fence.** `CFRunLoopRunInMode(mode, 0.001, FALSE)` returns on *timeout*, so an initial matching burst slower than ~1 ms (very possible: `create_device_info()` does IORegistry queries per device) left the registrant snapshotting only the processed prefix, and the remaining pre-connected devices were then delivered as *live* arrivals — even to a callback registered *without* `HID_API_HOTPLUG_ENUMERATE`. The snapshot boundary is now a real one: the cache is completed **synchronously** with `IOHIDManagerCopyDevices()` (the same call, and the same synchronous behavior, `hid_enumerate()` relies on) before the startup barrier, and the matching burst the manager replays once the run loop runs is **deduplicated by `io_service_t`** against the cache, so a device that was connected at snapshot time can never surface as a live arrival, no matter how slow the burst is. The 1 ms pump is kept *ahead* of the snapshot as best-effort insurance only (it can only add devices to the cache, never move one from the snapshot to the live events) — nothing depends on it completing, and it now also honors `kCFRunLoopRunStopped`. - **`hid_exit()` vs register/deregister (the false round-1 claim).** The bootstrap mutex did not serialize them: register released it before touching the hotplug mutex, and deregister never took it at all (it read `mutex_ready` unsynchronized, then locked). `hid_exit()` could therefore `pthread_mutex_destroy()` the hotplug mutex between another thread's check and its lock. Mirroring the decision taken on the windows backend: **the hotplug mutex is never destroyed** (it lives for the process lifetime, `mutex_ready` is never reset), and an explicit `exiting` state, set under the hotplug mutex at the top of the teardown, is observed by register (fails with -1 and an error string) and deregister (-1, no-op). `exiting` stays set across the whole of `hid_exit()`, so the implicit `hid_init()` in register — now performed under the hotplug mutex — cannot race the destruction of `hid_mgr` either. - **Global error string vs the event thread (the other false round-1 claim).** The round-1 mutex is writer-only, while `hid_error(NULL)` returns the raw pointer unlocked, so it never made `hid_error(NULL)` safe against an internal thread. Audited: no HIDAPI-internal event context in `mac/hid.c` writes the global error (the event thread, the IOKit callbacks and the replay perform report failures through internal state only; `create_device_info()` and the cache builder never touch it). The only writes reachable from the event thread are those an application's own callback initiates by calling register/deregister there — legitimized by the header amendment documenting that `hid_error(NULL)` must be serialized against register/deregister. The writer-side mutex is kept for those app-thread writes. - **Join spin could starve the thread it waits for.** The `join_in_progress` + `sched_yield()` spin re-acquired the hotplug mutex every iteration while the joiner sat in `pthread_join()` waiting for an event thread that needs that same mutex to finish its in-flight dispatch (Darwin mutexes are not FIFO-fair). Replaced with a condition variable (`join_done`), broadcast by the collector after clearing `join_in_progress`/`thread_needs_join`. The join protocol itself (no double join, no thread-handle reuse, no barrier re-init race) is unchanged. - **Lifecycle state was racy.** `thread_state` was written by the event thread without the mutex and read unlocked at the IOKit callback entries and in the loop condition, while also being written under the mutex elsewhere. Now *all* lifecycle state (`thread_state`, `thread_needs_join`, `join_in_progress`, `exiting`) is read and written exclusively under the hotplug mutex — the event thread no longer writes `thread_state` at all before the barrier; it reports its result in `startup_ok`, which the registrant (holding the mutex) publishes. The one documented exception is the event thread's startup phase, where the registrant holds the mutex parked at the barrier and the thread therefore has exclusive access — and must *not* take the mutex. The startup guard used by the IOKit callbacks is a separate, event-thread-private flag, so it is not a racy read of shared state. ### Minors - `hid_internal_hotplug_init()` now validates every `pthread_mutexattr_init/settype/mutex_init/cond_init` call, unwinds on failure and leaves the hotplug API unavailable; register/deregister fail with a retrievable error instead of locking an invalid — or worse, non-recursive — mutex. - A registration can no longer succeed against an event thread that died on its own (unsolicited run-loop exit): a stopped thread with callbacks still registered is detected under the mutex and the registration fails, instead of returning 0 for a callback that would receive neither its `ENUMERATE` pass nor any live event. - Deregistering the **last** callback from inside a callback (including by returning non-zero) no longer leaves a zombie thread, two leaked `CFRunLoopSource`s and the run loop behind until some later API call: the event thread now detaches and releases itself in an epilogue when nobody is joining it (the decision is taken under the mutex on both sides, so there is no double join and no join of a detached thread). - The implicit `hid_init()` from register schedules the global `hid_mgr` on the *registering* thread's run loop, which `hid_enumerate()`/`hid_open()` on another thread will not service. Nothing depends on it (the manager answers `IOHIDManagerCopyDevices()` synchronously), and changing `init_hid_manager()`'s run-loop scheduling would alter the non-hotplug enumeration path, so the behavior is documented at the call site instead: applications wanting the classic behavior should call `hid_init()` explicitly, from the thread they use HIDAPI on, before registering. - Nits: register clears the stale global error on success; the thread-failed-to-start unwind is now symmetric with the snapshot-failure one (it frees a partially built cache); `hid_internal_copy_device_info()` documents that it must be updated when `struct hid_device_info` gains a field, and the `hid_device_info` / `hid_device_info_ex` allocation asymmetry (a copy must never be fed back to `match_ref_to_info()`); the manager is closed on the new post-`IOHIDManagerOpen` failure path. ### Verification Manual only for the concurrency work — macOS has no hotplug hardware coverage in CI. Full re-read of the file; a written global lock-order statement audited against every acquisition; brace/paren balance and lock/unlock + CFRetain/CFRelease pairing checked on every path including all failure unwinds; explicit race walkthroughs for `hid_exit()` vs a callback that registers, removal during startup, a slow initial burst, `hid_exit()` racing deregister, last-callback self-deregistration, and concurrent joiners. C and C++ portability builds stay clean (no designated initializers, no `int`в†’`enum`). --- ### Final state (after review) Built on the #794 self-join fix and restructured further in review: an interim bootstrap mutex (which introduced an ABBA deadlock against register-from-callback) was removed in favor of `pthread_once` + an in-mutex `exiting` flag; the device cache is built synchronously from IOHIDManagerCopyDevices when the event thread starts (before it goes live), and every registration's ENUMERATE snapshot is a deep copy of that live cache taken under the hotplug mutex; device identity is `io_service_t` with a `MACH_PORT_NULL` guard (devices that report no backing io_service_t are excluded from hotplug (cache and events) because they cannot be matched for dedupe or removal; hid_enumerate() still lists them with an empty path); and internal-event-thread writes to the global error string are suppressed via the captured event-thread id. **Verification.** Every fix commit on this branch was adversarially delta-verified by an independent reviewer (Codex on a separate plan, or a fresh model) tasked to find what the fix itself broke — a loop that caught a defect in nearly every fix commit. A per-backend white-box harness injects fake device events into the real implementation and runs under ASan/UBSan/LSan and TSan (TSan proven live). The `hotplug-integration` branch — all four final backends merged with the test suite (#826) — is CI-green on every platform, with HotplugAPI_darwin executing per push (the device-backed Hotplug_darwin still self-skips). Implements `device->next == NULL` (#791, #792) and the ENUMERATE return-value (#793) contract for darwin. Part of hotplug support (#238); contract documented in #790. Assisted-by: claude-code:claude-opus-4-8

Youw commented

2026-08-04 11:58:18 -05:00

(Migrated from github.com)

Full implementation review (fresh pass, head `cc60780`)

Attempted to refute ~45 candidate issues across the register/deregister/exit/event-thread state machine, the CFRunLoop source signaling, the startup-barrier protocol, the exactly-once ENUMERATE boundary, retain/release balance, and the global-error suppression — almost all correctly handled, usually with an explicit comment explaining exactly the interleaving probed. The #794 self-join fix is complete (event-thread deregistration self-detaches in the epilogue; pthread_join only ever runs off-thread with the mutex released). No blockers, no majors; 1 minor and 1 nit survive, both narrow edge cases.

[minor] `mac/hid.c:1460-1466` — live-arrival OOM silently violates exactly-once

In hid_internal_hotplug_connect_callback, if create_device_info(device) returns NULL (allocation failure), the function returns after unlocking: the device is neither added to the cache nor dispatched, and because IOHIDManager never re-reports it, it is reported neither by any registration's ENUMERATE pass nor as a live event; its later disconnect also matches nothing, so no LEFT fires. This contradicts the "exactly once — never both and never neither" contract. The registration-time paths were deliberately hardened for exactly this (1666-1674 fails startup, 2102-2124 fails the registration rather than deliver a partial pass), which makes the remaining live-path hole a conscious asymmetry — but it is undocumented.

Suggested fix: there is no failure channel in a live IOKit callback, so full recovery is impractical; at minimum document at the early-return that the exactly-once guarantee is best-effort under allocation failure (matching the care taken on the two registration paths), optionally with a single retry before dropping.

[nit] `mac/hid.c:1401-1405` — MACH_PORT_NULL-service devices can be double-reported

match_ref_to_info returns 0 when IOHIDDeviceGetService() is MACH_PORT_NULL, so such a device is never "known": the startup drain (1820) and the synchronous IOHIDManagerCopyDevices merge (1662) can each add an entry for it, and the post-barrier re-report burst dispatches it again as a live arrival (1453 dedupe fails) — "both" instead of "exactly once" — while its disconnect evicts nothing. Practically unreachable (IOHIDManager devices are always created from a backing io_service_t), and the guard is the correct trade against the identity-aliasing bug it fixed. If desired: skip caching entries whose service is MACH_PORT_NULL (they can never be matched or evicted anyway) so a pathological device is consistently invisible rather than inconsistently duplicated.

Verified sound (checked, no issues): exactly-once boundary (synchronous snapshot at thread startup, is_known_device dedupe of the re-report burst, stop_after freeze + cache-truncation for registrations made inside callbacks, per-usage-pair entries); replay always flushed before any live event (ARRIVED-before-LEFT holds even for a disconnect racing a pending pass); no lost wakeups (version-0 sources latch signals; every thread_state = 2 writer signals under the mutex); join/teardown protocol race-free (join_in_progress/join_done, epilogue self-detach vs collector join mutually exclusive, startup barrier destroyed only after both participants left, hotplug mutex never destroyed, re-init after hid_exit works, handles never reused); contract conformance (deregister blocks on in-flight invocations, stale handles → -1, *callback_handle written before any delivery, device->next NULL on every payload path, LEFT payloads are cached arrival-time copies, non-zero return aborts the rest of the pass and frees the remaining replay); global-error invariant (only writers suppressed via event_thread_id under global_error_mutex; the event thread never writes it, including via implicit hid_init); CF/IOKit retain/release balance on all paths.

Drafted with Claude Code.

## Full implementation review (fresh pass, head `cc60780`) Attempted to refute ~45 candidate issues across the register/deregister/exit/event-thread state machine, the CFRunLoop source signaling, the startup-barrier protocol, the exactly-once ENUMERATE boundary, retain/release balance, and the global-error suppression — almost all correctly handled, usually with an explicit comment explaining exactly the interleaving probed. The #794 self-join fix is complete (event-thread deregistration self-detaches in the epilogue; `pthread_join` only ever runs off-thread with the mutex released). **No blockers, no majors; 1 minor and 1 nit survive, both narrow edge cases.** ### [minor] `mac/hid.c:1460-1466` — live-arrival OOM silently violates exactly-once In `hid_internal_hotplug_connect_callback`, if `create_device_info(device)` returns NULL (allocation failure), the function returns after unlocking: the device is neither added to the cache nor dispatched, and because IOHIDManager never re-reports it, it is reported *neither* by any registration's ENUMERATE pass nor as a live event; its later disconnect also matches nothing, so no LEFT fires. This contradicts the "exactly once — never both and never neither" contract. The registration-time paths were deliberately hardened for exactly this (1666-1674 fails startup, 2102-2124 fails the registration rather than deliver a partial pass), which makes the remaining live-path hole a conscious asymmetry — but it is undocumented. **Suggested fix:** there is no failure channel in a live IOKit callback, so full recovery is impractical; at minimum document at the early-return that the exactly-once guarantee is best-effort under allocation failure (matching the care taken on the two registration paths), optionally with a single retry before dropping. ### [nit] `mac/hid.c:1401-1405` — MACH_PORT_NULL-service devices can be double-reported `match_ref_to_info` returns 0 when `IOHIDDeviceGetService()` is `MACH_PORT_NULL`, so such a device is never "known": the startup drain (1820) and the synchronous `IOHIDManagerCopyDevices` merge (1662) can each add an entry for it, and the post-barrier re-report burst dispatches it again as a live arrival (1453 dedupe fails) — "both" instead of "exactly once" — while its disconnect evicts nothing. Practically unreachable (IOHIDManager devices are always created from a backing `io_service_t`), and the guard is the correct trade against the identity-aliasing bug it fixed. If desired: skip caching entries whose service is MACH_PORT_NULL (they can never be matched or evicted anyway) so a pathological device is consistently invisible rather than inconsistently duplicated. --- Verified sound (checked, no issues): exactly-once boundary (synchronous snapshot at thread startup, `is_known_device` dedupe of the re-report burst, `stop_after` freeze + cache-truncation for registrations made inside callbacks, per-usage-pair entries); replay always flushed before any live event (ARRIVED-before-LEFT holds even for a disconnect racing a pending pass); no lost wakeups (version-0 sources latch signals; every `thread_state = 2` writer signals under the mutex); join/teardown protocol race-free (`join_in_progress`/`join_done`, epilogue self-detach vs collector join mutually exclusive, startup barrier destroyed only after both participants left, hotplug mutex never destroyed, re-init after `hid_exit` works, handles never reused); contract conformance (deregister blocks on in-flight invocations, stale handles → -1, `*callback_handle` written before any delivery, `device->next` NULL on every payload path, LEFT payloads are cached arrival-time copies, non-zero return aborts the rest of the pass and frees the remaining replay); global-error invariant (only writers suppressed via `event_thread_id` under `global_error_mutex`; the event thread never writes it, including via implicit `hid_init`); CF/IOKit retain/release balance on all paths. _Drafted with Claude Code._

Youw commented

2026-08-04 12:05:31 -05:00

(Migrated from github.com)

Re-review of fix commit `72169eb`

A fresh delta review (independent reviewer) verified the fix against both findings and hunted for newly introduced defects:

Live-arrival OOM documentation — FIXED. The comment at the if (!info) early-return is accurate and correctly placed; both contrast claims were verified (hid_internal_hotplug_build_device_cache OOM fails startup loudly via startup_ok, and registration-snapshot OOM detaches the callback and fails the registration). The parenthetical about the startup phase is also correct: the drain precedes the cache build, so an un-cached device is retried there and fails loudly on repeat OOM.
MACH_PORT_NULL guards — FIXED and complete. The only two insertion points into the hotplug device cache are the connect callback and the startup cache build; both are now guarded. Placement before the mutex is safe (only side-effect-free statements precede it; IOKit matching callbacks pass the device at +0, so the early return leaks nothing), the !device half hardens a pre-existing create_device_info(NULL) path, and the startup drain inserts via the guarded callback — no unguarded bypass exists. The disconnect path needs no change (match_ref_to_info already returns 0 for MACH_PORT_NULL).
New defects: none found. macOS CI (macos-cmake, macos-automake) compiles the fix clean.

PR #824 is review-clean from my side.

Drafted with Claude Code.

## Re-review of fix commit `72169eb` A fresh delta review (independent reviewer) verified the fix against both findings and hunted for newly introduced defects: - **Live-arrival OOM documentation — FIXED.** The comment at the `if (!info)` early-return is accurate and correctly placed; both contrast claims were verified (`hid_internal_hotplug_build_device_cache` OOM fails startup loudly via `startup_ok`, and registration-snapshot OOM detaches the callback and fails the registration). The parenthetical about the startup phase is also correct: the drain precedes the cache build, so an un-cached device is retried there and fails loudly on repeat OOM. - **MACH_PORT_NULL guards — FIXED and complete.** The only two insertion points into the hotplug device cache are the connect callback and the startup cache build; both are now guarded. Placement before the mutex is safe (only side-effect-free statements precede it; IOKit matching callbacks pass the device at +0, so the early return leaks nothing), the `!device` half hardens a pre-existing `create_device_info(NULL)` path, and the startup drain inserts via the guarded callback — no unguarded bypass exists. The disconnect path needs no change (`match_ref_to_info` already returns 0 for MACH_PORT_NULL). - **New defects: none found.** macOS CI (`macos-cmake`, `macos-automake`) compiles the fix clean. PR #824 is review-clean from my side. _Drafted with Claude Code._

Youw commented

2026-09-08 15:49:57 -05:00

(Migrated from github.com)

Fresh review pass (2026-09-08)

Two independent reviews (Claude Fable 5.1 and gpt-6-astra), merged and deduplicated: 31 findings (1 major, 5 minor, 14 nit, 11 note).

Fixes landed in four commits, each validated by the new HotplugLifecycle_darwin test on the macOS CI jobs:

5894384 - IOHIDManagerOpen removed from startup, strict usage-chain construction, join ownership retained, the lifecycle test added. Its first CI run hung: dyld backs a dylib __thread marker with a pthread key whose destructor frees it before application TSD destructors run, so a destructor re-entering registration joined its own thread.
1c84663 - identity resolved under the hotplug mutex instead; this introduced an ABBA deadlock against an application that serializes its HIDAPI calls with its own lock (found by the delta review).
a35f00a - the event thread publishes its OS thread id atomically at startup; identity is read lock-free (TLS marker or published id), and the global-error path never touches the hotplug mutex. Regression added for the owner-mutex scenario.
685a850 - refuse to start if pthread_threadid_np() fails (a zero id would silently disable the destructor-phase fallback) and document the non-recoverable state after a failed join.

Final adversarial verification (Fable, on a35f00a): 13 fixed, 3 skipped as contract-forbidden triggers (hid_exit() from a callback, concurrent init/exit), 1 not fixed - mac-26, the later documentation commits on connection-callback cited by some comments are not in this PR's tree, which resolves on merge; 13 wording items were not re-checked by design. The note and nit it raised are round 4 above.

CI: macOS jobs green on 685a850, HotplugLifecycle_darwin passes.

Drafted with Claude Code.

## Fresh review pass (2026-09-08) Two independent reviews (Claude Fable 5.1 and gpt-6-astra), merged and deduplicated: **31 findings** (1 major, 5 minor, 14 nit, 11 note). Fixes landed in four commits, each validated by the new `HotplugLifecycle_darwin` test on the macOS CI jobs: - 5894384 - `IOHIDManagerOpen` removed from startup, strict usage-chain construction, join ownership retained, the lifecycle test added. Its first CI run hung: dyld backs a dylib `__thread` marker with a pthread key whose destructor frees it before application TSD destructors run, so a destructor re-entering registration joined its own thread. - 1c84663 - identity resolved under the hotplug mutex instead; this introduced an ABBA deadlock against an application that serializes its HIDAPI calls with its own lock (found by the delta review). - a35f00a - the event thread publishes its OS thread id atomically at startup; identity is read lock-free (TLS marker or published id), and the global-error path never touches the hotplug mutex. Regression added for the owner-mutex scenario. - 685a850 - refuse to start if `pthread_threadid_np()` fails (a zero id would silently disable the destructor-phase fallback) and document the non-recoverable state after a failed join. Final adversarial verification (Fable, on a35f00a): 13 fixed, 3 skipped as contract-forbidden triggers (`hid_exit()` from a callback, concurrent init/exit), 1 not fixed - mac-26, the later documentation commits on `connection-callback` cited by some comments are not in this PR's tree, which resolves on merge; 13 wording items were not re-checked by design. The note and nit it raised are round 4 above. CI: macOS jobs green on 685a850, `HotplugLifecycle_darwin` passes. _Drafted with Claude Code._

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin 'hotplug-async-enumerate-mac:hotplug-async-enumerate-mac'

git switch 'hotplug-async-enumerate-mac'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'connection-callback'

git merge --no-ff 'hotplug-async-enumerate-mac'

git switch 'hotplug-async-enumerate-mac'

git rebase 'connection-callback'

git switch 'connection-callback'

git merge --ff-only 'hotplug-async-enumerate-mac'

git switch 'hotplug-async-enumerate-mac'

git rebase 'connection-callback'

git switch 'connection-callback'

git merge --no-ff 'hotplug-async-enumerate-mac'

git switch 'connection-callback'

git merge --squash 'hotplug-async-enumerate-mac'