mac: deliver hotplug ENUMERATE pass asynchronously on the event context#824
Open
Youw wants to merge 5 commits into
Open
mac: deliver hotplug ENUMERATE pass asynchronously on the event context#824Youw wants to merge 5 commits into
Youw wants to merge 5 commits into
Conversation
…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
…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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the hotplug contract documented in
hidapi/hidapi.h(#790) for the mac backend, and fixes the self-join hang of #794.What changed
HID_API_HOTPLUG_ENUMERATEpass.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->nextalwaysNULL), exactly once, and never from within the register call itself.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 withmutex_in_useset, honoring a non-zero callback return (deregisters the callback and drops the rest of its pass).CFRunLoopPerformBlockwas avoided on purpose: it relies on the blocks extension, which is a portability risk for the plain-C/C++ builds.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 withENUMERATEfor both event types therefore never sees aDEVICE_LEFTfor a device whose arrival it has not been told about first.hid_internal_hotplug_cleanup()is reached on the event thread itself and used topthread_join()its own thread - a permanent hang. Cleanup now detects this withpthread_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 byhid_internal_hotplug_join_thread()at the next registration or athid_exit(), guarded by athread_needs_joinflag. The flag also makes cleanup idempotent, fixing a pre-existing double-join/stale-wakeup whenhid_exit()ran after the last callback had already been deregistered.hid_init(). Register now initializes the library implicitly, per contract. The hotplug IOHIDManager remains fully independent ofhid_mgr.hid_exit()leak. Hotplug teardown was guarded byif (hid_mgr), so hotplug-only usage never stopped the event thread or freed the callback list. The teardown now runs unconditionally. Therun_loop_modeCFString (previously leaked) is released there as well.*callback_handle(when non-NULL) is set to 0; on success it is written before any event can be delivered.callback_handle <= 0early rejection (parity with the other backends); an unknown/stale handle stays a safe no-op returning -1. Deregister andhid_exit()free undelivered replay entries, so a deregistered callback never fires again, including its pending synthetic events.device->next == NULLfor every invocation. The IOHID connect callback previously passed chainedhid_device_infoentries (multi-usage devices) straight into the callbacks; each entry is now detached for the duration of its delivery.thread_state != 1), unwound cleanly, and turned into a -1 with an error string.pthread_createfailure is likewise handled.dup_wcs()got a missing malloc NULL-check so snapshot OOM fails cleanly instead of crashing.Design decisions / deviations
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.ENUMERATEcallback 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 thatFixed 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.DEVICE_LEFTwithout a prior arrival.hid_init()schedules the globalhid_mgron 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:pthread_joinran with the hotplug mutex held while the event thread blocked on that mutex inside the replay perform. The join moved out ofhid_internal_hotplug_cleanup()into a dedicated collector invoked from the public entry points with the mutex released, serializing concurrent joiners.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.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 makehid_error(NULL)— which reads the pointer unlocked — safe against another thread. Both are superseded below; the bootstrap mutex is gone (pthread_once+ an in-mutexexitingguard) and the hotplug mutex is now never destroyed.DEVICE_LEFTwithout a prior arrival).IOHIDManagerOpenresult checked (registration fails instead of returning an inert handle);pthread_barrier_initchecked 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_usesave/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 ahid_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 usespthread_once(), and thehid_exit()teardown is guarded from inside the hotplug mutex by anexitingflag. 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 > 0guard in the disconnect callback wrapped only its explicit lock/unlock — thehid_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), andhid_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 withoutHID_API_HOTPLUG_ENUMERATE. The snapshot boundary is now a real one: the cache is completed synchronously withIOHIDManagerCopyDevices()(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 byio_service_tagainst 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 honorskCFRunLoopRunStopped.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 readmutex_readyunsynchronized, then locked).hid_exit()could thereforepthread_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_readyis never reset), and an explicitexitingstate, 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).exitingstays set across the whole ofhid_exit(), so the implicithid_init()in register — now performed under the hotplug mutex — cannot race the destruction ofhid_mgreither.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 madehid_error(NULL)safe against an internal thread. Audited: no HIDAPI-internal event context inmac/hid.cwrites 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 thathid_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 inpthread_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 clearingjoin_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_statewas 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 writesthread_stateat all before the barrier; it reports its result instartup_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 everypthread_mutexattr_init/settype/mutex_init/cond_initcall, 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.ENUMERATEpass nor any live event.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).hid_init()from register schedules the globalhid_mgron the registering thread's run loop, whichhid_enumerate()/hid_open()on another thread will not service. Nothing depends on it (the manager answersIOHIDManagerCopyDevices()synchronously), and changinginit_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 callhid_init()explicitly, from the thread they use HIDAPI on, before registering.hid_internal_copy_device_info()documents that it must be updated whenstruct hid_device_infogains a field, and thehid_device_info/hid_device_info_exallocation asymmetry (a copy must never be fed back tomatch_ref_to_info()); the manager is closed on the new post-IOHIDManagerOpenfailure 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, noint→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-mutexexitingflag; the registration snapshot is built synchronously fromIOHIDManagerCopyDevicesbefore the run-loop thread goes live; device identity isio_service_twith aMACH_PORT_NULLguard; 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-integrationbranch — all four final backends merged with the test suite (#826) — is CI-green on every platform, with the hotplug contract tests executing against this backend, not skipped.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