[CI/validation] Run hotplug tests against all four backend implementations (do not merge)#830
Open
Youw wants to merge 45 commits into
Open
[CI/validation] Run hotplug tests against all four backend implementations (do not merge)#830Youw wants to merge 45 commits into
Youw wants to merge 45 commits into
Conversation
Implements the hotplug contract documented in hidapi.h for the hidraw backend: - hid_hotplug_register_callback() now takes a deep-copied snapshot of the matching connected devices and the udev monitor thread replays it as synthetic "arrived" events: never on an application thread, never from within the register call itself, and always before any live events for that callback; a non-zero return stops the remainder of the pass and deregisters the callback. - Live "arrived" events are delivered as one invocation per device entry with next == NULL (multi-usage devices previously exposed the chained list to callbacks). - All register/deregister failure paths set the global error string and register resets *callback_handle to 0 on failure. - The monitor thread only takes the mutex with trylock and releases no shared state on exit; joining and monitoring-context teardown are centralized in hid_internal_hotplug_cleanup(), fixing an unjoined thread and a teardown race when the last callback removes itself on the monitor thread and a later registration re-creates the context. Assisted-by: claude-code:claude-fable-5
…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
…nt context Implement the hotplug contract documented in #790 for the windows backend: The HID_API_HOTPLUG_ENUMERATE initial pass is now a deep-copied registration-time snapshot of the matching connected devices, replayed on a threadpool work item (the same critical-section-serialized event context that delivers live events) instead of running synchronously inside hid_hotplug_register_callback on the application thread. A callback's pending snapshot is flushed before any live event is delivered to it, so each device connection is reported exactly once and the pass always precedes live events; a non-zero callback return stops the remainder of the pass and deregisters the callback. Also per the contract: - hid_hotplug_deregister_callback returns -1 for unknown or stale handles (0 only when the callback was found and deregistered) - every register/deregister failure path sets the global error string, and *callback_handle is zeroed on registration failure - undelivered snapshots are freed on deregistration and hid_exit - CM_Unregister_Notification is no longer called from within the notification callback (forbidden, deadlocks) nor while holding the hotplug critical section (deadlocks against in-flight notifications): the teardown is detached under the lock and performed outside it, deferred to the work item when triggered from the notification itself Addresses #793 for the windows backend. Assisted-by: claude-code:claude-fable-5
…t thread Implement the hotplug contract documented in hidapi.h for the libusb backend: - The initial HID_API_HOTPLUG_ENUMERATE pass no longer runs synchronously inside hid_hotplug_register_callback(): registration takes a deep-copied snapshot of the matching connected devices and replays it on the callback (event) thread as synthetic arrival events, woken up by a queued marker message. The snapshot is always delivered before any live event for that callback (exactly-once per device connection), and a non-zero return stops the rest of the pass and deregisters the callback. - device->next is now NULL for every callback invocation, including multi-interface devices. - Every failure path of register/deregister sets the global error string, and *callback_handle is zeroed on any registration failure. - The event threads are wound down via a dedicated shutdown flag; the join is deferred to the next registration or hid_exit() so that removing the last callback from within a callback cannot deadlock, and the message queue (devices and markers) is drained before the hotplug libusb context is destroyed. - Concurrent registrations no longer race in hid_init() or the machinery initialization. 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
…TE pass
- Arm the CM notification BEFORE taking the registration-time device
snapshot, and deduplicate arrivals by path in the notification callback:
a device connecting between the two is now caught by the notification
and absorbed by the dedupe instead of being missed by both (it was
reported by neither before)
- Only (re)build the device cache when actually arming a notification;
while one is attached the cache is kept current by its callbacks
(re-enumerating during a deferred teardown could double-report)
- Serialize new-notification arming against in-flight
CM_Unregister_Notification calls (pending counter + condition variable):
a detached handle stays live at the OS until its unregistration
completes, and overlapping registrations would deliver events twice;
hid_exit waits for the same quiescence before destroying state
- Treat a failed CM_Unregister_Notification as a poisoned epoch: hid_exit
reports the failure, returns -1 and deliberately leaks the critical
section, work item and loaded DLLs rather than let a still-live
notification touch destroyed state
- Guard the machinery bootstrap with a statically-initialized SRWLOCK
(two racing first registrations could both initialize the critical
section), and serialize all mutations of the global error string with
another SRWLOCK (concurrent thread-safe failures could double-free it)
- Distinguish a genuinely failed enumeration from an empty system at
registration (fail the registration on the former), and clear the stale
"No HID devices found" error on successful registration
- Fail registration with an error when the callback handle space is
exhausted instead of signed-overflowing and wrapping onto live handles
- Replace { 0 } struct initializers with memset in hid.c for
-Wmissing-field-initializers cleanliness when built as C++ with GNU
compilers; use the WinAPI error helper for CreateThreadpoolWork failures
Addresses round-1 review of the windows part of #793.
Assisted-by: claude-code:claude-fable-5
Follow-up to the asynchronous ENUMERATE pass, fixing the issues raised in the first review round: - Serialize every mutation of the error strings with a dedicated mutex: registration/deregistration and the monitor thread could write (and double-free) the global error string concurrently. - Guard the first-time initialization of the hotplug machinery with a static bootstrap mutex: two concurrent first registrations could both initialize the mutex and reset the context fields. - Serialize hid_init(): a registration implicitly initializes the library both directly and through its initial enumeration, and setlocale() is not thread-safe against itself. - Never read the monitor loop's decision state unlocked, and never join the monitor thread while holding the mutex: the thread announces its exit under the mutex, and the cleanup unlocks before joining. - Skip devices that are already known on arrival: a device showing up between arming the udev monitor and taking the initial enumeration was reported (and cached) twice, and left twice. - Update the device cache before dispatching an event, and bound every dispatch to the callbacks registered when the event started, so that a callback registered from within a callback observes the device through its own snapshot exactly once, instead of missing it or seeing a "left" with no matching "arrived". - Make the initial snapshot all-or-nothing: a failed copy now unwinds the registration instead of delivering a partial device list, and a genuine enumeration failure fails the registration (an empty system does not). - Check every libudev setup call and fail the registration with an error string; treat only a negative monitor file descriptor as invalid (0 is a valid one). - Fail the registration when the callback handles are exhausted instead of overflowing (undefined behaviour) and wrapping onto live handles. - Report deregistration of an already deregistered handle as not found. - Reap a self-exited monitor thread before deregistration returns early. - Handle a NULL action/devnode from udev, and use poll() instead of select() for the monitor file descriptor. Assisted-by: claude-code:claude-opus-4-8
… review Address the first round of review findings on the libusb hotplug backend: - Serialize every mutation of the global error state: two concurrent (documented thread-safe) register/deregister calls could double-free the error string. - Deliver an arriving device exactly once, also when a callback registers another callback while the arrival is being dispatched: the arrived infos are appended to the device cache before any callback runs, and the dispatch boundary is computed once per message instead of per interface. - Arm the libusb listener before taking the initial device snapshot and deduplicate arrivals against the cache: a device connecting in between was in neither the snapshot nor any live event, and its removal went unreported as well. - Tell a genuine hid_enumerate() failure from an empty system: registration used to succeed with an empty cache, making every already-connected device invisible forever. - Make the ENUMERATE snapshot and its replay marker all-or-nothing, and fail the registration if an event thread cannot be started (the thread handles were joined unchecked, and the callback thread is now started by the registration so a failure can be reported at all). - Wind the event threads down synchronously when the last callback is deregistered from an application thread, so that no callback can be running once it returns; the deferral is only needed for a self-deregistration from the event thread, which is now awaited on a condition variable rather than a busy `unlock`/`lock` spin. - Deregistering an already-removed callback fails again instead of silently succeeding, and handle allocation refuses to overflow rather than wrapping into live handles. - Publish the hotplug mutex through the init mutex (both reads and writes) and never destroy it, so hid_exit() cannot pull it from under a concurrent register/deregister. Assisted-by: claude-code:claude-opus-4-8
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
…TE pass Critical section lifetime. hid_exit() used to delete the critical section (and reset mutex_ready) with nothing holding it, while a registration sleeping on a pending unregistration, a concurrent deregistration or a late notification could still be about to enter it. The critical section and the quiescence event are now created once and kept for the process lifetime, and an explicit `exiting` state, set under the lock, makes registration and deregistration fail instead of arming a notification behind a teardown. `exiting` is only cleared once hid_exit() has unloaded the libraries, which also closes the window in which a thread-safe (and implicitly initializing) hid_hotplug_register_callback() could call into hid.dll/cfgmgr32.dll while they were being unloaded. Global error string. The internal event context no longer writes it: hid_error(NULL) hands the raw pointer to the application without a lock, so a threadpool work item freeing it underneath an application thread is a use-after-free the application cannot serialize away. The unregistration failure is recorded in internal state and reported by the next hid_exit() instead. The writer-side lock is kept. Failed unregistration. Scoped to the teardown that reports it, so hid_exit() no longer fails forever after one failure. A leaked notification is still live at the OS level, so no second notification is ever armed next to it (both would deliver every event) - hotplug registration fails with an explicit error instead - the libraries it calls into stay loaded, and the module that contains the callback is pinned so that it cannot be unmapped underneath the OS. Arrival dedupe. It only ever existed to absorb the overlap between arming the notification and taking the registration-time snapshot, but it suppressed every arrival whose path was cached - and an interface path is reused when a device is replugged into the same port. It is now scoped to that window: the enumerated paths are recorded, each swallows at most one arrival, and a path is dropped as soon as its device leaves. Outside the window an arrival for a cached path is a new connection, and it replaces the stale record rather than being shadowed by it. Enumeration and cache consistency. A per-device allocation failure is reported through the failure channel instead of passing for a successful (but silently short) enumeration; a device_info is never handed out half-built, so the cache can no longer hold a record with a NULL path - which the removal walk dereferenced - and the bus-specific fixups can no longer read a NULL string. Threadpool. Resolved dynamically from kernel32 (and hotplug registration fails cleanly when it is unavailable) instead of forcing _WIN32_WINNT to 0x0600, which turned the threadpool, SRWLOCK and condition-variable calls into load-time Vista-only imports for every user of the library, including those that never touch hotplug. Assisted-by: claude-code:claude-opus-4-8
Two new test tiers for the hotplug API: - test_hotplug_api.c (HotplugAPI_<backend>): argument validation, callback-handle properties, implicit init, hid_exit() teardown and register/deregister thread churn. Needs no device or privileges, so it runs against every backend in the ordinary CI matrix. - test_hotplug.c (Hotplug_<backend>): device-backed hotplug scenarios (async delivery, exactly-once ENUMERATE pass, callback-return deregistration, pass-before-live ordering, ARRIVED/LEFT payloads, filtering, dispatch order, deregistration post-condition and re-entrant registration) against a virtual device whose presence is toggled with the new test_virtual_device_unplug()/_replug() calls. Implemented for the uhid provider (UHID_DESTROY/UHID_CREATE2 on the same fd); the other providers return TEST_VDEV_UNAVAILABLE and their hotplug tests self-skip until presence toggling is implemented. Assisted-by: claude-code:claude-fable-5
…TE pass Machinery lifetime, take three. Round 2 made hid_exit() bootstrap the hotplug machinery just to raise the `exiting` flag, and never destroyed it - so every program, including ones that never touch hotplug, had hid_exit() create a kernel event and a critical section it could not free: one leaked handle per load-use-unload cycle in a plugin-style host. The flag does not need the critical section: `hotplug_exiting` now lives under the statically initialized bootstrap spinlock (which costs no OS object), so hid_exit() can raise it before knowing whether the machinery even exists and return without creating anything. On top of that, public hotplug calls are now counted into the machinery through the same lock (enter/leave), which gives hid_exit() the proof round 2 lacked: once `exiting` cuts off new entries, every notification is unregistered, the work item is drained and closed, and the user count has hit zero, nothing can be inside or blocked on the critical section - so the clean path destroys the event and the critical section it created instead of keeping them for the process lifetime. When a notification could not be unregistered, everything is still kept, exactly as in round 2: a live OS callback must never find a deleted critical section. 100 x (LoadLibrary, hid_init, hid_enumerate, hid_exit, FreeLibrary) now leaks 0 handles (was +1 per cycle), with or without hotplug use in the loop. Pending-arrivals dedupe, scoped in time as well. The registration-time records suppressed the one in-flight duplicate they were made for, but a record whose device never re-arrived outlived that window - so a device whose removal notification was missed had its genuine re-arrival swallowed by its own stale record, and the stale-record recovery could never run for exactly the devices it was written for. The list now expires (10 s, far beyond any notification delivery): an expired record no longer marks a duplicate, and the failure modes are asymmetric in the right direction - an over-aged duplicate produces a spurious left+arrived pair instead of a silently dropped connection. The owed DEVICE_LEFT. Replacing a stale cache record on a same-path arrival reported the new connection but silently discarded the previous one, although the header promises a "left" event for every matching device that disconnects while a callback is registered. The live dispatch loop is factored out and the stale record is now dispatched as a synthetic DEVICE_LEFT - before the new device is cached, so a callback registered from within sees the connection exactly once. Spinlock discipline. The global error string is now built and freed outside the spinlock - only the pointer swap is guarded - and the acquire loop escalates from Sleep(0) to Sleep(1) after a few attempts: Sleep(0) yields only to equal-or-higher priority threads, so spinning on a lower-priority holder on a single CPU made no progress until the balance-set manager intervened. Enumeration failure taxonomy. hid_internal_UTF16toUTF8() returns NULL both for invalid UTF-16 (e.g. an unpaired surrogate in an interface path) and on allocation failure, and round 2 escalated both to a full enumeration failure reported as out-of-memory - one malformed path lost every other device. The two are now told apart: an unrepresentable device is skipped (consistently with the hotplug notifications, which can neither report nor cache it), and only genuine allocation failures abort the pass. Also corrects the comment on the global error lock (a user callback calling the public API from the event context does write the global error; the header's serialization requirement covers it), removes a dead `failed` term that notification_leaked always implies, and documents why the registration wait loop may release the critical section unconditionally. Assisted-by: claude-code:claude-fable-5
…ites - Never destroy the hotplug mutex: hid_exit() could destroy it under a concurrent register/deregister. It is now created once (pthread_once) and lives for the process; teardown is gated by an `exiting` flag set inside the mutex, and hid_exit() loops its cleanup until the callback list is empty and the monitor thread is reaped. - Keep the global error string off HIDAPI's internal monitor thread (an application cannot serialize hid_error(NULL) against it): a quiet flag through create_device_info_for_device()/get_hid_report_descriptor*(), and a register/deregister nested in a callback no longer writes or clears it. - hid_internal_enumerate(): check udev_enumerate_add_match_subsystem() and udev_enumerate_scan_devices(), so a failed scan fails the registration instead of silently caching an empty device list. - Copy the whole arrival up front (dropping it entirely on OOM) instead of dispatching a cache entry with a truncated `next`. - Allocate the callback handle after the cleanup that may drop the mutex, and return it to the counter on every failure path. - The monitor thread now releases its context and detaches itself when its last callback deregistered itself from within a callback. - A dead udev monitor socket (POLLHUP/POLLNVAL, or a persistent POLLERR) tears the monitoring context down instead of dropping events forever. - Match a remove uevent without DEVNAME by syspath, so a stale cache entry cannot swallow the arrival of the next device on the same /dev/hidrawN. Assisted-by: claude-code:claude-opus-4-8
…honest Round-2 review fixes on top of the hotplug rework: - Never write the global error state from HIDAPI's own callback thread. The header allows registering and deregistering from within a callback, and both write it on their ordinary paths (every registration calls hid_init(), which resets it even on success), so hid_error(NULL) could double-free the string it was reading. hid_error() now also takes the writer mutex for the global context. - Keep the ':' when matching a libusb device against a cached HID device: without it "1-2" prefix-matches "1-20:0.0", so a device on port 2 was taken for one already known on port 20 and never reported at all. - Splice every interface of a departed device out of the cache BEFORE dispatching its removal: a callback registering from within that dispatch could otherwise snapshot the interfaces that were still queued and receive a synthetic arrival that no removal ever follows. - Tear the hotplug machinery down before destroying usb_context, and destroy it under the hotplug mutex: a callback re-entering hid_init() could resurrect a context that hid_exit() no longer frees, silently disabling the next hid_init(). - Keep a claimed-but-unfinished join visible (join_claimed) so nobody frees the device cache while the event threads still drain into it, check the read thread's creation in hid_open_path(), and drop the dead mutex_ready tests. Assisted-by: claude-code:claude-opus-4-8
The registration-time arrival dedupe was scoped by a 10-second wall-clock TTL.
An arrival whose pending-arrival marker had expired was reclassified as a new
connection on an already-cached path, which synthesized a DEVICE_LEFT for the
cached record before reporting a second DEVICE_ARRIVED.
Elapsed time is not a sound basis for that decision. A user callback is only
ADVISED to be short, so a perfectly legal slow callback that holds the critical
section for longer than the TTL let the still-queued arrival of a device that
was present at registration expire: one physical connection was then reported
as ARRIVED -> LEFT -> ARRIVED, breaking the documented exactly-once guarantee.
Dedupe against the live device cache instead, as the linux and mac backends do.
That needs no timing assumption anywhere:
- The OS delivers exactly one arrival notification per interface instance, so
the only way an arrival can be a duplicate is the arm-before-enumerate
window at registration - and there the enumeration has, by construction,
already put the device in the cache. An arrival for a path that is still
cached is therefore always a duplicate: skip the append and the dispatch.
- The removal notification is authoritative and drops the cache entry, so a
genuine re-plug - even onto the same, reused interface path - is not in the
cache and is reported.
The missed-removal recovery that the TTL existed to reach is removed along with
it, rather than re-gated on some other signal: a missed CM removal is not a real
scenario (removals arrive through the very notification that delivers arrivals),
and a spurious LEFT+ARRIVED pair for a device that never disconnected is a far
worse failure than the hypothetical duplicate it guarded against.
Assisted-by: claude-code:claude-opus-4-8
TSan (verified live with a racy probe) reported a data race on the read of hid_hotplug_context.monitor_fd in the monitor thread's prologue: it is unsynchronized against the setup of a later monitoring context. Read it under the mutex. Reap the monitor thread by detaching it at creation instead of joining it: when the last callback deregisters itself from within a callback, nothing is guaranteed to ever call into HIDAPI again to join the thread. The thread releases the monitoring context and announces its exit under the mutex, so nothing it owns or touches outlives hid_exit(); pthread_join(), the FINISHED state and the join bookkeeping all go away with it. (pthread_detach(pthread_self()) in the wind-down was the obvious alternative, but it makes ThreadSanitizer abort in its own interceptor - CHECK failed at tsan_rtl_thread.cpp:330, tid == kInvalidTid - which would break TSan runs for everyone.) Assisted-by: claude-code:claude-opus-4-8
Deduping arrivals against the live cache made that cache the single source of truth for what has been reported, and removed the stale-record recovery that used to paper over an inconsistent cache. That is sound only if the cache can never be left inconsistent - and one path could still leave it so. Both cache lookups took the interface path a CM notification carries (UTF-16) and converted it to UTF-8 before searching, which allocates. In the REMOVAL lookup that allocation is load-bearing: if it fails under memory pressure the removal is processed but its cache record survives. Because the cache is now the whole arrival dedupe, a surviving record is unrecoverable - interface paths are reused when a device is replugged into the same port, so the next connection on that path is classified as a duplicate and suppressed forever, for every callback, including later HID_API_HOTPLUG_ENUMERATE passes. The device becomes invisible. The removal notification did arrive; only its local processing failed, so the "removals come through the same notification stream" argument does not save it. Compare the cached UTF-8 path against the notification's UTF-16 symbolic link directly, encoding the UTF-16 to UTF-8 one code point at a time and matching it against the cached bytes (ASCII case-independent, exactly as the _stricmp() it replaces was). Both lookups - the arrival dedupe and the removal - are now allocation-free, so an out-of-memory condition can no longer desynchronize the cache from the system. The only allocation left on the notification path is the one that describes an arriving device, where failing it simply does not cache the device - and nothing was reported for it either, so the cache stays consistent. This is preferred over caching a second, wide copy of each path as a comparison key: it adds no per-device allocation to keep and free, no field to keep in sync with dev->path, and no change to the cached record type - the one remaining allocation is the one whose failure was always handled cleanly by dropping the arrival. Assisted-by: claude-code:claude-opus-4-8
Restore a joinable monitor-thread lifecycle (RUNNING -> FINISHED -> pthread_join -> NONE) so that once hid_exit() returns the thread is provably gone. The detached thread could publish its exit and let hid_exit() return while still running its own epilogue inside the library's text - a crash if the application then dlclose()s. The thread that self-terminates (its last callback deregistered from within a callback) leaves a joinable pthread_t reaped by the next register/deregister/hid_exit, mirroring the macOS deferred join. On a dead udev monitor socket, no longer synthesize DEVICE_LEFT for the cached (still-connected) devices. Instead rebuild the monitor a bounded number of times and, on success, re-enumerate and dispatch only the real deltas that occurred while the transport was down; if recreation ultimately fails, stop monitoring without any fabricated removal, leave the cache intact, and mark the machinery dead so new registrations are refused (the observable failure). The re-enumeration is quiet, so the monitor thread still never writes the global error string. Assisted-by: claude-code:claude-opus-4-8
…read-thread failure hid_exit() destroyed the main usb_context in a second, separately-locked step after the hotplug machinery was already torn down and the mutex released. A concurrent hid_hotplug_register_callback() could slip into that window, observe the still-non-NULL context, and spin up fresh hotplug threads that hid_exit() then left orphaned. Destroy the context while still holding the hotplug mutex, so the whole teardown is a single transaction. When the read thread failed to start, hidapi_initialize_device() only closed the handle and freed the device, leaving the kernel driver detached and the interface claimed (device unusable until replug). Release the interface and reattach the kernel driver, factoring the shared reattach into a helper so the failure paths cannot drift apart. Assisted-by: claude-code:claude-opus-4-8
hid_hotplug_register_callback() initialized usb_context (via hid_init()) BEFORE the loop that finishes any in-progress teardown. Both hid_internal_hotplug_finish_shutdown() and hid_internal_hotplug_wait_shutdown() release the hotplug mutex while a concurrent hid_exit() runs its single-transaction teardown, which NULLs usb_context before it hands the mutex back. A registration that parked in that loop therefore resumed with usb_context already destroyed and went on to hid_internal_enumerate() -> libusb_get_device_list(usb_context, ...) with usb_context == NULL, leaving the outcome to libusb's implicit-default-context handling instead of a cleanly serialized re-initialization. Move the hid_init() call to just after the wind-down loop rather than adding a second one: hid_init() is idempotent, so on the common path where no teardown intervened this is an unchanged no-op, while a register that slept across a teardown now re-establishes a consistent, non-NULL usb_context under the mutex before it enumerates. This keeps the single-transaction hid_exit() teardown intact and does not change the hotplug -> queue/error lock order. hid_hotplug_deregister_callback() has no symmetric hole: it never creates a context and never dereferences usb_context after its own wait. Assisted-by: claude-code:claude-opus-4-8
Follow-up review fixes, four of them in the recover/reconcile path: - Join the finished monitor thread with the mutex released and never from the thread itself, so a callback-armed TSD destructor can re-enter HIDAPI at join time without self-deadlock or self-join. - Treat a partial recovery re-enumeration as a failed attempt (a per-entry udev failure now flags it) instead of fabricating removals for present devices. - Reconcile on a stable per-connection identity, not the devnode path alone, so a device replaced on the same /dev/hidrawN yields LEFT(old) + ARRIVED(new). - Drop a reconcile arrival whose callback copy fails to allocate, so it is neither silently cached nor later reported as left. - Re-enumerate before the replacement monitor starts receiving, so a failed recovery does not discard already-queued events. 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
…cycle Remove the recover-and-reconcile machinery for a dead udev monitor socket (hid_internal_hotplug_recover_monitor / _reconcile / _same_connection / _find_same_connection and the wide-string helper). No other backend attempts recovery, and the approach carried inherent flaws (all-or-nothing re-enumeration; connection identity cannot distinguish two serial-less devices reusing the same /dev/hidrawN). On an unrecoverable POLLERR/POLLHUP/POLLNVAL the monitor thread now stops delivering events cleanly: it fabricates no DEVICE_LEFT, leaves the device cache and the application's open handles untouched, and sets monitor_dead so further hid_hotplug_register_callback() calls are refused. The bounded ENOBUFS poll retry is kept. Rework the monitor-thread reap to the join_in_progress-flag + condition-variable pattern. The thread runs joinable and publishes FINISHED as its last write; a reaper on an app thread sets join_in_progress, joins with the mutex released, then re-acquires, publishes NONE and broadcasts. A second reaper waits on the condvar instead of double-joining, and a register waiting to start a new thread waits out any in-flight join before reusing the pthread_t. A re-entrant reap from the monitor thread itself (a callback-armed TSD destructor) defers via pthread_equal; hid_exit() performs any outstanding join so the thread is provably gone once it returns. hid_internal_enumerate loses its now-unused quiet parameter; the failure out-parameter is retained for the initial-registration snapshot. Assisted-by: claude-code:claude-opus-4-8
Merge the two adjacent comment blocks (an editing artifact from the fix rounds) into one covering both the return contract and the thread-model migration note. Assisted-by: claude-code:claude-opus-4-8
The hotplug callback runs on HIDAPI's internal event context (a threadpool work item or the CM notification callback). A user callback that re-enters the public hid_hotplug_register/deregister_callback from there wrote the global error string on that context, which an application cannot serialize its lock-free hid_error(NULL) read against. Drop such writes at the global-error chokepoint while a dispatch is in progress (mutex_in_use), matching the other backends; per-device hid_error(dev) writes are unaffected. App-thread failures still set the global error as before. Also guard hid_winapi_get_container_id against a NULL dev->device_info, like its sibling accessors: hid_open_path stores hid_internal_get_device_info() without a NULL check and that helper can now return NULL. Assisted-by: claude-code:claude-opus-4-8
The previous guard suppressed global-error writes based on mutex_in_use, which
is process-global ("a dispatch is in progress somewhere"). That wrongly dropped
the genuine error of a concurrent application thread calling the hotplug API
while a callback ran on the event context, and it read the flag without
synchronization (a data race).
Use a thread-local flag set only around each user-callback invocation in both
dispatch paths instead, so suppression applies to the dispatching thread alone:
an application thread always records its errors, while a callback's nested
register/deregister writes (failure and the success-path clear) are dropped.
This matches how the other backends key off the event thread's identity.
Assisted-by: claude-code:claude-opus-4-8
…ndow The hotplug reaper releases the mutex to pthread_join() a FINISHED monitor thread. A thread-specific-data destructor running on that very thread can re-enter hid_hotplug_register_callback() during the join (the pthread_equal self-guard lets it proceed instead of cond-waiting), which finds the callback list empty and starts a NEW monitor generation, overwriting context.thread and setting thread_state = RUNNING. On re-acquiring the mutex the reaper then unconditionally stamped thread_state = NONE, clobbering that live new generation: hid_exit() would see NONE and return without joining it, leaving a monitor thread running inside the library after it is unloaded (the dlclose use-after-free). Advance to NONE only when context.thread still names the thread this reaper joined (by identity, pthread_equal) and it is still FINISHED; otherwise a newer generation now owns the lifecycle state and is left untouched (reaped by its own future reaper / hid_exit). join_in_progress is still cleared and thread_cond broadcast unconditionally, so any reaper or registration waiting out the join wakes and re-evaluates. Assisted-by: claude-code:claude-opus-4-8
__declspec(thread) is silently ignored by MinGW/Cygwin GCC (a -Werror=attributes failure), breaking the fedora-mingw and windows-cmake-mingw builds. Select __declspec(thread) for MSVC/clang-cl and __thread for GCC/Clang. Assisted-by: claude-code:claude-opus-4-8
Give the hotplug machinery a retired-thread list so a monitor generation that is superseded by a re-entrant registration - a callback's TSD destructor running on the just-finished monitor thread starts a new generation - is never dropped and is always joined before hid_exit() returns. The reaper now moves a FINISHED generation onto the list (keyed by a stable monotonic id token) and then joins it with the mutex released, so a pthread_t is never inspected after pthread_join() invalidated it. hid_exit() joins every retired generation and the current one. A per-entry being_joined claim replaces join_in_progress; the exiting guard, clean stop on monitor death, quiet error suppression on the monitor thread and the single lock order are unchanged. Assisted-by: claude-code:claude-opus-4-8
Reserve each monitor generation's retired-list node with a calloc at pthread_create time and let the generation own that node for its whole life, so hid_internal_hotplug_retire_current() only splices the already-owned node onto the retired list - a pointer move that can never fail. A finished generation is therefore always retired and joined. This removes the last OOM race: hid_exit()'s in-place-join backstop, which kept the current slot published while it dropped the mutex to pthread_join (racing a pre-exit reaper already inside the cleanup loop into a double-join or a post-join pthread_equal), is deleted as now unreachable. On node-allocation failure the registration fails cleanly without creating the thread, so no orphaned, unjoined generation is ever possible. Assisted-by: claude-code:claude-opus-4-8
Broaden the label-gated Windows virtual-device job to run every _winapi virtual-device test (DeviceIO_winapi, HotplugAPI_winapi, Hotplug_winapi) so the ci-virtual-device label actually exercises the hotplug contract against the driver-backed winapi backend. (Hotplug_winapi self-skips until the vhidmini2 disable/enable provider lands; HotplugAPI_winapi runs for real.) Assisted-by: claude-code:claude-opus-4-8
Youw
added
ci-virtual-device
Factor the plug/unplug machinery out of create()/destroy() into rg_plug() and rg_unplug() and implement test_virtual_device_unplug()/_replug() with them, so Hotplug_libusb runs against the raw-gadget provider instead of self-skipping. rg_plug() resets all per-plug state up front so a replug re-enumerates identically to the first plug (same VID/PID/serial); the mutex/cond and identity fields persist across the cycle. Make T8b (needs two concurrent devices) skip gracefully when the second create() returns TEST_VDEV_UNAVAILABLE, as on raw-gadget where only one dummy_udc.0 exists; uhid still runs it fully. Assisted-by: claude-code:claude-opus-4-8
Implement test_virtual_device_unplug/_replug for the winapi provider by disabling/enabling the root-enumerated devnode (ROOT\VHIDMINIUM\0000) via cfgmgr32, so Hotplug_winapi runs in the win-vhid CI job instead of self-skipping. Disabling the devnode tears down the HIDClass child PDO, so the GUID_DEVINTERFACE_HID interface disappears and the backend sees a removal (LEFT); enabling it re-creates the interface (ARRIVED). A devnode that cannot be located (driver/device not installed) or disabled for lack of elevation (CR_ACCESS_DENIED) maps to UNAVAILABLE, so the test skips cleanly instead of failing; other CONFIGRETs are errors. Replug failure is a hard error. destroy() best-effort re-enables the devnode so an interrupted run leaves nothing disabled. Link cfgmgr32 on the winapi vdev-test targets via target_link_libraries (not #pragma comment(lib)) so MinGW links it too. Assisted-by: claude-code:claude-opus-4-8
The win-vhid hotplug provider assumed the root devnode instance id was ROOT\VHIDMINIUM\0000, but PnP derives it from the driver's setup class (Class=HIDClass) as ROOT\HIDCLASS\0000, so the probe never found it and Hotplug_winapi always self-skipped. Locate the devnode by scanning the ROOT enumerator for the INF hardware id (root\VhidminiUm) instead, robust to the instance path and index. Add CONFIGRET diagnostics on the skip/error paths and run the winapi tests with ctest -V so the plug/unplug flow is visible in CI. Assisted-by: claude-code:claude-opus-4-8
With the devnode now located, the win-vhid hotplug probe got past unplug and the absence wait, but CM_Enable_DevNode alone did not re-create the child HID PDO, so the device never re-enumerated and Hotplug_winapi skipped after a full timeout. Force a synchronous re-enumeration of the parent subtree after enabling, so PnP restarts the function device and rebuilds the GUID_DEVINTERFACE_HID interface. Also log the post-enable devnode status/problem code for diagnosis. Assisted-by: claude-code:claude-opus-4-8
…eplug The previous replug re-enumerated the parent (ROOT), which only re-scans ROOT's children (the already-started function device) and never re-queries the function device's own children, so the HID child PDO stayed gone and Hotplug_winapi still skipped after the enumerate timeout - the diagnostic confirmed the function device was DN_STARTED with problem=0. Re-enumerate the function devnode itself so PnP re-queries its children and the HID collection is rebuilt, and list the child devnodes for diagnosis. Assisted-by: claude-code:claude-opus-4-8
…tplug Disabling the function devnode (ROOT\HIDCLASS\0000) tore down the whole UMDF stack; re-enabling restarted the function device (DN_STARTED, problem=0) and re-created the child HID PDO, but the child came back non-started, so its GUID_DEVINTERFACE_HID interface never activated and hid_enumerate stayed blind - Hotplug_winapi skipped after the enumerate timeout. Instead find the HID child of the function device and disable/enable that leaf directly, mirroring Device Manager: the UMDF host keeps running and only the HID interface is toggled, so LEFT/ARRIVED land and the device re-enumerates. Keep the child's post-enable status diagnostic. Assisted-by: claude-code:claude-opus-4-8
The real reason Hotplug_winapi kept skipping after every PnP fix: the test looks for VID:PID 0xF1D0:0x9002 with serial "HIDAPI-HOTPLUG-TEST", but the pre-built vhidmini driver reports 0xF1D0:0x9001 with a different serial, so hid_enumerate(0xF1D0, 0x9002) never matched and the device was reported as never enumerating - regardless of the plug/unplug mechanism. Unlike Linux/macOS, where the provider creates a device with the requested identity, the Windows driver is static, so the test must match it. - test_hotplug.c: on Windows the primary device uses the driver's PID (0x9001); other platforms keep 0x9002 (still distinct from the device-I/O test there). - driver: report serial "HIDAPI-HOTPLUG-TEST" so the serial match succeeds. - Windows provider create(): probe hid_enumerate and return UNAVAILABLE when the requested device is not actually present, so the mid-pass-stop test's second device (0x9003, no counterpart on Windows) skips cleanly. Logs the device's real serial/path for diagnosis. Assisted-by: claude-code:claude-opus-4-8
Remove the success-path [win-vdev] create/replug logging added while diagnosing the skip, keeping only the error-path diagnostics that fire on a genuine failure, and restore ctest --output-on-failure for the winapi run. 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
1 participant
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.
Integration / CI-validation branch — not a merge unit. This aggregates the four backend
hotplug implementations (#822 linux, #823 windows, #824 mac, #825 libusb) and the test
suite (#826) on top of
connection-callback, so the hotplug tests run against the realasynchronous implementations together. Those five PRs are the actual merge units into
connection-callback; this branch exists only to exercise the tests end-to-end (the tests on#826alone sit onconnection-callback, whose backends are still the old synchronous ones).What runs, and where
Ordinary per-push CI (no label) — runs against the real async backends:
HotplugAPI_{hidraw,libusb,winapi,darwin}— the device-less contract suite (argumentvalidation, handle semantics,
hid_exitteardown races, thread churn), exercising eachreal backend's event machinery on every platform.
Hotplug_hidraw— the full behavioral suite (T6–T15) driven by realuhidplug/unplugon Linux. This is the one platform with working device-backed hotplug testing.
Label-gated extended CI (
ci-virtual-device):win-vhid-test— installs the vhidmini2 driver and runs the_winapivirtual-device tests(
DeviceIO_winapi+HotplugAPI_winapirun for real;Hotplug_winapiself-skips — see below).libusb-vhid-test— runs the suite inside araw_gadget+dummy_hcdVM(
DeviceIO_libusb+HotplugAPI_libusbrun for real;Hotplug_libusbself-skips).Local-only: the macOS device-backed provider needs the
com.apple.developer.hid.virtual.deviceentitlement, soHotplug_darwinself-skips on hosted CI.Known coverage gap
Real device-backed hotplug plug/unplug currently works only via
uhid(Linux). TheHotplug_winapiandHotplug_libusbproviders' plug/unplug halves are stubs (they returnTEST_VDEV_UNAVAILABLE, so those tests self-skip). Wiring them up is the deferred tier-3/tier-4provider work:
raw_gadgetre-bind for libusb, andCM_Disable_DevNode/CM_Enable_DevNodeon the root devnode for the Windows driver. Until then, the label validates the virtual-device
harness + the device-less hotplug contract in those environments, not device-backed hotplug events.
Assisted-by: claude-code:claude-opus-4-8