linux: deliver hotplug ENUMERATE pass asynchronously on the event context#822
Open
Youw wants to merge 8 commits into
Open
linux: deliver hotplug ENUMERATE pass asynchronously on the event context#822Youw wants to merge 8 commits into
Youw wants to merge 8 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
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
…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
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
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
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
…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
…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
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 linux/hidraw backend.What changed
Asynchronous ENUMERATE pass.
hid_hotplug_register_callback()no longer runs theHID_API_HOTPLUG_ENUMERATEinitial pass synchronously on the calling thread. Instead it takes a deep-copied, registration-time snapshot of the matching entries of the internal device cache (only whenHID_API_HOTPLUG_EVENT_DEVICE_ARRIVEDis requested), stores it on the callback record (replay), and the udev monitor thread delivers it as synthetic "arrived" events:hid_internal_invoke_callbacks()flushes a callback's pending snapshot before handing it a live event, and the monitor thread also flushes pending snapshots at the top of its loop (the existing 5 ms poll timeout caps the latency, no extra wakeup fd needed);This also covers registrations made while the monitor thread is already running, including registrations made from within another hotplug callback.
device->nextis now NULL for every invocation. The udev arrival path used to pass the chained multi-usage list to callbacks (the old TODO); each entry is now delivered on its own, keeping the internal cache intact for later "left" matching.Error strings and out-parameter. Every failure path of register/deregister (NULL callback, empty/unknown events bits, unknown flags bits, allocation/udev/thread-start failures, unknown or stale deregister handle) now sets the global error string retrievable via
hid_error(NULL), and register sets*callback_handleto 0 on failure before any error return.Monitor-thread lifecycle fixes. Previously, when the last callback removed itself on the monitor thread (non-zero return or in-callback deregister), the thread exited on its own, was never joined, and freed the shared device list / udev monitor unlocked — racing a subsequent first registration that re-creates them. Now the exiting thread releases nothing; joining and monitoring-context teardown are centralized in
hid_internal_hotplug_cleanup()and a new first registration reaps the previous thread before re-creating the context.Design decisions / deviations
hid_enumerate(0, 0)inside the first registration may set the "No HID devices found" global error on an empty system; register clears it, since an empty system is not a registration failure. A genuine enumeration failure does fail the registration.Verification
gcc -fsyntax-only -Wall -Wextra -pedantic -Werror -Wformat-signednessandg++ -fsyntax-only -Wall -Wextra -Werrorclean; full CMake builds (C andHIDAPI_BUILD_AS_CXX=ONwith-Werror) pass.CONFIG_UHIDnot set), so the tests: virtual HID device test harness for all four backends (hidraw, libusb, winapi, darwin) #815 virtual-device harness could not drive real udev events. Instead a throwaway whitebox harness (includinglinux/hid.cdirectly, injecting fake entries into the hotplug device cache and driving the internal arrival/removal handlers) exercised 119 checks: async delivery on the monitor thread (never the registering thread), VID/PID filtering of the snapshot,next == NULLon every invocation, exactly-once vs. later-connected devices and vs. a pre-queued add, non-zero return stopping the pass, self-deregistration and nested registration from within a callback, registration from within an in-flight dispatch (both event types, multi-usage devices), tombstoned/stale-handle deregister, handle exhaustion, last-callback self-removal followed by re-registration (thread reaping),hid_exit()with an undelivered snapshot, re-initialization afterhid_exit(), concurrent first registrations, concurrent stale deregistrations, and all argument-validation error strings.Addresses #792 and #793 for the linux/hidraw backend.
Review round 1
All findings from the first review round are addressed in
linux: address the hotplug review findings:last_global_error_strwith no lock (on the validation paths, before the hotplug mutex exists, and on the post-unlock paths), and the monitor thread wrote it too (viacreate_device_info_for_device()); two concurrent, documented-thread-safe calls could double-free it. Every mutation now goes through a file-static mutex in the lowest-level writer (register_error_str()), which all theregister_global_error*()/register_device_error*()helpers funnel into. Reading viahid_error()remains subject to the documented thread-safety rules.mutex_ready == 0and bothpthread_mutex_init()the hotplug mutex (undefined behaviour) while resetting the context fields under each other. A static bootstrap mutex now guards the machinery init/teardown block.hid_init()race (found while making TSan clean). A registration implicitly initializes the library twice — directly, and again inside the initial enumeration — andsetlocale()is not thread-safe against itself, so two concurrent registrations raced inhid_init().hid_init()now serializes itself.trylocks every iteration and checks the callback list (and the pending initial passes) only under the acquired mutex, so a stale read cannot keep it alive or make it exit early. The thread announces its exit under the mutex, andhid_internal_hotplug_cleanup()unlocks beforepthread_join()— nothing is ever joined while holding the mutex. This removed the previously "excused" races, and TSan is now fully clean.udev_monitor_enable_receiving()and the initialhid_enumerate()was in both the snapshot and the queued netlink event, producing a duplicate "arrived", a duplicate cache entry and later a duplicate "left". Arrivals whose path is already cached are now skipped.*callback_handleat 0), sets the error string and returns -1. A partialhid_internal_copy_device_info()result is likewise rejected instead of reaching a callback. A genuinehid_enumerate()failure now fails the registration, and is distinguished from an empty system.0is a valid file descriptor:-1is now the sentinel, and the checks test for< 0.next_handle++atINT_MAXwas signed-overflow undefined behaviour, and wrapping back to 1 could collide with a live handle. Registration now fails with an error instead of wrapping.hid_exit().poll()replacesselect()for the monitor file descriptor (noFD_SETSIZEundefined behaviour).Assisted-by: claude-code:claude-fable-5
Assisted-by: claude-code:claude-opus-4-8