windows: deliver hotplug ENUMERATE pass asynchronously on the event context#823
Open
Youw wants to merge 9 commits into
Open
windows: deliver hotplug ENUMERATE pass asynchronously on the event context#823Youw wants to merge 9 commits into
Youw wants to merge 9 commits into
Conversation
…nt context Implement the hotplug contract documented in #790 for the windows backend: The HID_API_HOTPLUG_ENUMERATE initial pass is now a deep-copied registration-time snapshot of the matching connected devices, replayed on a threadpool work item (the same critical-section-serialized event context that delivers live events) instead of running synchronously inside hid_hotplug_register_callback on the application thread. A callback's pending snapshot is flushed before any live event is delivered to it, so each device connection is reported exactly once and the pass always precedes live events; a non-zero callback return stops the remainder of the pass and deregisters the callback. Also per the contract: - hid_hotplug_deregister_callback returns -1 for unknown or stale handles (0 only when the callback was found and deregistered) - every register/deregister failure path sets the global error string, and *callback_handle is zeroed on registration failure - undelivered snapshots are freed on deregistration and hid_exit - CM_Unregister_Notification is no longer called from within the notification callback (forbidden, deadlocks) nor while holding the hotplug critical section (deadlocks against in-flight notifications): the teardown is detached under the lock and performed outside it, deferred to the work item when triggered from the notification itself Addresses #793 for the windows backend. Assisted-by: claude-code:claude-fable-5
…TE pass
- Arm the CM notification BEFORE taking the registration-time device
snapshot, and deduplicate arrivals by path in the notification callback:
a device connecting between the two is now caught by the notification
and absorbed by the dedupe instead of being missed by both (it was
reported by neither before)
- Only (re)build the device cache when actually arming a notification;
while one is attached the cache is kept current by its callbacks
(re-enumerating during a deferred teardown could double-report)
- Serialize new-notification arming against in-flight
CM_Unregister_Notification calls (pending counter + condition variable):
a detached handle stays live at the OS until its unregistration
completes, and overlapping registrations would deliver events twice;
hid_exit waits for the same quiescence before destroying state
- Treat a failed CM_Unregister_Notification as a poisoned epoch: hid_exit
reports the failure, returns -1 and deliberately leaks the critical
section, work item and loaded DLLs rather than let a still-live
notification touch destroyed state
- Guard the machinery bootstrap with a statically-initialized SRWLOCK
(two racing first registrations could both initialize the critical
section), and serialize all mutations of the global error string with
another SRWLOCK (concurrent thread-safe failures could double-free it)
- Distinguish a genuinely failed enumeration from an empty system at
registration (fail the registration on the former), and clear the stale
"No HID devices found" error on successful registration
- Fail registration with an error when the callback handle space is
exhausted instead of signed-overflowing and wrapping onto live handles
- Replace { 0 } struct initializers with memset in hid.c for
-Wmissing-field-initializers cleanliness when built as C++ with GNU
compilers; use the WinAPI error helper for CreateThreadpoolWork failures
Addresses round-1 review of the windows part of #793.
Assisted-by: claude-code:claude-fable-5
…TE pass Critical section lifetime. hid_exit() used to delete the critical section (and reset mutex_ready) with nothing holding it, while a registration sleeping on a pending unregistration, a concurrent deregistration or a late notification could still be about to enter it. The critical section and the quiescence event are now created once and kept for the process lifetime, and an explicit `exiting` state, set under the lock, makes registration and deregistration fail instead of arming a notification behind a teardown. `exiting` is only cleared once hid_exit() has unloaded the libraries, which also closes the window in which a thread-safe (and implicitly initializing) hid_hotplug_register_callback() could call into hid.dll/cfgmgr32.dll while they were being unloaded. Global error string. The internal event context no longer writes it: hid_error(NULL) hands the raw pointer to the application without a lock, so a threadpool work item freeing it underneath an application thread is a use-after-free the application cannot serialize away. The unregistration failure is recorded in internal state and reported by the next hid_exit() instead. The writer-side lock is kept. Failed unregistration. Scoped to the teardown that reports it, so hid_exit() no longer fails forever after one failure. A leaked notification is still live at the OS level, so no second notification is ever armed next to it (both would deliver every event) - hotplug registration fails with an explicit error instead - the libraries it calls into stay loaded, and the module that contains the callback is pinned so that it cannot be unmapped underneath the OS. Arrival dedupe. It only ever existed to absorb the overlap between arming the notification and taking the registration-time snapshot, but it suppressed every arrival whose path was cached - and an interface path is reused when a device is replugged into the same port. It is now scoped to that window: the enumerated paths are recorded, each swallows at most one arrival, and a path is dropped as soon as its device leaves. Outside the window an arrival for a cached path is a new connection, and it replaces the stale record rather than being shadowed by it. Enumeration and cache consistency. A per-device allocation failure is reported through the failure channel instead of passing for a successful (but silently short) enumeration; a device_info is never handed out half-built, so the cache can no longer hold a record with a NULL path - which the removal walk dereferenced - and the bus-specific fixups can no longer read a NULL string. Threadpool. Resolved dynamically from kernel32 (and hotplug registration fails cleanly when it is unavailable) instead of forcing _WIN32_WINNT to 0x0600, which turned the threadpool, SRWLOCK and condition-variable calls into load-time Vista-only imports for every user of the library, including those that never touch hotplug. Assisted-by: claude-code:claude-opus-4-8
…TE pass Machinery lifetime, take three. Round 2 made hid_exit() bootstrap the hotplug machinery just to raise the `exiting` flag, and never destroyed it - so every program, including ones that never touch hotplug, had hid_exit() create a kernel event and a critical section it could not free: one leaked handle per load-use-unload cycle in a plugin-style host. The flag does not need the critical section: `hotplug_exiting` now lives under the statically initialized bootstrap spinlock (which costs no OS object), so hid_exit() can raise it before knowing whether the machinery even exists and return without creating anything. On top of that, public hotplug calls are now counted into the machinery through the same lock (enter/leave), which gives hid_exit() 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 has hit zero, nothing can be inside or blocked on the critical section - so the clean path destroys the event and the critical section it created instead of keeping them for the process lifetime. When a notification could not be unregistered, everything is still kept, exactly as in round 2: a live OS callback must never find a deleted critical section. 100 x (LoadLibrary, hid_init, hid_enumerate, hid_exit, FreeLibrary) now leaks 0 handles (was +1 per cycle), with or without hotplug use in the loop. Pending-arrivals dedupe, scoped in time as well. The registration-time records suppressed the one in-flight duplicate they were made for, but a record whose device never re-arrived outlived that window - so a device whose removal notification was missed had its genuine re-arrival swallowed by its own stale record, and the stale-record recovery could never run for exactly the devices it was written for. The list now expires (10 s, far beyond any notification delivery): an expired record no longer marks a duplicate, and the failure modes are asymmetric in the right direction - an over-aged duplicate produces a spurious left+arrived pair instead of a silently dropped connection. The owed 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 and the stale record is now dispatched as a synthetic DEVICE_LEFT - before the new device is cached, so a callback registered from within sees the connection exactly once. Spinlock discipline. The global error string is now built and freed outside the spinlock - only the pointer swap is guarded - 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 on a lower-priority holder on a single CPU made no progress until the balance-set manager intervened. Enumeration failure taxonomy. hid_internal_UTF16toUTF8() returns NULL both for invalid UTF-16 (e.g. an unpaired surrogate in an interface path) and on allocation failure, and round 2 escalated both to a full enumeration failure reported as out-of-memory - one malformed path lost every other device. The two are now told apart: 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 corrects the comment on the global error lock (a user callback calling the public API from the event context does write the global error; the header's serialization requirement covers it), removes a dead `failed` term that notification_leaked always implies, and documents why the registration wait loop may release the critical section unconditionally. Assisted-by: claude-code:claude-fable-5
The registration-time arrival dedupe was scoped by a 10-second wall-clock TTL.
An arrival whose pending-arrival marker had expired was reclassified as a new
connection on an already-cached path, which synthesized a DEVICE_LEFT for the
cached record before reporting a second DEVICE_ARRIVED.
Elapsed time is not a sound basis for that decision. A user callback is only
ADVISED to be short, so a perfectly legal slow callback that holds the critical
section for longer than the TTL let the still-queued arrival of a device that
was present at registration expire: one physical connection was then reported
as ARRIVED -> LEFT -> ARRIVED, breaking the documented exactly-once guarantee.
Dedupe against the live device cache instead, as the linux and mac backends do.
That needs no timing assumption anywhere:
- The OS delivers exactly one arrival notification per interface instance, so
the only way an arrival can be a duplicate is the arm-before-enumerate
window at registration - and there the enumeration has, by construction,
already put the device in the cache. An arrival for a path that is still
cached is therefore always a duplicate: skip the append and the dispatch.
- The removal notification is authoritative and drops the cache entry, so a
genuine re-plug - even onto the same, reused interface path - is not in the
cache and is reported.
The missed-removal recovery that the TTL existed to reach is removed along with
it, rather than re-gated on some other signal: a missed CM removal is not a real
scenario (removals arrive through the very notification that delivers arrivals),
and a spurious LEFT+ARRIVED pair for a device that never disconnected is a far
worse failure than the hypothetical duplicate it guarded against.
Assisted-by: claude-code:claude-opus-4-8
Deduping arrivals against the live cache made that cache the single source of truth for what has been reported, and removed the stale-record recovery that used to paper over an inconsistent cache. That is sound only if the cache can never be left inconsistent - and one path could still leave it so. Both cache lookups took the interface path a CM notification carries (UTF-16) and converted it to UTF-8 before searching, which allocates. In the REMOVAL lookup that allocation is load-bearing: if it fails under memory pressure the removal is processed but its cache record survives. Because the cache is now the whole arrival dedupe, a surviving record is unrecoverable - interface paths are reused when a device is replugged into the same port, so the next connection on that path is classified as a duplicate and suppressed forever, for every callback, including later HID_API_HOTPLUG_ENUMERATE passes. The device becomes invisible. The removal notification did arrive; only its local processing failed, so the "removals come through the same notification stream" argument does not save it. Compare the cached UTF-8 path against the notification's UTF-16 symbolic link directly, encoding the UTF-16 to UTF-8 one code point at a time and matching it against the cached bytes (ASCII case-independent, exactly as the _stricmp() it replaces was). Both lookups - the arrival dedupe and the removal - are now allocation-free, so an out-of-memory condition can no longer desynchronize the cache from the system. The only allocation left on the notification path is the one that describes an arriving device, where failing it simply does not cache the device - and nothing was reported for it either, so the cache stays consistent. This is preferred over caching a second, wide copy of each path as a comparison key: it adds no per-device allocation to keep and free, no field to keep in sync with dev->path, and no change to the cached record type - the one remaining allocation is the one whose failure was always handled cleanly by dropping the arrival. Assisted-by: claude-code:claude-opus-4-8
The hotplug callback runs on HIDAPI's internal event context (a threadpool work item or the CM notification callback). A user callback that re-enters the public hid_hotplug_register/deregister_callback from there wrote the global error string on that context, which an application cannot serialize its lock-free hid_error(NULL) read against. Drop such writes at the global-error chokepoint while a dispatch is in progress (mutex_in_use), matching the other backends; per-device hid_error(dev) writes are unaffected. App-thread failures still set the global error as before. Also guard hid_winapi_get_container_id against a NULL dev->device_info, like its sibling accessors: hid_open_path stores hid_internal_get_device_info() without a NULL check and that helper can now return NULL. Assisted-by: claude-code:claude-opus-4-8
The previous guard suppressed global-error writes based on mutex_in_use, which
is process-global ("a dispatch is in progress somewhere"). That wrongly dropped
the genuine error of a concurrent application thread calling the hotplug API
while a callback ran on the event context, and it read the flag without
synchronization (a data race).
Use a thread-local flag set only around each user-callback invocation in both
dispatch paths instead, so suppression applies to the dispatching thread alone:
an application thread always records its errors, while a callback's nested
register/deregister writes (failure and the success-path clear) are dropped.
This matches how the other backends key off the event thread's identity.
Assisted-by: claude-code:claude-opus-4-8
__declspec(thread) is silently ignored by MinGW/Cygwin GCC (a -Werror=attributes failure), breaking the fedora-mingw and windows-cmake-mingw builds. Select __declspec(thread) for MSVC/clang-cl and __thread for GCC/Clang. Assisted-by: claude-code:claude-opus-4-8
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the hotplug contract documented via #790 for the windows backend (
windows/hid.conly).What changed
hid_hotplug_register_callbackno longer runs theHID_API_HOTPLUG_ENUMERATEpass 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 andeventsincludesDEVICE_ARRIVED) into a per-callbackreplaylist, and submits a threadpool work item (CreateThreadpoolWork/SubmitThreadpoolWork) that delivers it on the same critical-section-serialized context that delivers liveCM_Register_Notificationevents. The pass never fires from withinregisteritself and never on an application thread.device->nextis nowNULLfor every invocation, including the synthetic ones (the old sync pass handed out devices still linked into the cached list).0only when the handle was found and deregistered;-1for unknown, stale or already-self-deregistered handles (safe no-op).hid_error(NULL);*callback_handleis zeroed on any registration failure.CM_Unregister_Notificationis 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_exitfrees 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
PTP_WORKcreated with the first callback registration and closed byhid_exit. I usedCreateThreadpoolWorkrather thanTrySubmitThreadpoolCallbackbecause teardown needsWaitForThreadpoolWorkCallbacksto guarantee no work item can touch the critical section afterhid_exitdeletes it; the fire-and-forget API offers no way to wait.mutex_in_useset: whichever of the work item or a live event acquires the lock first delivers the pass, the other findsreplay == NULL._WIN32_WINNTto0x0600when 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
Review round 1
Reviewed independently by two reviewers against the #790 contract; all confirmed findings are addressed in
a3350c4:CM_Unregister_Notificationcalls 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_exitracing a concurrent deregister — the same quiescence gate is awaited before destroying state (previously it couldDeleteCriticalSection/CloseThreadpoolWorkwhile a detached notification was still live at the OS). A failedCM_Unregister_Notificationnow poisons the epoch:hid_exitreports the failure and deliberately leaks the critical section, work item and DLL handles rather than let a live notification touch destroyed state.SRWLOCK(two racing first registrations could both initialize the critical section), and all mutations of the global error string are serialized by anotherSRWLOCK(concurrent failures in the documented-thread-safe register/deregister could double-free it).{ 0 }struct initializers replaced withmemsetfor-Wmissing-field-initializersunder GNU C++.Review round 2
Reviewed independently by two further reviewers; all confirmed findings are addressed in
1f621c1:hid_exitdeleted the critical section with no lock held across the teardown, while a registration sleeping on a pending unregistration, a concurrent (contract-legal) deregistration, orhid_internal_hotplug_initcould 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 explicitexitingstate, set under the lock, makes registration and deregistration fail rather than arm a notification behind a teardown.mutex_readyis only ever read under the bootstrap lock or under the critical section itself.hid_exitunloading the DLLs under a concurrent registration — found by the new stress test.hid_hotplug_register_callbackis thread-safe and initializes the library implicitly, so it could be callingCM_Register_Notificationthrough a function pointer whilehid_exitwasFreeLibrary-ingcfgmgr32.dll.exitingis 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.hid_error(NULL)returns the raw string pointer with no lock, so the threadpool work item reporting a failedCM_Unregister_Notificationcouldfree()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 nexthid_exit. The writer-side lock (round 1) is kept for the application-thread writers.hid_exitthat reports it (a single failed unregistration no longer makes every laterhid_exitreturn -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.hid_device_infois never handed out half-built, so the hotplug cache can no longer hold a record with aNULLpath — which the removal walk dereferenced — and the bus-specific fixups (hid_internal_get_usb_info/_get_ble_info, whichwcslen()the strings) can no longer read aNULL._WIN32_WINNTforce-bump removed (supersedes the round-1 design note) — it turned the threadpool,SRWLOCKand condition-variable calls into load-time Vista-only imports ofhidapi.dllfor every user, including those that never touch hotplug, and it contradicted the file'sLoadLibrary/GetProcAddressdiscipline. The four threadpool entry points are now resolved fromkernel32like 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 /importson 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_exitwith a pending replay, re-init, argument validation,hid_exitracing 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 /WXbuilds,dumpbin /imports, a DLL load/unload handle-leak harness, and a 6-thread register/deregister storm racinghid_exit) confirmed the round-2 synchronization model holds, and found one newly-introduced blocker plus several minors; all are addressed in0ddecf2:hid_exitleaked a kernel event and a critical section for every user (blocker, introduced by round 2) — round 2 madehid_exitbootstrap the hotplug machinery just to raise theexitingflag, and the never-destroy policy then guaranteed the event handle and the critical section were never freed: a plainhid_init/hid_enumerate/hid_exitprogram that never touches hotplug leaked one handle per load/use/unload cycle (measured: +100 handles over 100 cycles), contradictinghid_exit's documented purpose. The flag now lives under the statically-initialized bootstrap spinlock — which costs no OS object — sohid_exitcan 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: onceexitingcuts 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 ofhid_exitnow 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).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 syntheticDEVICE_LEFTbefore the new device is cached, preserving exactly-once for callbacks registered from within a callback.FormatMessageWplusfree/callocran under it), and the acquire loop escalates fromSleep(0)toSleep(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.hid_internal_UTF16toUTF8returnsNULLboth 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.failedterm the stickynotification_leakedflag 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 theHIDAPI_USE_DDKcompile),dumpbin /imports(still no*Threadpool*/*SRWLock*/*ConditionVariable*imports; the only new kernel32 import isGetTickCount), 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-threadhid_exitrace, all green.