libusb: deliver hotplug ENUMERATE pass asynchronously on the event context#825
Open
Youw wants to merge 6 commits into
Open
libusb: deliver hotplug ENUMERATE pass asynchronously on the event context#825Youw wants to merge 6 commits into
Youw wants to merge 6 commits into
Conversation
…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
… 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
…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
…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
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
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 libusb backend: theHID_API_HOTPLUG_ENUMERATEinitial pass is now delivered asynchronously on the same internal event context as live events, never from withinhid_hotplug_register_callback()and never on an application thread.What changed
hid_hotplug_register_callback()no longer invokes the callback synchronously. WhenHID_API_HOTPLUG_ENUMERATEis set (andDEVICE_ARRIVEDis requested), it builds a deep-copied snapshot of the matching entries of the internal device cache (hid_hotplug_context.devs) under the hotplug mutex and stores it on the new callback record (replay).device == NULL) is enqueued on the existing callback-thread queue so the snapshot is delivered promptly even when there is no live hotplug traffic.hid_internal_invoke_callbacks()flushes that callback's replay first. Combined with the snapshot being taken from the cache (which does not yet contain arrivals still sitting in the queue backlog), this preserves exactly-once per device connection and arrival-before-left forENUMERATEregistrations. A non-zero callback return during the pass stops the remainder of the pass and deregisters the callback, matching the live-event behavior.device->nextis NULL for every invocation, including multi-interface/composite devices (each interface entry is delivered as a standalone invocation; the internal cache chain is preserved for LEFT matching). This closes the old TODO in the arrival path.hid_hotplug_register_callback()/hid_hotplug_deregister_callback()(argument validation, missingLIBUSB_CAP_HAS_HOTPLUG,libusb_init/libusb_hotplug_register_callbackfailures, unknown handles) now set the global error via the hid_error implementation for libusb #708 infrastructure, and*callback_handleis zeroed on any registration failure.shutdown_pendingflag instead of watching the callback list, and the thread join is deferred to the next registration orhid_exit():hid_exit().hid_init()or in the machinery initialization (guarded by a statically-initialized mutex).Verification
gcc -std=c99 -fsyntax-only -Wall -Wextra -pedantic -Werrorandg++ -x c++ -Wall -Wextra -Werrorboth clean; full CMake build withHIDAPI_WITH_LIBUSB=TRUEand tests passes.ENUMERATEwith no devices, no synchronous invocation from register, register/deregister churn,hid_exit()teardown with a live registration, re-init afterhid_exit()) passes cleanly under AddressSanitizer/UBSan (no leaks) and ThreadSanitizer (no warnings), including a 4-thread concurrent register/deregister stress.Addresses #792 and #793 for the libusb backend.
Review round 1
Follow-up commit addressing the review findings. Each item below is covered by a regression test that fails when the fix is reverted (see Verification (round 1)).
Correctness / lost events
LIBUSB_HOTPLUG_ENUMERATE), and its later removal was silently dropped as well. The reverse order can only report a device twice, which the arrival path now suppresses using the same identity the removal path matches on.hid_enumerate()failure is no longer mistaken for an empty system. The unconditional error reset masked it, so the registration succeeded with an empty cache and every already-connected device stayed invisible to that callback and all later ones. An enumeration failure now fails the registration and preserves the libusb error; only the genuinely-empty case resets it.Thread safety
hid_hotplug_register_callback()/hid_hotplug_deregister_callback()are thread-safe by contract, but their failure paths wrotelast_global_erroroutside any lock: two concurrent calls could double-free the stored error string. All mutations now go through a file-static mutex in the lowest-level writer.mutex_readyis no longer read without synchronization, and the hotplug mutex is no longer destroyed byhid_exit()while another thread is about to lock it. The flag is a one-way latch, written and read under the init mutex; the mutex and the thread states it advertises are reused by a later registration rather than destroyed.unlock/lockspin, which was unfair and could livelock under a real-time scheduling policy (the spinner starving the joiner).Lifecycle
dlcloseraced the threads into unmapped code, and the header's "the machinery runs until the last callback is deregistered" was only eventually true. The join is still deferred when the shutdown is initiated from the event thread itself, which cannot join itself.next_handle++atINT_MAXwas signed-overflow UB, and the wrap-around to 1 could collide with a live handle; registration now fails with an error instead.Failure paths
ENUMERATEsnapshot is all-or-nothing: a failed device-info copy used to be skipped, silently hiding a connected device from that callback forever. It now unwinds the registration.hidapi_thread_create()returnedvoidandthreads_runningwas set regardless, so a failedpthread_create()left a garbagepthread_tto be joined later (UB) and a queue that never drained. It returns a status now, the callback thread is started by the registration (rather than by the libusb thread, which had no way to report a failure), and either failure fully unwinds the registration.Verification (round 1)
gcc -std=c99 -pedantic -Wall -Wextra -Werrorandg++ -x c++ -Wall -Wextra -Werrorclean; CMake builds withHIDAPI_WITH_LIBUSB=TRUE,HIDAPI_WITH_TESTS=ONand withHIDAPI_BUILD_AS_CXX=ONpass.hid_exit()with a live registration and with a deferred wind-down, register/deregister churn, and a 4-thread register/deregister stress with concurrent device churn.shutdown_pending- both of which are fixed above.Assisted-by: claude-code:claude-fable-5
Assisted-by: claude-code:claude-opus-4-8