RFC: Hotplug implementation (combined improvements for testing) - #836
Draft
Youw wants to merge 117 commits into
Draft
RFC: Hotplug implementation (combined improvements for testing)#836Youw wants to merge 117 commits into
Youw wants to merge 117 commits into
Conversation
…back (#299) - initial API; - Windows backend implementation;
- fix merge conflict - fix indentation in a few places
* netbsd hotplug stubs * Make cygwin happy (fix copied from libusb)
…EnterCriticalSection (#689)
Fix the possible issues that happen when a register or de-register call is made from inside a callback. The way it works is the same for all platforms and is described below. Resolves: #673 1) The mutex is made recursive, so it can be locked by the same thread multiple times 2) `mutex_ready` kept intact, added 2 more flags, `mutex_in_use` and `cb_list_dirty`. 3) When the `mitex_in_use` flag is set, the Deregister call is no longer allowed to immediately remove any callbacks from the list. Instead, the `events` field in the callback is set to 0, which makes it "invalid", as it will no longer match any events, and the `cb_list_dirty` flag is set to 1 to indicate that the list needs to be checked for invalid events later. 4) When a callback returns a non-zero value, indicating that the callback is to be disarmed and removed from the list, it is marked in the same manner until the processing finishes (unless the callback was called directly by the Register call, in which case it's return value is ignored on purpose) 5) After all the callbacks are processed, if `cb_list_dirty` flag is set, the list of callbacks is checked for any callbacks marked for removal (`events` field set to 0), and those are only removed after all the processing is finished. 6) The Register call is allowed to register callbacks, as it causes no issues so long as the mutex it locks is recursive 7) Since the Register call can also call the new callback if `HID_API_HOTPLUG_ENUMERATE` is specified, `mutex_in_use` flag is set to prevent callback removal in that new callback. 8) The return value of any callbacks called for pre-existing devices is still ignored as per documentation and does not mark them invalid.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Callback_thread used hidapi_thread_cond_timedwait with an absolute timespec already in the past, causing a tight spin loop. Replaced with cond_wait + predicate pattern so the thread sleeps until an event or a shutdown signal arrives. Multithreading fixes folded in along the way: - Queue mutex moved from libusb_thread to callback_thread (its natural condvar partner), with signal-under-mutex to avoid a lost-wakeup race at shutdown. - process_hotplug_event() now runs under hid_hotplug_context.mutex to match hid_hotplug_register_callback()'s HID_API_HOTPLUG_ENUMERATE traversal of hid_hotplug_context.devs — fixes a use-after-free where a concurrent disconnect could free a list node mid-traversal. - devs teardown moved to hid_internal_hotplug_cleanup() (after hidapi_thread_join) so every access to devs happens under hid_hotplug_context.mutex. - hid_hotplug_register_callback() now frees devs on the libusb_hotplug_register_callback() error path, which previously leaked. Closes: #782 Assisted-by: Copilot:claude-sonnet-4.6 Assisted-by: Copilot:claude-opus-4.6 Assisted-by: Claude:claude-opus-4.7
The windows-cmake job set up the MSVC environment by calling a hard-coded "C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Auxiliary\Build\vcvars64.bat" in its NMake, clang-cl and Meson steps. The hosted windows-latest image no longer has Visual Studio under that "Enterprise" path, so those steps started failing with "The system cannot find the path specified." Set up the developer environment once per job with the TheMrMilchmann/setup-msvc-dev action -- it locates the installed toolchain (no hard-coded path or edition) -- and drop all the hard-coded vcvars64.bat calls. That environment is job-wide, so the MinGW build (which must use gcc, not cl) is split out into its own windows-mingw job that runs without the MSVC environment. The MSVC, NMake, clang-cl and Meson builds stay in windows-cmake with the action. Assisted-by: claude-code:claude-opus-4-8 (cherry picked from commit 2514836)
) Document the full hotplug API contract in hidapi.h: thread-safety rules (re-entrant hotplug mutex, safe-to-call list inside callbacks), execution context (callbacks only ever run on HIDAPI's internal event context), HID_API_HOTPLUG_ENUMERATE redefined as an asynchronous initial pass from a registration-time snapshot (exactly-once, delivered before live events, return value honoured), device lifetime rules (device->next == NULL, cached DEVICE_LEFT info), deregistration post-conditions, handle non-reuse, argument validation, and Since-version annotations. Implementation gaps vs this contract are tracked as follow-up issues (see PR description). Supersedes #784. Assisted-by: claude-code:claude-fable-5
Brings in #813 (IWYU + leak guards), #815 (virtual-device test harness), #817 (CI vcvars fix, already cherry-picked) and #708 (libusb hid_error). Conflicts in libusb/hid.c (hotplug context vs last_global_error, refactored hid_enumerate vs its error reporting) and linux/hid.c (includes) resolved by keeping both sides' functionality. Assisted-by: claude-code:claude-fable-5
The HIDAPI_BUILD_AS_CXX check (from #814, merged from master) rejected the hotplug code: out-of-order/partial designated initializers on the static hotplug contexts, int-to-enum conversions on the callback events bitmask, and an uncast calloc. Replace the static initializers with zero-initialized storage plus lazy next_handle setup, make the internal events field a plain int bitmask, and cast the remaining sites. Assisted-by: claude-code:claude-fable-5
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
tests-1: apply test and harness review findings tests-2: apply test and harness review findings tests-3: apply test and harness review findings tests-4: apply test and harness review findings tests-5: apply test and harness review findings tests-6: apply test and harness review findings tests-7: apply test and harness review findings tests-8: apply test and harness review findings tests-9: apply test and harness review findings tests-10: apply test and harness review findings tests-11: apply test and harness review findings tests-12: apply test and harness review findings tests-13: apply test and harness review findings tests-14: apply test and harness review findings tests-15: apply test and harness review findings tests-16: apply test and harness review findings tests-17: apply test and harness review findings tests-18: apply test and harness review findings tests-19: apply test and harness review findings tests-20: apply test and harness review findings tests-21: apply test and harness review findings tests-22: apply test and harness review findings tests-23: apply test and harness review findings tests-24: apply test and harness review findings tests-25: apply test and harness review findings tests-26: apply test and harness review findings tests-27: apply test and harness review findings tests-28: apply test and harness review findings tests-29: apply test and harness review findings tests-30: apply test and harness review findings tests-31: apply test and harness review findings tests-32: apply test and harness review findings tests-33: apply test and harness review findings tests-34: apply test and harness review findings tests-35: apply test and harness review findings tests-36: apply test and harness review findings tests-37: apply test and harness review findings tests-38: apply test and harness review findings tests-39: apply test and harness review findings docs-3: apply test and harness review findings docs-4: apply test and harness review findings docs-16: apply test and harness review findings docs-17: apply test and harness review findings docs-18: apply test and harness review findings docs-19: apply test and harness review findings docs-20: apply test and harness review findings docs-21: apply test and harness review findings docs-22: apply test and harness review findings docs-23: apply test and harness review findings docs-38: apply test and harness review findings docs-39: apply test and harness review findings docs-40: apply test and harness review findings docs-41: apply test and harness review findings docs-42: apply test and harness review findings docs-43: apply test and harness review findings docs-44: apply test and harness review findings docs-45: apply test and harness review findings docs-46: apply test and harness review findings docs-61: apply test and harness review findings docs-62: apply test and harness review findings Assisted-by: codex-cli:gpt-6-astra
linux-r2-1: Retain failed joins without retrying or spinning at exit. linux-r2-2: Limit fatal receive errors to broken or overrun transports. Assisted-by: codex-cli:gpt-6-astra
libusb-r2-1: Propagate configuration descriptor allocation failures. libusb-r2-2: Destroy the dedicated context before the pump returns. libusb-r2-3: Reconcile dropped removals before later snapshots. Assisted-by: codex-cli:gpt-6-astra
mac-r2-1: Retain event identity and join ownership through teardown. Assisted-by: codex-cli:gpt-6-astra
windows-r2-1: Queue recovered arrivals for ordered boundary dispatch. Assisted-by: codex-cli:gpt-6-astra
libusb-r3-1: Retain retired identities and deliver unseen arrivals. Assisted-by: codex-cli:gpt-6-astra
tests-r2-1: Limit truncation failures to test-device payloads. tests-r2-2: Gate exit delivery and use a fresh post-init counter. tests-r2-3: Validate cancellation before retrying target cleanup. tests-r2-4: Skip candidates that disappear during devnode queries. tests-r2-5: Correct Windows provider creation comments. Assisted-by: codex-cli:gpt-6-astra
# Conflicts: # README.md
# Conflicts: # src/tests/CMakeLists.txt
mac-r3-1: Publish the OS thread ID atomically and test owner-lock calls. Assisted-by: codex-cli:gpt-6-astra
libusb-r4-1: Release retired identities on a queued LEFT. libusb-r4-2: Reconcile stale devices before every registration. libusb-r4-3: Clear stale state while preserving retries and wake-ups. Assisted-by: codex-cli:gpt-6-astra
windows-r3-1: Explain recovered arrival and snapshot delivery order. windows-r3-2: Clarify recovered arrival recipients and pairing. windows-r3-3: Explain wait error reporting after completed teardown. Assisted-by: codex-cli:gpt-6-astra
…mption - rename the shadowed serial-narrowing loop variable (C4456) - replace getenv() with a Win32-safe helper (C4996 under /W4 /WX) - T15: both callbacks must run off the registering thread, but the Windows backend delivers from a threadpool / CM notification thread, so the two invocations need not share one thread id - document why cb_publication may read the registering thread's handle - libusb-vhid CI: fetch dummy_hcd.c through the authenticated contents API with retries; anonymous raw.githubusercontent.com fetches hit 429 Assisted-by: claude-code:claude-fable-5-1
pthread_threadid_np() never fails for the calling thread, but its result was ignored; a zero id would silently disable the destructor-phase identity fallback. Fail startup cleanly instead. Also document the (practically unreachable) non-recoverable state after a failed join in hid_exit() and hid_hotplug_deregister_callback(). Assisted-by: claude-code:claude-fable-5-1
Youw
added
the
ci-virtual-device
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.
This is a combined branch for testing all the hotplug improvements made recently by various AI agents together. It collects the current improvements to
connection-callbackonconnection-callback-2, withmasteras the comparison target, alongside the original draft PR #674. The existing PRs remain open for individual review.The combined branch includes the current tips of all seven open PRs targeting
connection-callback:connection-callback-2includes the complete integration history from9fdce4186c17c6d81b20a6595029ca38409844ac, the updated libusb PR #825, and the latestmasterat852cc68b8e0e4c7eba0815ef992951cd8b75c55b. Codex verified that every current PR tip listed above andmasterare ancestors of the combined branch.The
libusb/hidapi_libusb.hconflict was resolved in #825 by mergingmasterand preserving both the hotplug error-serialization documentation and the timeout declarations from #832. The updated libusb branch was then merged into this combined branch; no conflict remains withmaster.Validation of the updated combined tree (
932e5ac):HotplugAPI_hidrawandHotplugAPI_libusbpassed; four device-backed tests self-skipped because the required virtual devices are unavailable locally.HotplugAPI_winapi; two device-backed tests self-skipped because the virtual HID driver is not installed locally.The updated PR commits trigger fresh CI. The test setup and platform requirements are documented in
src/tests/README.md.Related to #674 and #238 (hotplug support), #785 (Windows error handling), #791 and #792 (callback device-list contract), #793 (enumeration callback return values), and #794 (macOS self-join deadlock).
Builds on the existing work from #708 (libusb error reporting), #790 (hotplug contract), and #815 (virtual-device test harness). Related follow-ups remain #828 (Linux monitor recovery) and #829 (custom libusb thread-model compatibility).
Also includes the latest merged
masterwork from #831 (pkg-config generation), #832 (libusb timeouts), and #834 (API usage contract).Generated by Codex (GPT-6 Astra).
Assisted-by: codex:gpt-6-astra