gh-libusb/hidapi

Watch

Fork 0

libusb: deliver hotplug ENUMERATE pass asynchronously on the event context #825

Open

Youw wants to merge 15 commits from hotplug-async-enumerate-libusb into connection-callback

Youw commented

2026-07-13 17:46:36 -05:00

(Migrated from github.com)

Implements the hotplug contract documented in hidapi/hidapi.h (#790) for the libusb backend: the HID_API_HOTPLUG_ENUMERATE initial pass is now delivered asynchronously on the same internal event context as live events, never from within hid_hotplug_register_callback() and never on an application thread.

What changed

Registration-time replay snapshot. hid_hotplug_register_callback() no longer invokes the callback synchronously. When HID_API_HOTPLUG_ENUMERATE is set (and DEVICE_ARRIVED is 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).
Queue marker wake-up. A lightweight "replay marker" message (device == NULL) is enqueued on the existing callback-thread queue so the snapshot is delivered promptly even when there is no live hotplug traffic.
Pass-before-live ordering. Before dispatching any live event to a callback with a pending snapshot, 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 for ENUMERATE registrations. A non-zero callback return during the pass stops the remainder of the pass and deregisters the callback, matching the live-event behavior.
device->next is 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.
Error strings. All failure paths of hid_hotplug_register_callback() / hid_hotplug_deregister_callback() (argument validation, missing LIBUSB_CAP_HAS_HOTPLUG, libusb_init / libusb_hotplug_register_callback failures, unknown handles) now set the global error via the #708 infrastructure, and *callback_handle is zeroed on any registration failure. (when called from an application thread; per hidapi.h, a call made from within a hotplug callback reports only -1 and leaves hid_error(NULL) untouched)
Shutdown/teardown rework. The event threads now wind down on a dedicated shutdown_pending flag instead of watching the callback list, and the last application-thread deregistration winds the event threads down synchronously; the join is deferred to the next registration or hid_exit() only when the last callback is removed from the event thread itself (which cannot join itself):
- removing the last callback from within a callback (non-zero return or in-callback deregister) can no longer self-join the event threads;
- deregistering the last callback from an application thread while events are still queued can no longer deadlock (previously the join was performed while holding the mutex that the callback thread needs to drain its queue);
- the message queue (device references and markers) is drained before the hotplug libusb context is destroyed, and undelivered replay snapshots are freed on deregistration and hid_exit().
Registration is thread-safe end-to-end: concurrent registrations no longer race in the implicit hid_init() or in the machinery initialization (guarded by a statically-initialized mutex).

Verification

gcc -std=c99 -fsyntax-only -Wall -Wextra -pedantic -Werror and g++ -x c++ -Wall -Wextra -Werror both clean; full CMake build with HIDAPI_WITH_LIBUSB=TRUE and tests passes.
A smoke test (registration/deregistration, argument validation and error strings, ENUMERATE with no devices, no synchronous invocation from register, register/deregister churn, hid_exit() teardown with a live registration, re-init after hid_exit()) passes cleanly under AddressSanitizer/UBSan (no leaks) and ThreadSanitizer (no warnings), including a 4-thread concurrent register/deregister stress.
Live device plug/unplug was not exercised (no USB passthrough available in the build environment).

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

An arriving device is now delivered exactly once, even to a callback registered from within another callback. The arrival path invoked the callbacks for each interface before appending the new entries to the device cache, and recomputed the "do not deliver to callbacks registered during this dispatch" boundary per interface. A callback registered from inside another callback during a live arrival therefore snapshotted a cache that did not yet contain the arriving device and was excluded from the in-flight dispatch, so the device reached it through neither path (and was later reported as LEFT without a preceding ARRIVED); for multi-interface devices the remaining interfaces were delivered twice instead. The infos are now appended to the cache before any callback runs, and the boundary is computed once per hotplug message.
The libusb listener is armed before the initial enumeration, and arrivals are deduplicated against the cache. Previously the snapshot was taken first: a device connecting in between was in neither the snapshot nor any live event (libusb does not back-fill without 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.
A genuine 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

The global error state is serialized. hid_hotplug_register_callback() / hid_hotplug_deregister_callback() are thread-safe by contract, but their failure paths wrote last_global_error outside 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_ready is no longer read without synchronization, and the hotplug mutex is no longer destroyed by hid_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.
The deferred join is awaited on a condition variable instead of a busy unlock/lock spin, which was unfair and could livelock under a real-time scheduling policy (the spinner starving the joiner).

Lifecycle

Deregistering the last callback from an application thread now winds the event threads down synchronously. It used to return while they were still winding down, so deregister + dlclose raced 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.
Deregistering an already-removed callback fails again instead of returning success a second time: a handle whose removal was postponed (an in-callback deregistration) no longer matches the lookup.
Handle allocation refuses to overflow. next_handle++ at INT_MAX was signed-overflow UB, and the wrap-around to 1 could collide with a live handle; registration now fails with an error instead.

Failure paths

The ENUMERATE snapshot 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.
A failed replay-marker enqueue used to be ignored, deferring the whole pass indefinitely when no live event followed; it now fails the registration too. (A dropped live event on enqueue failure is pre-existing and unavoidable - libusb offers no way to postpone or retry it - and is now called out in a comment.)
Both event threads are checked at creation. hidapi_thread_create() returned void and threads_running was set regardless, so a failed pthread_create() left a garbage pthread_t to 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 -Werror and g++ -x c++ -Wall -Wextra -Werror clean; CMake builds with HIDAPI_WITH_LIBUSB=TRUE, HIDAPI_WITH_TESTS=ON and with HIDAPI_BUILD_AS_CXX=ON pass.
The smoke test was rebuilt on top of an in-process libusb replacement, so arrivals, removals, multi-interface devices, enumeration/allocation/thread-creation failures and the arm-before-snapshot window can be simulated without real hardware (no USB access is available in the build environment). 18 test cases, including nested registration during a live arrival (single- and multi-interface, asserting exactly-once delivery and no LEFT-without-ARRIVED), arrival deduplication, the double-deregister contract, the synchronous wind-down, the deferred-shutdown race, OOM injection on both snapshot paths, both thread-creation failures, handle exhaustion, 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.
Every fix above was validated with a negative control: the defect was re-introduced and the corresponding test observed to fail.
10/10 clean runs under ASan + UBSan + LSan (no leaks, including the libusb device references) and 10/10 clean under TSan. TSan caught two real defects introduced during this round - a lock-order inversion between the init mutex and the hotplug mutex (a nested registration from a callback takes them in the opposite order) and an under-synchronized write to shutdown_pending - both of which are fixed above.
Live device plug/unplug on real hardware is still not exercised.

Final state (after review)

Final design: two internal threads (a libusb event pump + a callback thread draining a message queue), with the ENUMERATE pass delivered as a marker message in that queue; hid_exit() tears the hotplug machinery down and then calls libusb_exit(usb_context) under the hotplug mutex as one transaction (no resurrected context, no orphaned threads); device identity compares the bus-port prefix including the : separator (fixing a port-2-vs-port-20 mismatch that silently dropped arrivals); and internal-thread writes to the global error string are suppressed via the captured callback-thread id. hidapi_thread_create() now returns int so a pthread_create failure can be reported and unwound (a source change for out-of-tree thread models тАФ see #829).

libusb backend: hidapi_thread_create() in custom HIDAPI_THREAD_MODEL_INCLUDE thread models must now return int (0 on success); void implementations no longer compile.

Verification. Every fix commit on this branch was adversarially delta-verified by an independent reviewer (Codex on a separate plan, or a fresh model) tasked to find what the fix itself broke тАФ a loop that caught a defect in nearly every fix commit. A per-backend white-box harness injects fake device events into the real implementation and runs under ASan/UBSan/LSan and TSan (TSan proven live). The hotplug-integration branch тАФ all four final backends merged with the test suite (#826) тАФ is CI-green on every platform, with HotplugAPI_libusb executing per push and Hotplug_libusb in the label-gated libusb-vhid-test VM job.

Implements device->next == NULL (#791, #792) and the ENUMERATE return-value (#793) contract for libusb. Part of hotplug support (#238); contract documented in #790. Follow-up: #829.

Assisted-by: claude-code:claude-opus-4-8

Assisted-by: claude-code:claude-opus-5
Assisted-by: claude-code:claude-fable-5
Assisted-by: codex-cli:gpt-6-astra

Master integration

The latest master at 852cc68 is now merged into this branch in bf7d6ba. Codex resolved the libusb/hidapi_libusb.h conflict by preserving the hotplug error-serialization documentation alongside master's timeout declarations and formatting. The updated branch is also merged into the combined test PR #836.

Validation on the updated combined tree (932e5ac): Linux/WSL GCC and Windows/MSVC AddressSanitizer builds passed; HotplugAPI_hidraw, HotplugAPI_libusb, and 26 Windows tests passed. Four Linux and two Windows device-backed tests self-skipped because the required virtual devices are unavailable locally. The libusb implementation, header, thread model, and CMake file are identical between this branch and the tested combined tree. A focused independent merge review found no integration issues.

Related to #831, #832, #834 (the merged master changes), and #836 (combined testing).

Generated by Codex (GPT-6 Astra).

Assisted-by: codex:gpt-6-astra

Implements the hotplug contract documented in `hidapi/hidapi.h` (#790) for the libusb backend: the `HID_API_HOTPLUG_ENUMERATE` initial pass is now delivered asynchronously on the same internal event context as live events, never from within `hid_hotplug_register_callback()` and never on an application thread. ### What changed - **Registration-time replay snapshot.** `hid_hotplug_register_callback()` no longer invokes the callback synchronously. When `HID_API_HOTPLUG_ENUMERATE` is set (and `DEVICE_ARRIVED` is 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`). - **Queue marker wake-up.** A lightweight "replay marker" message (`device == NULL`) is enqueued on the existing callback-thread queue so the snapshot is delivered promptly even when there is no live hotplug traffic. - **Pass-before-live ordering.** Before dispatching any live event to a callback with a pending snapshot, `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 for `ENUMERATE` registrations. A non-zero callback return during the pass stops the remainder of the pass and deregisters the callback, matching the live-event behavior. - **`device->next` is 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. - **Error strings.** All failure paths of `hid_hotplug_register_callback()` / `hid_hotplug_deregister_callback()` (argument validation, missing `LIBUSB_CAP_HAS_HOTPLUG`, `libusb_init` / `libusb_hotplug_register_callback` failures, unknown handles) now set the global error via the #708 infrastructure, and `*callback_handle` is zeroed on any registration failure. (when called from an application thread; per hidapi.h, a call made from within a hotplug callback reports only -1 and leaves hid_error(NULL) untouched) - **Shutdown/teardown rework.** The event threads now wind down on a dedicated `shutdown_pending` flag instead of watching the callback list, and the last application-thread deregistration winds the event threads down synchronously; the join is deferred to the next registration or hid_exit() only when the last callback is removed from the event thread itself (which cannot join itself): - removing the last callback from within a callback (non-zero return or in-callback deregister) can no longer self-join the event threads; - deregistering the last callback from an application thread while events are still queued can no longer deadlock (previously the join was performed while holding the mutex that the callback thread needs to drain its queue); - the message queue (device references and markers) is drained before the hotplug libusb context is destroyed, and undelivered replay snapshots are freed on deregistration and `hid_exit()`. - **Registration is thread-safe end-to-end:** concurrent registrations no longer race in the implicit `hid_init()` or in the machinery initialization (guarded by a statically-initialized mutex). ### Verification - `gcc -std=c99 -fsyntax-only -Wall -Wextra -pedantic -Werror` and `g++ -x c++ -Wall -Wextra -Werror` both clean; full CMake build with `HIDAPI_WITH_LIBUSB=TRUE` and tests passes. - A smoke test (registration/deregistration, argument validation and error strings, `ENUMERATE` with no devices, no synchronous invocation from register, register/deregister churn, `hid_exit()` teardown with a live registration, re-init after `hid_exit()`) passes cleanly under AddressSanitizer/UBSan (no leaks) and ThreadSanitizer (no warnings), including a 4-thread concurrent register/deregister stress. - Live device plug/unplug was not exercised (no USB passthrough available in the build environment). 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** - **An arriving device is now delivered exactly once, even to a callback registered from within another callback.** The arrival path invoked the callbacks for each interface *before* appending the new entries to the device cache, and recomputed the "do not deliver to callbacks registered during this dispatch" boundary per interface. A callback registered from inside another callback during a live arrival therefore snapshotted a cache that did not yet contain the arriving device *and* was excluded from the in-flight dispatch, so the device reached it through neither path (and was later reported as LEFT without a preceding ARRIVED); for multi-interface devices the remaining interfaces were delivered twice instead. The infos are now appended to the cache before any callback runs, and the boundary is computed once per hotplug message. - **The libusb listener is armed before the initial enumeration, and arrivals are deduplicated against the cache.** Previously the snapshot was taken first: a device connecting in between was in neither the snapshot nor any live event (libusb does not back-fill without `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. - **A genuine `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** - **The global error state is serialized.** `hid_hotplug_register_callback()` / `hid_hotplug_deregister_callback()` are thread-safe by contract, but their failure paths wrote `last_global_error` outside 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_ready` is no longer read without synchronization**, and the hotplug mutex is no longer destroyed by `hid_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. - **The deferred join is awaited on a condition variable** instead of a busy `unlock`/`lock` spin, which was unfair and could livelock under a real-time scheduling policy (the spinner starving the joiner). **Lifecycle** - **Deregistering the last callback from an application thread now winds the event threads down synchronously.** It used to return while they were still winding down, so deregister + `dlclose` raced 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. - **Deregistering an already-removed callback fails again** instead of returning success a second time: a handle whose removal was postponed (an in-callback deregistration) no longer matches the lookup. - **Handle allocation refuses to overflow.** `next_handle++` at `INT_MAX` was signed-overflow UB, and the wrap-around to 1 could collide with a live handle; registration now fails with an error instead. **Failure paths** - The `ENUMERATE` snapshot 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. - A **failed replay-marker enqueue** used to be ignored, deferring the whole pass indefinitely when no live event followed; it now fails the registration too. (A dropped *live* event on enqueue failure is pre-existing and unavoidable - libusb offers no way to postpone or retry it - and is now called out in a comment.) - **Both event threads are checked at creation.** `hidapi_thread_create()` returned `void` and `threads_running` was set regardless, so a failed `pthread_create()` left a garbage `pthread_t` to 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 -Werror` and `g++ -x c++ -Wall -Wextra -Werror` clean; CMake builds with `HIDAPI_WITH_LIBUSB=TRUE`, `HIDAPI_WITH_TESTS=ON` and with `HIDAPI_BUILD_AS_CXX=ON` pass. - The smoke test was rebuilt on top of an in-process libusb replacement, so arrivals, removals, multi-interface devices, enumeration/allocation/thread-creation failures and the arm-before-snapshot window can be simulated without real hardware (no USB access is available in the build environment). 18 test cases, including nested registration during a live arrival (single- and multi-interface, asserting exactly-once delivery and no LEFT-without-ARRIVED), arrival deduplication, the double-deregister contract, the synchronous wind-down, the deferred-shutdown race, OOM injection on both snapshot paths, both thread-creation failures, handle exhaustion, `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. - Every fix above was validated with a negative control: the defect was re-introduced and the corresponding test observed to fail. - 10/10 clean runs under ASan + UBSan + LSan (no leaks, including the libusb device references) and 10/10 clean under TSan. TSan caught two real defects introduced during this round - a lock-order inversion between the init mutex and the hotplug mutex (a nested registration from a callback takes them in the opposite order) and an under-synchronized write to `shutdown_pending` - both of which are fixed above. - Live device plug/unplug on real hardware is still not exercised. --- ### Final state (after review) Final design: two internal threads (a libusb event pump + a callback thread draining a message queue), with the ENUMERATE pass delivered as a marker message in that queue; `hid_exit()` tears the hotplug machinery down and then calls `libusb_exit(usb_context)` **under** the hotplug mutex as one transaction (no resurrected context, no orphaned threads); device identity compares the bus-port prefix **including** the `:` separator (fixing a port-2-vs-port-20 mismatch that silently dropped arrivals); and internal-thread writes to the global error string are suppressed via the captured callback-thread id. `hidapi_thread_create()` now returns `int` so a `pthread_create` failure can be reported and unwound (a source change for out-of-tree thread models тАФ see #829). libusb backend: hidapi_thread_create() in custom HIDAPI_THREAD_MODEL_INCLUDE thread models must now return int (0 on success); void implementations no longer compile. **Verification.** Every fix commit on this branch was adversarially delta-verified by an independent reviewer (Codex on a separate plan, or a fresh model) tasked to find what the fix itself broke тАФ a loop that caught a defect in nearly every fix commit. A per-backend white-box harness injects fake device events into the real implementation and runs under ASan/UBSan/LSan and TSan (TSan proven live). The `hotplug-integration` branch тАФ all four final backends merged with the test suite (#826) тАФ is CI-green on every platform, with HotplugAPI_libusb executing per push and Hotplug_libusb in the label-gated libusb-vhid-test VM job. Implements `device->next == NULL` (#791, #792) and the ENUMERATE return-value (#793) contract for libusb. Part of hotplug support (#238); contract documented in #790. Follow-up: #829. Assisted-by: claude-code:claude-opus-4-8 Assisted-by: claude-code:claude-opus-5 Assisted-by: claude-code:claude-fable-5 Assisted-by: codex-cli:gpt-6-astra ## Master integration The latest `master` at `852cc68` is now merged into this branch in `bf7d6ba`. Codex resolved the `libusb/hidapi_libusb.h` conflict by preserving the hotplug error-serialization documentation alongside master's timeout declarations and formatting. The updated branch is also merged into the combined test PR #836. Validation on the updated combined tree (`932e5ac`): Linux/WSL GCC and Windows/MSVC AddressSanitizer builds passed; `HotplugAPI_hidraw`, `HotplugAPI_libusb`, and 26 Windows tests passed. Four Linux and two Windows device-backed tests self-skipped because the required virtual devices are unavailable locally. The libusb implementation, header, thread model, and CMake file are identical between this branch and the tested combined tree. A focused independent merge review found no integration issues. Related to #831, #832, #834 (the merged master changes), and #836 (combined testing). Generated by Codex (GPT-6 Astra). Assisted-by: codex:gpt-6-astra

Youw commented

2026-08-04 11:58:20 -05:00

(Migrated from github.com)

Full implementation review (fresh pass, head `64ef449`)

Traced every contract clause from hidapi/hidapi.h against the full hotplug machinery in libusb/hid.c and tried to break the design with adversarial interleavings: nested register/deregister from callbacks, deferred shutdown initiated on the event thread, register parked across a concurrent hid_exit, marker vs. live-event queue orderings, wrong-waiter condvar consumption, double-free of replay snapshots, and refcount balance of queued libusb devices. The locking discipline (recursive mutex + callback-thread mutex, mutex_in_use/cb_list_dirty postponement, join_claimed/shutdown_pending wind-down protocol) holds up in every scenario constructible; exactly-once/pass-before-live, handle non-reuse, deregister-blocks semantics, global-error suppression on the callback thread, and hid_exit teardown ordering are all correctly implemented. Findings: 0 blocker, 0 major, 1 minor, 1 nit.

[minor] `libusb/hid.c:997-1006` — `hid_exit`'s "never initialized" fast path can race a concurrent first-ever registration on `usb_context`

hid_internal_hotplug_exit() samples mutex_ready under hid_hotplug_init_mutex (979-993) and, when 0, destroys usb_context with no lock held (1002-1005), justified by "a registration is the only path that would [race us], and it would have initialized the machinery (and taken mutex) first". That ordering is not enforced: a first-ever hid_hotplug_register_callback() on thread B may be past nothing at all when hid_exit on thread A samples mutex_ready == 0; B then sets mutex_ready, takes mutex, and its hid_init() (2070) plus hid_internal_enumerate() → libusb_get_device_list(usb_context, …) (1492 via 2126) read usb_context while A concurrently runs libusb_exit(usb_context); usb_context = NULL; — an unsynchronized read/write and potential use of a destroyed context. This contradicts the transaction guarantee the PR itself documents at 1032-1039 and 2057-2069 — the protection only engages once the machinery has ever been initialized. Mitigating: hidapi.h puts hid_init/hid_exit serialization on the application, so this is outside the strict contract — hence minor.

Suggested fix: replace the hid_internal_hotplug_lock() < 0 early-return path with an unconditional hid_internal_hotplug_init_and_lock(); the existing common path then handles the never-used case naturally (empty list, no threads, destroy usb_context under mutex). Alternatively keep the fast path but rewrite the comment at 998-1001 to state the residual race and that it is covered by the application's hid_exit serialization duty.

[nit] `libusb/hid.c:1858` — typo in comment

"the clenup function" → "the cleanup function" (the line was rewritten in this PR's region).

Verified sound (checked, no issues): exactly-once/ordering (listener armed before snapshot with devs-based dedupe; per-callback replay flushed before any live delivery; per-message last boundary excluding nested registrations; devs updated before invoking callbacks on both ARRIVED and LEFT so nested ENUMERATE snapshots are consistent); non-zero-return and deregister-from-callback paths free of double-frees (flush_replay detaches before invoking; postponed deregister frees and NULLs replay); queue/device refcount balance on every path including both thread-creation failure unwinds; no lost wakeups (the shared condvar's two waiter classes have mutually exclusive predicates; all shutdown_pending transitions broadcast); hid_exit teardown order once initialized (callbacks unlinked under mutex, listener deregistered and queue drained on the pump thread before libusb_exit, pump joins drainer, finish_shutdown joins pump, usb_context destroyed under mutex; re-init works, next_handle monotonic across generations); match_libusb_to_info prefix keeps the ':' and matches the real get_path format, with the len <= 0 case safely matching nothing; hidapi_thread_create's int return checked at all three call sites with correct no-join-on-failure unwind.

Known, deliberately documented limitations (not defects): an OOM-dropped LEFT message can hide the next same-port arrival (1733-1743); register/deregister failures from within a callback return −1 but leave the global error string untouched by design (169-185); hid_libusb_error(NULL)'s unlocked int read predates this PR.

Drafted with Claude Code.

## Full implementation review (fresh pass, head `64ef449`) Traced every contract clause from `hidapi/hidapi.h` against the full hotplug machinery in `libusb/hid.c` and tried to break the design with adversarial interleavings: nested register/deregister from callbacks, deferred shutdown initiated on the event thread, register parked across a concurrent `hid_exit`, marker vs. live-event queue orderings, wrong-waiter condvar consumption, double-free of replay snapshots, and refcount balance of queued libusb devices. The locking discipline (recursive `mutex` + callback-thread mutex, `mutex_in_use`/`cb_list_dirty` postponement, `join_claimed`/`shutdown_pending` wind-down protocol) holds up in every scenario constructible; exactly-once/pass-before-live, handle non-reuse, deregister-blocks semantics, global-error suppression on the callback thread, and `hid_exit` teardown ordering are all correctly implemented. **Findings: 0 blocker, 0 major, 1 minor, 1 nit.** ### [minor] `libusb/hid.c:997-1006` — `hid_exit`'s "never initialized" fast path can race a concurrent first-ever registration on `usb_context` `hid_internal_hotplug_exit()` samples `mutex_ready` under `hid_hotplug_init_mutex` (979-993) and, when 0, destroys `usb_context` with **no lock held** (1002-1005), justified by "a registration is the only path that would [race us], and it would have initialized the machinery (and taken `mutex`) first". That ordering is not enforced: a first-ever `hid_hotplug_register_callback()` on thread B may be past nothing at all when `hid_exit` on thread A samples `mutex_ready == 0`; B then sets `mutex_ready`, takes `mutex`, and its `hid_init()` (2070) plus `hid_internal_enumerate()` → `libusb_get_device_list(usb_context, …)` (1492 via 2126) read `usb_context` while A concurrently runs `libusb_exit(usb_context); usb_context = NULL;` — an unsynchronized read/write and potential use of a destroyed context. This contradicts the transaction guarantee the PR itself documents at 1032-1039 and 2057-2069 — the protection only engages once the machinery has ever been initialized. Mitigating: `hidapi.h` puts `hid_init`/`hid_exit` serialization on the application, so this is outside the strict contract — hence minor. **Suggested fix:** replace the `hid_internal_hotplug_lock() < 0` early-return path with an unconditional `hid_internal_hotplug_init_and_lock()`; the existing common path then handles the never-used case naturally (empty list, no threads, destroy `usb_context` under `mutex`). Alternatively keep the fast path but rewrite the comment at 998-1001 to state the residual race and that it is covered by the application's `hid_exit` serialization duty. ### [nit] `libusb/hid.c:1858` — typo in comment "the clenup function" → "the cleanup function" (the line was rewritten in this PR's region). --- Verified sound (checked, no issues): exactly-once/ordering (listener armed before snapshot with `devs`-based dedupe; per-callback replay flushed before any live delivery; per-message `last` boundary excluding nested registrations; `devs` updated before invoking callbacks on both ARRIVED and LEFT so nested ENUMERATE snapshots are consistent); non-zero-return and deregister-from-callback paths free of double-frees (`flush_replay` detaches before invoking; postponed deregister frees and NULLs `replay`); queue/device refcount balance on every path including both thread-creation failure unwinds; no lost wakeups (the shared condvar's two waiter classes have mutually exclusive predicates; all `shutdown_pending` transitions broadcast); `hid_exit` teardown order once initialized (callbacks unlinked under `mutex`, listener deregistered and queue drained on the pump thread before `libusb_exit`, pump joins drainer, `finish_shutdown` joins pump, `usb_context` destroyed under `mutex`; re-init works, `next_handle` monotonic across generations); `match_libusb_to_info` prefix keeps the ':' and matches the real `get_path` format, with the `len <= 0` case safely matching nothing; `hidapi_thread_create`'s int return checked at all three call sites with correct no-join-on-failure unwind. Known, deliberately documented limitations (not defects): an OOM-dropped LEFT message can hide the next same-port arrival (1733-1743); register/deregister failures from within a callback return −1 but leave the global error string untouched by design (169-185); `hid_libusb_error(NULL)`'s unlocked int read predates this PR. _Drafted with Claude Code._

Youw commented

2026-08-04 12:05:47 -05:00

(Migrated from github.com)

Re-review of fix commit `8b87914`

A fresh delta review (independent reviewer) verified the fix against both findings and hunted for newly introduced defects:

hid_exit fast-path race — FIXED. Both hid_internal_hotplug_exit() and a first-ever hid_hotplug_register_callback() now enter via hid_internal_hotplug_init_and_lock(), so exit and registration serialize on mutex, and usb_context is read/destroyed only with mutex held. Both interleavings were walked: exit-first leaves a no-op wind-down (empty list, wait_shutdown returns immediately on shutdown_pending==0) and the subsequent registration's hid_init() re-creates usb_context under the same mutex; register-first has exit remove the callback and run the normal wind-down before destroying the context. Lock ordering (init-mutex released before mutex, leaf rule) and unlock balance were verified; hid_internal_hotplug_lock remains used by deregister, so no unused-function warning.
Typo — fixed.
New defects: none found. The commit strictly adds locking to a previously lock-free path; no synchronization removed, no wind-down logic changed.

Two pre-existing observations surfaced while verifying (unchanged by this commit, noted for completeness): the macOS pthread_barrier_init fallback's ignored return predates the commit and matches the file's uniform pattern, and during exit's wait_shutdown window a concurrent registration can start a new generation before exit returns — the app-serialization caveat hidapi.h already places on hid_exit.

PR #825 is review-clean from my side.

Drafted with Claude Code.

## Re-review of fix commit `8b87914` A fresh delta review (independent reviewer) verified the fix against both findings and hunted for newly introduced defects: - **`hid_exit` fast-path race — FIXED.** Both `hid_internal_hotplug_exit()` and a first-ever `hid_hotplug_register_callback()` now enter via `hid_internal_hotplug_init_and_lock()`, so exit and registration serialize on `mutex`, and `usb_context` is read/destroyed only with `mutex` held. Both interleavings were walked: exit-first leaves a no-op wind-down (empty list, `wait_shutdown` returns immediately on `shutdown_pending==0`) and the subsequent registration's `hid_init()` re-creates `usb_context` under the same mutex; register-first has exit remove the callback and run the normal wind-down before destroying the context. Lock ordering (init-mutex released before `mutex`, leaf rule) and unlock balance were verified; `hid_internal_hotplug_lock` remains used by deregister, so no unused-function warning. - **Typo — fixed.** - **New defects: none found.** The commit strictly adds locking to a previously lock-free path; no synchronization removed, no wind-down logic changed. Two pre-existing observations surfaced while verifying (unchanged by this commit, noted for completeness): the macOS `pthread_barrier_init` fallback's ignored return predates the commit and matches the file's uniform pattern, and during exit's `wait_shutdown` window a concurrent registration can start a new generation before exit returns — the app-serialization caveat `hidapi.h` already places on `hid_exit`. PR #825 is review-clean from my side. _Drafted with Claude Code._

Youw commented

2026-09-08 15:49:59 -05:00

(Migrated from github.com)

Fresh review pass (2026-09-08)

Two independent reviews (Claude Fable 5.1 and gpt-6-astra), merged and deduplicated: 21 findings (0 major, 4 minor, 9 nit, 8 note).

Fixes landed in four commits:

e33a98f - shutdown generations, referenced libusb_device identities, atomic failure of enumeration and the hotplug snapshot
1707c33 - descriptor out-of-memory unwound atomically, the pump destroys the context on self-removal, dropped-removal recovery via cache_stale reconciliation
750e041 - arrivals queued behind a reconciliation are still delivered
8976d38 - retired identities freed once a real LEFT proves no arrival can follow, reconcile at every registration (not only ENUMERATE|ARRIVED), cache_stale cleared before each reconcile

Final adversarial verification (Fable, on 750e041): 15 fixed, 1 skipped (contract-forbidden trigger), 8 wording items not re-checked by design. The three notes it raised are round 4 above.

CI: green on 8976d38; on the aggregate branch (#830) the Linux libusb virtual-device job passes DeviceIO, HotplugAPI and Hotplug.

Drafted with Claude Code.

## Fresh review pass (2026-09-08) Two independent reviews (Claude Fable 5.1 and gpt-6-astra), merged and deduplicated: **21 findings** (0 major, 4 minor, 9 nit, 8 note). Fixes landed in four commits: - e33a98f - shutdown generations, referenced `libusb_device` identities, atomic failure of enumeration and the hotplug snapshot - 1707c33 - descriptor out-of-memory unwound atomically, the pump destroys the context on self-removal, dropped-removal recovery via `cache_stale` reconciliation - 750e041 - arrivals queued behind a reconciliation are still delivered - 8976d38 - retired identities freed once a real LEFT proves no arrival can follow, reconcile at every registration (not only ENUMERATE|ARRIVED), `cache_stale` cleared before each reconcile Final adversarial verification (Fable, on 750e041): 15 fixed, 1 skipped (contract-forbidden trigger), 8 wording items not re-checked by design. The three notes it raised are round 4 above. CI: green on 8976d38; on the aggregate branch (#830) the Linux libusb virtual-device job passes DeviceIO, HotplugAPI and Hotplug. _Drafted with Claude Code._

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin 'hotplug-async-enumerate-libusb:hotplug-async-enumerate-libusb'

git switch 'hotplug-async-enumerate-libusb'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'connection-callback'

git merge --no-ff 'hotplug-async-enumerate-libusb'

git switch 'hotplug-async-enumerate-libusb'

git rebase 'connection-callback'

git switch 'connection-callback'

git merge --ff-only 'hotplug-async-enumerate-libusb'

git switch 'hotplug-async-enumerate-libusb'

git rebase 'connection-callback'

git switch 'connection-callback'

git merge --no-ff 'hotplug-async-enumerate-libusb'

git switch 'connection-callback'

git merge --squash 'hotplug-async-enumerate-libusb'