Skip to content

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

Open
Youw wants to merge 5 commits into
connection-callbackfrom
hotplug-async-enumerate-mac
Open

mac: deliver hotplug ENUMERATE pass asynchronously on the event context#824
Youw wants to merge 5 commits into
connection-callbackfrom
hotplug-async-enumerate-mac

Conversation

@Youw

@Youw Youw commented Jul 13, 2026

Copy link
Copy Markdown
Member

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 (Hotplug (macOS): self-join deadlock in hid_internal_hotplug_cleanup #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 (plus the barrier destroy and the release of both run loop sources, which the winding-down thread may still need) is performed by hid_internal_hotplug_join_thread() at the next registration or at hid_exit(), guarded by a thread_needs_join flag. The flag also makes cleanup idempotent, fixing a pre-existing double-join/stale-wakeup when hid_exit() ran after the last callback had already been deregistered.
  • 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.
  • 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 to register through the existing barrier handshake (thread_state != 1), 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

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 intenum).

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

Youw added 3 commits July 14, 2026 01:39
…hread

Implement the hotplug contract documented in hidapi.h for the darwin
backend: the HID_API_HOTPLUG_ENUMERATE initial pass is now a
registration-time snapshot replayed on the hotplug event thread via a
second run loop source, always before any live events for that callback.

Also fix the event thread joining itself when a callback deregisters the
last callback from within a device-removal event (the join is deferred to
the next registration or hid_exit), make hid_exit() tear down the hotplug
state without an explicit hid_init(), initialize the library implicitly on
registration, reject invalid handles in deregister, keep device->next NULL
for every callback invocation, and set the global error string on all
register/deregister failure paths.

Fixes #794

Assisted-by: claude-code:claude-fable-5
Never join the event thread with the hotplug mutex held: the join moved
out of hid_internal_hotplug_cleanup() into a dedicated collector that runs
from the public entry points with the mutex released, serializing
concurrent joiners (fixes the register+deregister deadlock with a pending
ENUMERATE replay).

Serialize the one-time setup (implicit hid_init and hotplug mutex
creation) and the hid_exit teardown with a static bootstrap mutex, and
serialize all mutations of the global error string with a static mutex.

Drain the initial device-matching burst in the private run loop mode the
manager is actually scheduled on, before releasing the startup barrier -
so the device cache is populated for the first registrant's snapshot and
pre-connected devices never surface as live arrival events.

Freeze each dispatch at the tail callback registered at dispatch start,
and append arriving cache entries before invoking callbacks: a callback
registered from within a callback never receives the in-flight event and
its snapshot covers arrivals exactly once.

Also: check IOHIDManagerOpen result and fail the registration; check
pthread_barrier_init and fix the shim's error checks (pthread errors are
positive); reject deregistering an already-deregistered handle; fail
registration on callback handle exhaustion instead of wrapping; publish
the unsolicited run-loop exit under the mutex; use save/restore
discipline for mutex_in_use consistently.

Assisted-by: claude-code:claude-fable-5
The round-1 bootstrap mutex was ordered outside the hotplug mutex, which
deadlocks against a registration made from inside a callback: replace it
with pthread_once() and guard the hid_exit() teardown from inside the
hotplug mutex with an `exiting` flag. The hotplug mutex is never destroyed
any more, so no thread can lock it while hid_exit() frees it.

Make the initial HID_API_HOTPLUG_ENUMERATE snapshot a real completion
boundary (a synchronous IOHIDManagerCopyDevices(), with the matching burst
deduplicated by io_service_t) instead of a 1 ms run loop pump, hoist the
removal dispatch under the startup guard (a removal during the startup
window could lock the mutex held by the registrant parked at the barrier),
replace the join spin with a condition variable, and keep all of the event
thread's lifecycle state under the mutex.

Assisted-by: claude-code:claude-opus-4-8
Youw added 2 commits July 15, 2026 02:57
…hing

Two devices that both lack an IOService (service == MACH_PORT_NULL) compared
equal, so the arrival dedupe would suppress the second one and a removal could
evict the wrong cache entry. Require a non-null service before matching.

Assisted-by: claude-code:claude-opus-4-8
Honor the cross-backend contract (documented in hidapi.h and already
enforced by the libusb and linux backends) that HIDAPI calls made from
within a hotplug callback do not update the global error string: the
callback runs on HIDAPI's internal event thread, so such a write races an
application's hid_error(NULL) read - a use-after-free of
last_global_error_str, the same class as the original hotplug blocker.

The event thread now publishes its pthread id (guarded by the leaf
global_error_mutex) as its first action and clears it in its epilogue.
register_global_error()[_format]() skip the write when invoked on that
thread, covering both the failure paths and the success-path clear of a
callback that re-enters hid_hotplug_(de)register_callback(). Writes from
application threads are unaffected, and per-device errors are never
suppressed.

Assisted-by: claude-code:claude-opus-4-8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant