gh-libusb/hidapi

Watch

Fork 0

windows: deliver hotplug ENUMERATE pass asynchronously on the event context #823

Open

Youw wants to merge 13 commits from hotplug-async-enumerate-windows into connection-callback

Youw commented

2026-07-13 17:40:50 -05:00

(Migrated from github.com)

Implements the hotplug contract documented via #790 for the windows backend (windows/hid.c only).

What changed

Asynchronous ENUMERATE pass. hid_hotplug_register_callback no longer runs the HID_API_HOTPLUG_ENUMERATE pass synchronously on the calling thread. Instead it takes a deep-copied snapshot of the matching entries of the cached device list (under the hotplug critical section, only when the flag is set and events includes DEVICE_ARRIVED) into a per-callback replay list, and submits a threadpool work item (CreateThreadpoolWork/SubmitThreadpoolWork) that delivers it on the same critical-section-serialized context that delivers live CM_Register_Notification events. The pass never fires from within register itself and never on an application thread.
Exactly once, pass-before-live. Live dispatch flushes a callback's pending replay before delivering any live event to it, and is bounded to the callbacks present at event time, so a callback registered from inside another callback picks the in-flight device up from its snapshot rather than from the live loop (previously it could see it twice). device->next is now NULL for every invocation, including the synthetic ones (the old sync pass handed out devices still linked into the cached list).
Callback return value honored during the pass (#793, windows): a non-zero return stops the remainder of the pass, frees the undelivered snapshot and deregisters the callback.
Deregister return value: 0 only when the handle was found and deregistered; -1 for unknown, stale or already-self-deregistered handles (safe no-op).
Error strings: all register/deregister failure paths (including argument validation) set the global error retrievable via hid_error(NULL); *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)
Teardown safety: CM_Unregister_Notification is documented to deadlock when called from its own notification callback, and it also deadlocks if called while holding a lock the notification callback takes. The teardown now detaches the notification handle under the critical section and unregisters outside it; when the last callback removes itself from inside the notification callback, the actual teardown is deferred to the threadpool work item. hid_exit frees undelivered snapshots, waits for in-flight work items (WaitForThreadpoolWorkCallbacks) before deleting the critical section, and sweeps anything a last-gasp notification appended to the device cache.

Design notes

The work item is a single reusable PTP_WORK created with the first callback registration and closed by hid_exit. I used CreateThreadpoolWork rather than TrySubmitThreadpoolCallback because teardown needs WaitForThreadpoolWorkCallbacks to guarantee no work item can touch the critical section after hid_exit deletes it; the fire-and-forget API offers no way to wait.
Replay flushing is idempotent and runs under the critical section with mutex_in_use set: whichever of the work item or a live event acquires the lock first delivers the pass, the other finds replay == NULL.
Registration reuses an already-registered CM notification when a deferred teardown has not run yet (the existing device cache stays in place - it is kept current by that notification; a rebuild happens only when a notification is newly armed), instead of failing as the old code did.
The threadpool API needs Vista-level declarations, so the file now raises _WIN32_WINNT to 0x0600 when it is set lower (the dynamically-resolved CM notification API already requires Windows 8 at runtime).

Addresses #793 for the windows backend.

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

Review round 1

Reviewed independently by two reviewers against the #790 contract; all confirmed findings are addressed in a3350c4:

Enumerate-before-subscribe blind window (blocker) — the CM notification is now armed before the registration-time snapshot, and arrivals are deduplicated by path in the notification callback, so a device connecting between the two is caught by the notification and absorbed by the dedupe instead of being reported by neither.
Detached-but-live notification vs a fresh registration, and cache refresh during a deferred teardown — arming a new notification now waits for in-flight CM_Unregister_Notification calls to complete (pending counter + condition variable), and the device cache is rebuilt only when a notification is actually being armed. Both windows could previously deliver one connection twice.
hid_exit racing a concurrent deregister — the same quiescence gate is awaited before destroying state (previously it could DeleteCriticalSection / CloseThreadpoolWork while a detached notification was still live at the OS). A failed CM_Unregister_Notification now poisons the epoch: hid_exit reports the failure and deliberately leaks the critical section, work item and DLL handles rather than let a live notification touch destroyed state.
Initialization and error-reporting races — the machinery bootstrap is guarded by a statically-initialized SRWLOCK (two racing first registrations could both initialize the critical section), and all mutations of the global error string are serialized by another SRWLOCK (concurrent failures in the documented-thread-safe register/deregister could double-free it).
Enumeration failure vs empty system — the two are now distinguished (registration fails on genuine failure), and the stale "No HID devices found" error is cleared on successful registration.
Handle exhaustion — registration now fails with an error instead of signed-overflowing and wrapping onto live handles.
C++ cleanliness — { 0 } struct initializers replaced with memset for -Wmissing-field-initializers under GNU C++.

Review round 2

Reviewed independently by two further reviewers; all confirmed findings are addressed in 1f621c1:

Critical section lifetime (blocker) — hid_exit deleted the critical section with no lock held across the teardown, while a registration sleeping on a pending unregistration, a concurrent (contract-legal) deregistration, or hid_internal_hotplug_init could still be about to enter it. The critical section and the quiescence event are now created once and never destroyed (consistent with the leaked-notification path, which already had to keep them), and an explicit exiting state, set under the lock, makes registration and deregistration fail rather than arm a notification behind a teardown. mutex_ready is only ever read under the bootstrap lock or under the critical section itself.
hid_exit unloading the DLLs under a concurrent registration — found by the new stress test. hid_hotplug_register_callback is thread-safe and initializes the library implicitly, so it could be calling CM_Register_Notification through a function pointer while hid_exit was FreeLibrary-ing cfgmgr32.dll. exiting is now held across the unload; every other call through those pointers is either made under the critical section or accounted for by the pending-unregistration counter that the teardown waits out.
Global error string written from an internal thread — hid_error(NULL) returns the raw string pointer with no lock, so the threadpool work item reporting a failed CM_Unregister_Notification could free() it under an application thread: a use-after-free the application cannot serialize away. HIDAPI's internal event context no longer writes the global error at all; the failure is recorded in internal state and reported by the next hid_exit. The writer-side lock (round 1) is kept for the application-thread writers.
Poisoned epoch was permanent and re-armable — the failure is now scoped to the hid_exit that reports it (a single failed unregistration no longer makes every later hid_exit return -1), while the safety consequences stay for the process: the state a live notification can reach is never destroyed, the DLLs it calls into are never unloaded, the module that contains the callback is pinned (GetModuleHandleEx), and no second notification is ever armed next to it — registration fails with an explicit error, because two live registrations would deliver every event twice.
Path dedupe was permanent — an interface path is reused when a device is replugged into the same port, so deduplicating every arrival against the cache could swallow a genuine new connection. The dedupe now covers only the window it exists for (the overlap between arming the notification and taking the registration-time snapshot): the enumerated paths are recorded, each swallows at most one arrival, and a path is dropped as soon as its device leaves. Outside that window an arrival for a cached path is a new connection and replaces the stale record instead of being shadowed by it. (superseded, see Final state)
OOM handling in the enumeration — a per-device allocation failure was treated as a successful (but silently short) enumeration, so a registration could succeed with devices missing from the pass; it now goes through the failure channel. A hid_device_info is never handed out half-built, so the hotplug cache can no longer hold a record with a NULL path — which the removal walk dereferenced — and the bus-specific fixups (hid_internal_get_usb_info/_get_ble_info, which wcslen() the strings) can no longer read a NULL.
_WIN32_WINNT force-bump removed (supersedes the round-1 design note) — it turned the threadpool, SRWLOCK and condition-variable calls into load-time Vista-only imports of hidapi.dll for every user, including those that never touch hotplug, and it contradicted the file's LoadLibrary/GetProcAddress discipline. The four threadpool entry points are now resolved from kernel32 like everything else (hotplug registration fails with an error if they are unavailable), and the two Vista-only synchronisation primitives were replaced with equivalents that need no initialization (the quiescence wait is a manual-reset event; the two tiny writer locks are interlocked locks). dumpbin /imports on the resulting DLL shows no *Threadpool*, *SRWLock* or *ConditionVariable* symbols.

Verified with the CI recipes (MSVC C and C++, /W4 /WX, ASAN), the repo's ctest suite, and a throwaway white-box contract test (synthetic CM notifications + allocation fault injection) covering the async pass semantics, non-zero return, self-deregistration, register-from-callback, stale handles, hid_exit with a pending replay, re-init, argument validation, hid_exit racing register/deregister storms, replug-on-the-same-path, NULL-path cache records and 400 OOM injection points — 10x stress runs, all green.

Review round 3

A delta review of 1f621c1 (read and executed: MSVC C and C++20 /W4 /WX builds, dumpbin /imports, a DLL load/unload handle-leak harness, and a 6-thread register/deregister storm racing hid_exit) confirmed the round-2 synchronization model holds, and found one newly-introduced blocker plus several minors; all are addressed in 0ddecf2:

hid_exit leaked a kernel event and a critical section for every user (blocker, introduced by round 2) — round 2 made hid_exit bootstrap the hotplug machinery just to raise the exiting flag, and the never-destroy policy then guaranteed the event handle and the critical section were never freed: a plain hid_init/hid_enumerate/hid_exit program that never touches hotplug leaked one handle per load/use/unload cycle (measured: +100 handles over 100 cycles), contradicting hid_exit's documented purpose. The flag now lives under the statically-initialized bootstrap spinlock — which costs no OS object — so hid_exit can raise it without creating anything and return early when the machinery never existed. On top of that, every public hotplug call is now counted into the machinery under that same lock, which supplies the proof round 2 lacked: once exiting cuts off new entries, every notification is unregistered, the work item is drained and closed, and the user count is zero, nothing can be inside — or blocked on — the critical section, so the clean path of hid_exit now destroys the event and critical section it created. The leaked-notification path keeps everything alive forever, exactly as before. Measured after the fix: 0 handle growth over 100 cycles, with and without hotplug use in the loop (was +100 in both).
Stale pending-arrival records swallowed genuine re-arrivals — the registration-time dedupe records suppressed the one in-flight duplicate they exist for, but a record whose duplicate never came outlived that window; if the device's removal notification was then missed, its genuine re-arrival was consumed by its own stale record, and the stale-record recovery could never run for exactly the devices it targets. The list now expires (10 s, far beyond any notification delivery latency): the failure modes are asymmetric in the right direction — an over-aged duplicate would produce a spurious left+arrived pair instead of a silently dropped connection.
Stale-record replacement owed a DEVICE_LEFT — replacing a stale cache record on a same-path arrival reported the new connection but silently discarded the previous one, although the header promises a "left" event for every matching device that disconnects while a callback is registered. The live dispatch loop is factored out (hid_internal_hotplug_dispatch) and the stale record is now delivered as a synthetic DEVICE_LEFT before the new device is cached, preserving exactly-once for callbacks registered from within a callback. (superseded, see Final state)
Spinlock discipline — the global error message is now built and freed outside the internal spinlock (only the pointer swap is guarded; previously FormatMessageW plus free/calloc ran under it), and the acquire loop escalates from Sleep(0) to Sleep(1) after a few attempts: Sleep(0) yields only to equal-or-higher-priority threads, so spinning against a lower-priority holder on a single CPU made no progress until the balance-set manager intervened.
One malformed interface path failed the whole enumeration as an OOM — hid_internal_UTF16toUTF8 returns NULL both for invalid UTF-16 (e.g. an unpaired surrogate) and on allocation failure, and round 2 escalated both to a full-enumeration failure reported as "Failed to allocate memory". The two are now distinguished: an unrepresentable device is skipped (consistently with the hotplug notifications, which can neither report nor cache it), and only genuine allocation failures abort the pass.
Also: corrected the comment on the global error lock (a user callback calling the public hotplug API from the event context does write the global error; the header's serialization requirement covers it), removed a dead failed term the sticky notification_leaked flag always implies, and documented why the registration wait loop may release the critical section unconditionally (it can never be held recursively there).

Verified with the CI recipes (MSVC C and C++20, /W4 /WX, ASAN, plus the HIDAPI_USE_DDK compile), dumpbin /imports (still no *Threadpool*/*SRWLock*/*ConditionVariable* imports; the only new kernel32 import is GetTickCount), the handle-leak harness above, and the white-box contract test extended with the new cases (pending-arrival expiry, missed-removal recovery delivering left+arrived, invalid-UTF-16 vs OOM) — including 3x runs of the 8-second 6-thread hid_exit race, all green.

Final state (after review) — supersedes parts of the description above

The pending-arrival / wall-clock-expiry / missed-removal-recovery scheme described earlier was removed. The final design: arrival de-duplication and removal are an allocation-free live-cache path comparison (a hand-rolled, surrogate-validated UTF-16↔UTF-8 compare), so an OOM on the removal path can never leave the cache stale; the four threadpool APIs are resolved dynamically via GetProcAddress (keeping the DLL's load-time OS floor at XP — no static Vista-only imports); a user-count enter/leave gate lets hid_exit() prove no thread is inside or blocked on the critical section before destroying it (0 leaked handles for programs that never touch hotplug); and internal-event-context writes to the global error string are suppressed via a thread-local flag. An arrival whose interface path is already cached is dropped entirely (it can only be the arm-before-enumerate duplicate); the stale-record replacement and the synthetic DEVICE_LEFT described in rounds 2-3 no longer exist. The GetTickCount import went away with the wall-clock expiry scheme; the final DLL adds no new kernel32 import.

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_winapi executing per push and Hotplug_winapi in the label-gated win-vhid-test job.

Addresses the Windows register_callback error-handling review (#785) and implements device->next == NULL (#791, #792) and the ENUMERATE return-value (#793) contract for winapi. Part of hotplug support (#238); contract documented in #790.

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

Implements the hotplug contract documented via #790 for the **windows** backend (`windows/hid.c` only). ## What changed - **Asynchronous ENUMERATE pass.** `hid_hotplug_register_callback` no longer runs the `HID_API_HOTPLUG_ENUMERATE` pass synchronously on the calling thread. Instead it takes a deep-copied snapshot of the matching entries of the cached device list (under the hotplug critical section, only when the flag is set and `events` includes `DEVICE_ARRIVED`) into a per-callback `replay` list, and submits a threadpool work item (`CreateThreadpoolWork`/`SubmitThreadpoolWork`) that delivers it on the same critical-section-serialized context that delivers live `CM_Register_Notification` events. The pass never fires from within `register` itself and never on an application thread. - **Exactly once, pass-before-live.** Live dispatch flushes a callback's pending replay before delivering any live event to it, and is bounded to the callbacks present at event time, so a callback registered from inside another callback picks the in-flight device up from its snapshot rather than from the live loop (previously it could see it twice). `device->next` is now `NULL` for every invocation, including the synthetic ones (the old sync pass handed out devices still linked into the cached list). - **Callback return value honored during the pass** (#793, windows): a non-zero return stops the remainder of the pass, frees the undelivered snapshot and deregisters the callback. - **Deregister return value**: `0` only when the handle was found and deregistered; `-1` for unknown, stale or already-self-deregistered handles (safe no-op). - **Error strings**: all register/deregister failure paths (including argument validation) set the global error retrievable via `hid_error(NULL)`; `*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) - **Teardown safety**: `CM_Unregister_Notification` is documented to deadlock when called from its own notification callback, and it also deadlocks if called while holding a lock the notification callback takes. The teardown now detaches the notification handle under the critical section and unregisters outside it; when the last callback removes itself from inside the notification callback, the actual teardown is deferred to the threadpool work item. `hid_exit` frees undelivered snapshots, waits for in-flight work items (`WaitForThreadpoolWorkCallbacks`) before deleting the critical section, and sweeps anything a last-gasp notification appended to the device cache. ## Design notes - The work item is a single reusable `PTP_WORK` created with the first callback registration and closed by `hid_exit`. I used `CreateThreadpoolWork` rather than `TrySubmitThreadpoolCallback` because teardown needs `WaitForThreadpoolWorkCallbacks` to guarantee no work item can touch the critical section after `hid_exit` deletes it; the fire-and-forget API offers no way to wait. - Replay flushing is idempotent and runs under the critical section with `mutex_in_use` set: whichever of the work item or a live event acquires the lock first delivers the pass, the other finds `replay == NULL`. - Registration reuses an already-registered CM notification when a deferred teardown has not run yet (the existing device cache stays in place - it is kept current by that notification; a rebuild happens only when a notification is newly armed), instead of failing as the old code did. - The threadpool API needs Vista-level declarations, so the file now raises `_WIN32_WINNT` to `0x0600` when it is set lower (the dynamically-resolved CM notification API already requires Windows 8 at runtime). Addresses #793 for the windows backend. Assisted-by: claude-code:claude-fable-5 Assisted-by: claude-code:claude-opus-5 Assisted-by: codex-cli:gpt-6-astra ## Review round 1 Reviewed independently by two reviewers against the #790 contract; all confirmed findings are addressed in `a3350c4`: - **Enumerate-before-subscribe blind window** (blocker) — the CM notification is now armed *before* the registration-time snapshot, and arrivals are deduplicated by path in the notification callback, so a device connecting between the two is caught by the notification and absorbed by the dedupe instead of being reported by neither. - **Detached-but-live notification vs a fresh registration**, and **cache refresh during a deferred teardown** — arming a new notification now waits for in-flight `CM_Unregister_Notification` calls to complete (pending counter + condition variable), and the device cache is rebuilt only when a notification is actually being armed. Both windows could previously deliver one connection twice. - **`hid_exit` racing a concurrent deregister** — the same quiescence gate is awaited before destroying state (previously it could `DeleteCriticalSection` / `CloseThreadpoolWork` while a detached notification was still live at the OS). A *failed* `CM_Unregister_Notification` now poisons the epoch: `hid_exit` reports the failure and deliberately leaks the critical section, work item and DLL handles rather than let a live notification touch destroyed state. - **Initialization and error-reporting races** — the machinery bootstrap is guarded by a statically-initialized `SRWLOCK` (two racing first registrations could both initialize the critical section), and all mutations of the global error string are serialized by another `SRWLOCK` (concurrent failures in the documented-thread-safe register/deregister could double-free it). - **Enumeration failure vs empty system** — the two are now distinguished (registration fails on genuine failure), and the stale "No HID devices found" error is cleared on successful registration. - **Handle exhaustion** — registration now fails with an error instead of signed-overflowing and wrapping onto live handles. - **C++ cleanliness** — `{ 0 }` struct initializers replaced with `memset` for `-Wmissing-field-initializers` under GNU C++. ## Review round 2 Reviewed independently by two further reviewers; all confirmed findings are addressed in `1f621c1`: - **Critical section lifetime** (blocker) — `hid_exit` deleted the critical section with no lock held across the teardown, while a registration sleeping on a pending unregistration, a concurrent (contract-legal) deregistration, or `hid_internal_hotplug_init` could still be about to enter it. The critical section and the quiescence event are now created once and **never destroyed** (consistent with the leaked-notification path, which already had to keep them), and an explicit `exiting` state, set under the lock, makes registration and deregistration fail rather than arm a notification behind a teardown. `mutex_ready` is only ever read under the bootstrap lock or under the critical section itself. - **`hid_exit` unloading the DLLs under a concurrent registration** — found by the new stress test. `hid_hotplug_register_callback` is thread-safe and initializes the library implicitly, so it could be calling `CM_Register_Notification` through a function pointer while `hid_exit` was `FreeLibrary`-ing `cfgmgr32.dll`. `exiting` is now held across the unload; every other call through those pointers is either made under the critical section or accounted for by the pending-unregistration counter that the teardown waits out. - **Global error string written from an internal thread** — `hid_error(NULL)` returns the raw string pointer with no lock, so the threadpool work item reporting a failed `CM_Unregister_Notification` could `free()` it under an application thread: a use-after-free the application cannot serialize away. HIDAPI's internal event context no longer writes the global error at all; the failure is recorded in internal state and reported by the next `hid_exit`. The writer-side lock (round 1) is kept for the application-thread writers. - **Poisoned epoch was permanent and re-armable** — the failure is now scoped to the `hid_exit` that reports it (a single failed unregistration no longer makes every later `hid_exit` return -1), while the safety consequences stay for the process: the state a live notification can reach is never destroyed, the DLLs it calls into are never unloaded, the module that contains the callback is **pinned** (`GetModuleHandleEx`), and **no second notification is ever armed** next to it — registration fails with an explicit error, because two live registrations would deliver every event twice. - **Path dedupe was permanent** — an interface path is reused when a device is replugged into the same port, so deduplicating every arrival against the cache could swallow a genuine new connection. The dedupe now covers only the window it exists for (the overlap between arming the notification and taking the registration-time snapshot): the enumerated paths are recorded, each swallows at most one arrival, and a path is dropped as soon as its device leaves. Outside that window an arrival for a cached path is a new connection and replaces the stale record instead of being shadowed by it. (superseded, see Final state) - **OOM handling in the enumeration** — a per-device allocation failure was treated as a successful (but silently short) enumeration, so a registration could succeed with devices missing from the pass; it now goes through the failure channel. A `hid_device_info` is never handed out half-built, so the hotplug cache can no longer hold a record with a `NULL` path — which the removal walk dereferenced — and the bus-specific fixups (`hid_internal_get_usb_info`/`_get_ble_info`, which `wcslen()` the strings) can no longer read a `NULL`. - **`_WIN32_WINNT` force-bump removed** (supersedes the round-1 design note) — it turned the threadpool, `SRWLOCK` and condition-variable calls into load-time Vista-only imports of `hidapi.dll` for *every* user, including those that never touch hotplug, and it contradicted the file's `LoadLibrary`/`GetProcAddress` discipline. The four threadpool entry points are now resolved from `kernel32` like everything else (hotplug registration fails with an error if they are unavailable), and the two Vista-only synchronisation primitives were replaced with equivalents that need no initialization (the quiescence wait is a manual-reset event; the two tiny writer locks are interlocked locks). `dumpbin /imports` on the resulting DLL shows no `*Threadpool*`, `*SRWLock*` or `*ConditionVariable*` symbols. Verified with the CI recipes (MSVC C and C++, `/W4 /WX`, ASAN), the repo's ctest suite, and a throwaway white-box contract test (synthetic CM notifications + allocation fault injection) covering the async pass semantics, non-zero return, self-deregistration, register-from-callback, stale handles, `hid_exit` with a pending replay, re-init, argument validation, `hid_exit` racing register/deregister storms, replug-on-the-same-path, `NULL`-path cache records and 400 OOM injection points — 10x stress runs, all green. ## Review round 3 A delta review of `1f621c1` (read **and executed**: MSVC C and C++20 `/W4 /WX` builds, `dumpbin /imports`, a DLL load/unload handle-leak harness, and a 6-thread register/deregister storm racing `hid_exit`) confirmed the round-2 synchronization model holds, and found one newly-introduced blocker plus several minors; all are addressed in `0ddecf2`: - **`hid_exit` leaked a kernel event and a critical section for every user** (blocker, introduced by round 2) — round 2 made `hid_exit` bootstrap the hotplug machinery just to raise the `exiting` flag, and the never-destroy policy then guaranteed the event handle and the critical section were never freed: a plain `hid_init`/`hid_enumerate`/`hid_exit` program that never touches hotplug leaked one handle per load/use/unload cycle (measured: **+100 handles over 100 cycles**), contradicting `hid_exit`'s documented purpose. The flag now lives under the statically-initialized bootstrap spinlock — which costs no OS object — so `hid_exit` can raise it without creating anything and return early when the machinery never existed. On top of that, every public hotplug call is now *counted into* the machinery under that same lock, which supplies the proof round 2 lacked: once `exiting` cuts off new entries, every notification is unregistered, the work item is drained and closed, and the user count is zero, nothing can be inside — or blocked on — the critical section, so the clean path of `hid_exit` now **destroys** the event and critical section it created. The leaked-notification path keeps everything alive forever, exactly as before. Measured after the fix: **0 handle growth over 100 cycles, with and without hotplug use in the loop** (was +100 in both). - **Stale pending-arrival records swallowed genuine re-arrivals** — the registration-time dedupe records suppressed the one in-flight duplicate they exist for, but a record whose duplicate never came outlived that window; if the device's *removal* notification was then missed, its genuine re-arrival was consumed by its own stale record, and the stale-record recovery could never run for exactly the devices it targets. The list now expires (10 s, far beyond any notification delivery latency): the failure modes are asymmetric in the right direction — an over-aged duplicate would produce a spurious left+arrived pair instead of a silently dropped connection. - **Stale-record replacement owed a `DEVICE_LEFT`** — replacing a stale cache record on a same-path arrival reported the new connection but silently discarded the previous one, although the header promises a "left" event for every matching device that disconnects while a callback is registered. The live dispatch loop is factored out (`hid_internal_hotplug_dispatch`) and the stale record is now delivered as a synthetic `DEVICE_LEFT` before the new device is cached, preserving exactly-once for callbacks registered from within a callback. (superseded, see Final state) - **Spinlock discipline** — the global error message is now built and freed *outside* the internal spinlock (only the pointer swap is guarded; previously `FormatMessageW` plus `free`/`calloc` ran under it), and the acquire loop escalates from `Sleep(0)` to `Sleep(1)` after a few attempts: `Sleep(0)` yields only to equal-or-higher-priority threads, so spinning against a lower-priority holder on a single CPU made no progress until the balance-set manager intervened. - **One malformed interface path failed the whole enumeration as an OOM** — `hid_internal_UTF16toUTF8` returns `NULL` both for invalid UTF-16 (e.g. an unpaired surrogate) and on allocation failure, and round 2 escalated both to a full-enumeration failure reported as "Failed to allocate memory". The two are now distinguished: an unrepresentable device is skipped (consistently with the hotplug notifications, which can neither report nor cache it), and only genuine allocation failures abort the pass. - Also: corrected the comment on the global error lock (a user callback calling the public hotplug API from the event context *does* write the global error; the header's serialization requirement covers it), removed a dead `failed` term the sticky `notification_leaked` flag always implies, and documented why the registration wait loop may release the critical section unconditionally (it can never be held recursively there). Verified with the CI recipes (MSVC C and C++20, `/W4 /WX`, ASAN, plus the `HIDAPI_USE_DDK` compile), `dumpbin /imports` (still no `*Threadpool*`/`*SRWLock*`/`*ConditionVariable*` imports; the only new kernel32 import is `GetTickCount`), the handle-leak harness above, and the white-box contract test extended with the new cases (pending-arrival expiry, missed-removal recovery delivering left+arrived, invalid-UTF-16 vs OOM) — including 3x runs of the 8-second 6-thread `hid_exit` race, all green. --- ### Final state (after review) — supersedes parts of the description above The pending-arrival / wall-clock-expiry / missed-removal-recovery scheme described earlier was **removed**. The final design: arrival de-duplication and removal are an **allocation-free** live-cache path comparison (a hand-rolled, surrogate-validated UTF-16↔UTF-8 compare), so an OOM on the removal path can never leave the cache stale; the four threadpool APIs are resolved dynamically via `GetProcAddress` (keeping the DLL's load-time OS floor at XP — no static Vista-only imports); a user-count enter/leave gate lets `hid_exit()` prove no thread is inside or blocked on the critical section before destroying it (0 leaked handles for programs that never touch hotplug); and internal-event-context writes to the global error string are suppressed via a thread-local flag. An arrival whose interface path is already cached is dropped entirely (it can only be the arm-before-enumerate duplicate); the stale-record replacement and the synthetic DEVICE_LEFT described in rounds 2-3 no longer exist. The GetTickCount import went away with the wall-clock expiry scheme; the final DLL adds no new kernel32 import. **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_winapi executing per push and Hotplug_winapi in the label-gated win-vhid-test job. Addresses the Windows `register_callback` error-handling review (#785) and implements `device->next == NULL` (#791, #792) and the ENUMERATE return-value (#793) contract for winapi. Part of hotplug support (#238); contract documented in #790. Assisted-by: claude-code:claude-opus-4-8

Youw commented

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

(Migrated from github.com)

Full implementation review (fresh pass, head `9c0b299`)

Examined all hunks of the PR delta plus the complete current state of the hotplug machinery (bootstrap/teardown, notify callback, work item, register/deregister, error paths) against the contract in hidapi/hidapi.h. On Win8+ I could not break any contractual guarantee: the snapshot/live XOR holds through the arm-before-snapshot window, pass-before-live is enforced by flushing replay inside every dispatch, deregister blocks on in-flight invocations, non-zero return marks-and-frees with later deregister → -1, handles never reused, the event context never writes the global error string, lock ordering is acyclic, CM_Unregister_Notification is always called outside the CS, and hid_exit drains notifications/work/users before destroying anything, with a sound sticky-leak path. Findings: 2 major (both platform-floor issues, one pre-existing on the target branch), 1 minor, 1 nit.

[major] `windows/hid.c:182-189` — Win8-only CM notification functions resolved in the mandatory `lookup_functions`, breaking all of hidapi on Windows 7/Vista

RESOLVE(cfgmgr32_lib_handle, CM_Register_Notification) / CM_Unregister_Notification use the shared RESOLVE macro whose failure is goto err → free_library_handles(); return -1, and hid_init() fails when lookup_functions() fails (748-760). CM_Register_Notification is Windows 8+; on Windows 7/Vista (where master's hidapi works — everything else resolved there is Vista-or-older) hid_init() now fails wholesale, so enumeration/open are lost, not just hotplug. This contradicts the file's own design narrative (220-229, 434-444: everything above the minimum target is resolved dynamically so non-hotplug users are unaffected) — the dynamic-resolution fallback exists only for the threadpool piece. Pre-existing on the target branch (this PR's diff does not touch lookup_functions), but squarely part of the pre-Win8 behavior this backend claims.

Suggested fix: move the two CM notification functions out of lookup_functions into an optional lazy resolver mirroring hid_internal_hotplug_resolve_threadpool (called under the CS on first registration), and fail only hid_hotplug_register_callback with "Hotplug is not supported" when they are absent.

[major] `windows/hid.c:479-484` — implicit TLS (`__declspec(thread)`) breaks the "loads and fails gracefully on XP" goal for MSVC-built DLLs loaded via LoadLibrary

hid_in_hotplug_callback is __declspec(thread) under MSVC and is read in register_global_error_message (515) — i.e. on every global-error path, including hid_init()'s very first register_global_error(NULL) (750), so it executes in programs that never touch hotplug. Pre-Vista, the loader does not process the TLS directory for dynamically loaded DLLs, so accessing an implicit-TLS variable in a LoadLibrary'd DLL on XP indexes an unallocated/foreign slot of ThreadLocalStoragePointer — silent memory corruption or a crash. The rest of the PR spends real engineering (custom spinlock instead of SRWLOCK; GetProcAddress'd threadpool) precisely to keep the DLL loadable-and-gracefully-failing on pre-Vista, and this variable silently defeats that in the most common consumption mode. (MinGW __thread emutls is unaffected; Vista+ is unaffected.)

Suggested fix: dynamic TLS for MSVC: a TlsAlloc() index allocated lazily under hotplug_init_lock (treat "no index allocated" as flag==0 in register_global_error_message), with TlsSetValue around the callback invocation; or keep __declspec(thread) only when _WIN32_WINNT >= 0x0600.

[minor] `windows/hid.c:1996-2005, 903-917` — arrival dedupe assumes the OS serializes a registration's notification callbacks

An arrival whose path is already cached is dropped whole (1999-2005), justified by "the OS delivers exactly one arrival notification per interface instance, so that overlap is the only way an arrival can be a duplicate" (912-914). That holds only if the removal callback for the previous instance on the same reused path completes before the arrival callback for the new instance reaches the critical section. MSDN documents no serialization or ordering guarantee for CM_Register_Notification callbacks, and the CS is not FIFO-fair: if removal and arrival are ever dispatched concurrently and the arrival wins the CS, it is misclassified as the arm-window duplicate and dropped; the stale entry is then removed by the removal handler, so the new connection is reported neither as ARRIVED nor (at its eventual disconnect) as LEFT — violating "never neither". Could not be refuted from documentation; in practice the CM machinery appears to dispatch a registration's events sequentially, which would make the code sound.

Suggested fix: at minimum, extend the comment at 906-916 to state the per-registration serialization assumption explicitly; for robustness, funnel event processing through a FIFO (CM callback enqueues path+action, the threadpool work item processes in order).

[nit] `windows/hid.c:1980` — `hid_internal_notify_callback` has external linkage

Every other internal helper is static; this one is not, polluting the symbol namespace of static builds. (Pre-existing on the target branch.) Fix: declare it static.

Verified sound (checked, no issues): exactly-once/ordering across all constructible interleavings (arm-window duplicates, removed-after-snapshot with synthetic-ARRIVED-before-LEFT, nested register/deregister from callbacks via the last_at_event bound, re-registration reusing a still-attached notification); teardown ordering (exiting cutoff → purge → detach+unregister → quiescence → work drain → user drain → destroy; leaked-notification branch pins the module; re-init works, handles never reused); no deadlock (unregister never under the CS nor from the notify callback; register wait loop re-checks hotplug_exiting; strict CS → spinlock lock order); global-error invariant (no register_global_* reachable from the notify/work context outside user callbacks; nested public-API calls drop writes via the TLS flag with save/restore); memory safety (callback invocations and list mutations under one continuous CS hold; deregister frees only when mutex_in_use==0; LEFT payloads are cached copies with next NULLed; the allocation-free UTF-16/UTF-8 path comparison rejects unpaired surrogates and cannot overrun); OOM-hardening (registration fails rather than building a partial cache; removals allocate nothing; snapshot-copy failure tears down a would-be-first registration completely).

Drafted with Claude Code.

## Full implementation review (fresh pass, head `9c0b299`) Examined all hunks of the PR delta plus the complete current state of the hotplug machinery (bootstrap/teardown, notify callback, work item, register/deregister, error paths) against the contract in `hidapi/hidapi.h`. On Win8+ I could not break any contractual guarantee: the snapshot/live XOR holds through the arm-before-snapshot window, pass-before-live is enforced by flushing replay inside every dispatch, deregister blocks on in-flight invocations, non-zero return marks-and-frees with later deregister → -1, handles never reused, the event context never writes the global error string, lock ordering is acyclic, `CM_Unregister_Notification` is always called outside the CS, and `hid_exit` drains notifications/work/users before destroying anything, with a sound sticky-leak path. **Findings: 2 major (both platform-floor issues, one pre-existing on the target branch), 1 minor, 1 nit.** ### [major] `windows/hid.c:182-189` — Win8-only CM notification functions resolved in the mandatory `lookup_functions`, breaking all of hidapi on Windows 7/Vista `RESOLVE(cfgmgr32_lib_handle, CM_Register_Notification)` / `CM_Unregister_Notification` use the shared `RESOLVE` macro whose failure is `goto err` → `free_library_handles(); return -1`, and `hid_init()` fails when `lookup_functions()` fails (748-760). `CM_Register_Notification` is Windows 8+; on Windows 7/Vista (where master's hidapi works — everything else resolved there is Vista-or-older) `hid_init()` now fails wholesale, so enumeration/open are lost, not just hotplug. This contradicts the file's own design narrative (220-229, 434-444: everything above the minimum target is resolved dynamically so non-hotplug users are unaffected) — the dynamic-resolution fallback exists only for the threadpool piece. Pre-existing on the target branch (this PR's diff does not touch `lookup_functions`), but squarely part of the pre-Win8 behavior this backend claims. **Suggested fix:** move the two CM notification functions out of `lookup_functions` into an optional lazy resolver mirroring `hid_internal_hotplug_resolve_threadpool` (called under the CS on first registration), and fail only `hid_hotplug_register_callback` with "Hotplug is not supported" when they are absent. ### [major] `windows/hid.c:479-484` — implicit TLS (`__declspec(thread)`) breaks the "loads and fails gracefully on XP" goal for MSVC-built DLLs loaded via LoadLibrary `hid_in_hotplug_callback` is `__declspec(thread)` under MSVC and is read in `register_global_error_message` (515) — i.e. on every global-error path, including `hid_init()`'s very first `register_global_error(NULL)` (750), so it executes in programs that never touch hotplug. Pre-Vista, the loader does not process the TLS directory for dynamically loaded DLLs, so accessing an implicit-TLS variable in a LoadLibrary'd DLL on XP indexes an unallocated/foreign slot of `ThreadLocalStoragePointer` — silent memory corruption or a crash. The rest of the PR spends real engineering (custom spinlock instead of SRWLOCK; GetProcAddress'd threadpool) precisely to keep the DLL loadable-and-gracefully-failing on pre-Vista, and this variable silently defeats that in the most common consumption mode. (MinGW `__thread` emutls is unaffected; Vista+ is unaffected.) **Suggested fix:** dynamic TLS for MSVC: a `TlsAlloc()` index allocated lazily under `hotplug_init_lock` (treat "no index allocated" as flag==0 in `register_global_error_message`), with `TlsSetValue` around the callback invocation; or keep `__declspec(thread)` only when `_WIN32_WINNT >= 0x0600`. ### [minor] `windows/hid.c:1996-2005, 903-917` — arrival dedupe assumes the OS serializes a registration's notification callbacks An arrival whose path is already cached is dropped whole (1999-2005), justified by "the OS delivers exactly one arrival notification per interface instance, so that overlap is the only way an arrival can be a duplicate" (912-914). That holds only if the removal callback for the previous instance on the same reused path completes before the arrival callback for the new instance reaches the critical section. MSDN documents no serialization or ordering guarantee for `CM_Register_Notification` callbacks, and the CS is not FIFO-fair: if removal and arrival are ever dispatched concurrently and the arrival wins the CS, it is misclassified as the arm-window duplicate and dropped; the stale entry is then removed by the removal handler, so the new connection is reported neither as ARRIVED nor (at its eventual disconnect) as LEFT — violating "never neither". Could not be refuted from documentation; in practice the CM machinery appears to dispatch a registration's events sequentially, which would make the code sound. **Suggested fix:** at minimum, extend the comment at 906-916 to state the per-registration serialization assumption explicitly; for robustness, funnel event processing through a FIFO (CM callback enqueues path+action, the threadpool work item processes in order). ### [nit] `windows/hid.c:1980` — `hid_internal_notify_callback` has external linkage Every other internal helper is `static`; this one is not, polluting the symbol namespace of static builds. (Pre-existing on the target branch.) **Fix:** declare it `static`. --- Verified sound (checked, no issues): exactly-once/ordering across all constructible interleavings (arm-window duplicates, removed-after-snapshot with synthetic-ARRIVED-before-LEFT, nested register/deregister from callbacks via the `last_at_event` bound, re-registration reusing a still-attached notification); teardown ordering (exiting cutoff → purge → detach+unregister → quiescence → work drain → user drain → destroy; leaked-notification branch pins the module; re-init works, handles never reused); no deadlock (unregister never under the CS nor from the notify callback; register wait loop re-checks `hotplug_exiting`; strict CS → spinlock lock order); global-error invariant (no `register_global_*` reachable from the notify/work context outside user callbacks; nested public-API calls drop writes via the TLS flag with save/restore); memory safety (callback invocations and list mutations under one continuous CS hold; deregister frees only when `mutex_in_use==0`; LEFT payloads are cached copies with `next` NULLed; the allocation-free UTF-16/UTF-8 path comparison rejects unpaired surrogates and cannot overrun); OOM-hardening (registration fails rather than building a partial cache; removals allocate nothing; snapshot-copy failure tears down a would-be-first registration completely). _Drafted with Claude Code._

Youw commented

2026-08-04 12:18:39 -05:00

(Migrated from github.com)

Re-review of fix commit `31a799f`

A fresh delta review (independent reviewer) verified the fix against all four findings and hunted for newly introduced defects. All four FIXED; no new defects found. Windows CI (MSVC, MinGW, msbuild) is green on the commit.

Lazy CM-notification resolver — FIXED. CM_Register_Notification is called only inside the bootstrap block, strictly after the resolver returned 0 under the same CS hold; CM_Unregister_Notification only behind the notify_handle == NULL early return (a non-NULL handle implies successful registration, which implies resolution). The resolver's "all set" sentinel is the last-assigned pointer, so a partial first resolve retries both. Clearing the pointers in free_library_handles cannot race live machinery: hid_exit reaches it only after detaching+unregistering its handle, waiting out pending_unregistrations (incremented under the CS before detaching — no window), draining the work item, and spinning until hotplug_machinery_users == 0; the leaked-notification path never unloads. The resolve-failure cleanup is shape-identical to the threadpool-failure precedent. The mandatory lookup_functions resolve set is now byte-identical to master's — Win7/Vista hid_init() is restored.
Dynamic TLS — FIXED. No stragglers (every access goes through the three accessors). The getter saves/restores GetLastError() around TlsGetValue, and independently register_global_winapi_error captures the error code before any message building or flag read. Publication ordering is a full happens-before chain: index stored under hotplug_init_lock before the CS is initialized and mutex_ready published, and every event-context entry point acquires the CS first; the only unsynchronized readers are non-event app threads, for which a stale unset index correctly reads as flag 0. Failure paths (TlsAlloc / CreateEvent) report the right operation and unwind cleanly; the never-freed index is documented and idempotent on retry.
Sequential-dispatch assumption comment — FIXED, accurate and appropriately hedged; the FIFO-queue hardening is named as the future alternative.
static linkage — FIXED (definition + registration-site use only; no declaration anywhere).

PR #823 is review-clean from my side.

Drafted with Claude Code.

## Re-review of fix commit `31a799f` A fresh delta review (independent reviewer) verified the fix against all four findings and hunted for newly introduced defects. **All four FIXED; no new defects found.** Windows CI (MSVC, MinGW, msbuild) is green on the commit. - **Lazy CM-notification resolver — FIXED.** `CM_Register_Notification` is called only inside the bootstrap block, strictly after the resolver returned 0 under the same CS hold; `CM_Unregister_Notification` only behind the `notify_handle == NULL` early return (a non-NULL handle implies successful registration, which implies resolution). The resolver's "all set" sentinel is the *last-assigned* pointer, so a partial first resolve retries both. Clearing the pointers in `free_library_handles` cannot race live machinery: `hid_exit` reaches it only after detaching+unregistering its handle, waiting out `pending_unregistrations` (incremented under the CS before detaching — no window), draining the work item, and spinning until `hotplug_machinery_users == 0`; the leaked-notification path never unloads. The resolve-failure cleanup is shape-identical to the threadpool-failure precedent. The mandatory `lookup_functions` resolve set is now byte-identical to master's — Win7/Vista `hid_init()` is restored. - **Dynamic TLS — FIXED.** No stragglers (every access goes through the three accessors). The getter saves/restores `GetLastError()` around `TlsGetValue`, and independently `register_global_winapi_error` captures the error code before any message building or flag read. Publication ordering is a full happens-before chain: index stored under `hotplug_init_lock` before the CS is initialized and `mutex_ready` published, and every event-context entry point acquires the CS first; the only unsynchronized readers are non-event app threads, for which a stale unset index correctly reads as flag 0. Failure paths (TlsAlloc / CreateEvent) report the right operation and unwind cleanly; the never-freed index is documented and idempotent on retry. - **Sequential-dispatch assumption comment — FIXED**, accurate and appropriately hedged; the FIFO-queue hardening is named as the future alternative. - **`static` linkage — FIXED** (definition + registration-site use only; no declaration anywhere). PR #823 is review-clean from my side. _Drafted with Claude Code._

Youw commented

2026-09-08 15:49:55 -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: 27 findings (0 major, 4 minor, 10 nit, 13 note).

Fixes landed in three commits:

378f9df - transactional out-of-memory recovery for uncached arrivals, exit disarm, an interlocked event-context marker replacing the thread-local one (delivery comes from threadpool / CM notification threads, not a dedicated thread), coalesced work items, CONFIGRET reporting
f94a68e - recovered arrivals staged per event and drained in registration order, so ARRIVED/LEFT pairing stays consistent for every armed callback
4db5fd4 - comments only: the ordering of a recovered arrival relative to the repairing callback's snapshot, the recipient set of a recovered arrival, and why hid_exit() still reports -1 after a quiescence-wait anomaly that the polling fallback completed

Final adversarial verification (Fable, on f94a68e): 24 fixed, 1 skipped (its only trigger is contract-forbidden). The 1 nit and 2 notes it raised are the comment-only round 3 above.

CI: green on 4db5fd4; on the aggregate branch (#830) the Windows virtual-device job passes DeviceIO, HotplugAPI and Hotplug with ASan enabled.

Drafted with Claude Code.

## Fresh review pass (2026-09-08) Two independent reviews (Claude Fable 5.1 and gpt-6-astra), merged and deduplicated: **27 findings** (0 major, 4 minor, 10 nit, 13 note). Fixes landed in three commits: - 378f9df - transactional out-of-memory recovery for uncached arrivals, exit disarm, an interlocked event-context marker replacing the thread-local one (delivery comes from threadpool / CM notification threads, not a dedicated thread), coalesced work items, CONFIGRET reporting - f94a68e - recovered arrivals staged per event and drained in registration order, so ARRIVED/LEFT pairing stays consistent for every armed callback - 4db5fd4 - comments only: the ordering of a recovered arrival relative to the repairing callback's snapshot, the recipient set of a recovered arrival, and why `hid_exit()` still reports -1 after a quiescence-wait anomaly that the polling fallback completed Final adversarial verification (Fable, on f94a68e): 24 fixed, 1 skipped (its only trigger is contract-forbidden). The 1 nit and 2 notes it raised are the comment-only round 3 above. CI: green on 4db5fd4; on the aggregate branch (#830) the Windows virtual-device job passes DeviceIO, HotplugAPI and Hotplug with ASan enabled. _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-windows:hotplug-async-enumerate-windows'

git switch 'hotplug-async-enumerate-windows'

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-windows'

git switch 'hotplug-async-enumerate-windows'

git rebase 'connection-callback'

git switch 'connection-callback'

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

git switch 'hotplug-async-enumerate-windows'

git rebase 'connection-callback'

git switch 'connection-callback'

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

git switch 'connection-callback'

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