Repository navigation
Conversation
New functions on hid_device, implemented across all five backends (linux, libusb, mac, windows, netbsd): hid_read_interrupt - asynchronously cancel a blocked read hid_is_read_interrupted - query the sticky interrupt state hid_read_clear_interrupt - clear the state, allowing reads to resume hid_read_interrupt is the only hidapi function safe to call concurrently with another function on the same device. Other device operations (write, feature/output reports, info getters) are unaffected. Allows a dedicated reader thread to be stopped without busy-polling on a short timeout. New hid_read_test tool, to check new functionality. Resolves: #146 Assisted-by: Claude:claude-opus-4.7
Youw
changed the title
There was a problem hiding this comment.
Pull request overview
This PR adds a new public HIDAPI mechanism to asynchronously interrupt a blocked hid_read()/hid_read_timeout() call in a thread-safe way (intended to allow clean shutdown of dedicated reader threads without short timeouts/busy polling), and introduces a small hid_read_test utility to exercise the behavior.
Changes:
- Adds
hid_read_interrupt(),hid_is_read_interrupted(), andhid_read_clear_interrupt()to the public header and implements them across all backends. - Updates backend
hid_read_timeout()implementations to return-1with an “operation interrupted” read error when interruption is requested. - Adds a new optional
hid_read_testtool and a top-level CMake option to build it.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
windows/hid.c |
Implements interrupt via a manual-reset event + interlocked flag and integrates it into WaitForMultipleObjects() in hid_read_timeout(). |
netbsd/hid.c |
Implements interrupt via a nonblocking pipe added to the poll() fd set, plus a sticky interrupt flag. |
mac/hid.c |
Implements interrupt via a mutex-protected flag + condition broadcast integrated into the read wait loops. |
linux/hid.c |
Implements interrupt via eventfd added to poll() plus a sticky interrupt flag; adjusts failure cleanup path to call hid_close(). |
libusb/hid.c |
Implements interrupt via a mutex-protected flag + condition broadcast integrated into the read wait loops. |
hidapi/hidapi.h |
Adds the public API declarations and documents the intended thread-safety/semantics. |
hid_read_test/main.cpp |
New command-line tool that opens a device, runs a blocking read thread, and interrupts on Enter/Ctrl+C. |
hid_read_test/CMakeLists.txt |
Adds build/install rules for the new test tool (standalone or as a subdir). |
CMakeLists.txt |
Adds HIDAPI_BUILD_HID_READ_TEST option to include the new tool in the build. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1149,12 +1165,15 @@ int HID_API_EXPORT hid_read_timeout(hid_device *dev, unsigned char *data, size_t | |||
| properly report device disconnection through read() when | |||
| in non-blocking mode. */ | |||
| int ret; | |||
| struct pollfd fds; | |||
|
|
|||
| fds.fd = dev->device_handle; | |||
| fds.events = POLLIN; | |||
| fds.revents = 0; | |||
| ret = poll(&fds, 1, milliseconds); | |||
| struct pollfd fds[2]; | |||
|
|
|||
| fds[0].fd = dev->device_handle; | |||
| fds[0].events = POLLIN; | |||
| fds[0].revents = 0; | |||
| fds[1].fd = dev->interrupt_efd; | |||
| fds[1].events = POLLIN; | |||
| fds[1].revents = 0; | |||
| ret = poll(fds, 2, milliseconds); | |||
| /* Interrupt fired and no data is ready. The pending ReadFile is | ||
| left in flight; it will resume on the next hid_read_timeout() | ||
| call after hid_read_clear_interrupt(). */ | ||
| register_string_error_to_buffer(&dev->last_read_error_str, L"hid_read_timeout: operation interrupted"); |
| #ifdef _WIN32 | ||
| std::signal(SIGINT, on_signal); | ||
| #ifdef SIGTERM | ||
| std::signal(SIGTERM, on_signal); |
| extern "C" void on_signal(int) | ||
| { | ||
| /* async-signal-safe: atomic store only. Main thread will call | ||
| hid_read_interrupt() once cin.get() returns from EINTR. */ | ||
| g_terminate.store(true, std::memory_order_release); |
| option(HIDAPI_BUILD_HID_READ_TEST "Build hid_read_test cmd-line tool" OFF) | ||
| if(HIDAPI_BUILD_HID_READ_TEST) | ||
| add_subdirectory(hid_read_test) |
| 1 if the read pipeline is interrupted, 0 if not, -1 on error. | ||
| Call hid_error(dev) to get the failure reason. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
- linux/hid.c: poll() unconditionally in hid_read_timeout(), including the blocking (milliseconds < 0) path, so hid_read_interrupt() can wake a blocking read via the interrupt eventfd. Previously the blocking path went straight to read() and never observed the interrupt, so a dedicated reader thread blocked in hid_read() could not be woken — defeating the documented interrupt -> join -> hid_close() shutdown. - hidapi/hidapi.h: hid_is_read_interrupted() is documented as returning only 1/0; drop the misleading "-1 on error" wording since no backend has an error path here. - hid_read_test/main.cpp: use a volatile sig_atomic_t flag (the type the standard guarantees safe to write from a signal handler) instead of std::atomic<bool>, and observe it from the main thread via a detached input-wait helper so Ctrl+C reliably triggers shutdown on all platforms (including Windows, where the console read isn't interrupted by signals). - CMakeLists.txt: add the hid_read_test* targets to the ASan target list. Assisted-by: Claude:claude-opus-4.8
Add a backend-generic HIDAPI test harness and a unit test for the
hid_read_interrupt() / hid_is_read_interrupted() / hid_read_clear_interrupt()
API.
src/tests/test_virtual_device.h
An opaque, backend-agnostic "virtual HID device" interface (create,
inject input, set feature reply, capture output, open via HIDAPI,
destroy). Test scenarios depend only on this, so the same scenarios can
be reused for other backends by adding a provider.
src/tests/test_virtual_device_uhid.c
Linux provider, backed by the kernel /dev/uhid interface: it makes the
kernel expose a real /dev/hidrawN node that the hidraw backend opens.
Answers GET_REPORT and captures OUTPUT/SET_REPORT via an event pump
thread (useful for future read/write/feature tests too).
src/tests/test_read_interrupt.c
Scenarios: normal read delivery, interrupt-before-read (non-blocking
return), blocking read interrupted from another thread (the regression
guard for the hidraw blocking-path fix), timed read interrupted, sticky
interrupt, clear resumes reads, idempotent interrupt/clear. Uses
pthread_timedjoin_np so a regression fails instead of hanging.
The test self-skips (CTest SKIP_RETURN_CODE 77) when no virtual device can
be created (e.g. no 'uhid' module / insufficient privileges), so it never
fails spuriously.
Build/CI:
- HIDAPI_WITH_TESTS is now offered on Linux (default OFF) and builds
src/tests for the hidraw backend.
- The ubuntu-cmake CI job configures with -DHIDAPI_WITH_TESTS=ON, loads
the uhid module (from linux-modules-extra) and runs ctest as root
(LeakSanitizer off, ASan use-after-free detection on).
Assisted-by: Claude:claude-opus-4.8
Resolve the linux/hid.c include conflict: master's IWYU cleanup (#813) removed the unused <sys/utsname.h>; keep this branch's <sys/eventfd.h> and <stdint.h>, which the eventfd-based read interruption uses. Assisted-by: claude-code:claude-fable-5
Code review found the documented sticky contract -- "once interrupted, subsequent hid_read()/hid_read_timeout() calls return -1 immediately" -- was violated by the mac and libusb backends: both returned queued input reports before checking the interrupt flag, and their reader threads keep queuing new reports after an interrupt. With a device that streams input continuously, the documented shutdown pattern (interrupt -> join reader -> close) never observed -1 on those two backends and the join could hang forever. windows/linux/netbsd already check the flag at entry. - mac, libusb: check read_interrupted at hid_read_timeout() entry, before the queued-report drain, matching the other three backends. A call already blocked when the interrupt arrives keeps its existing behavior (may still return data that raced in). - hidapi.h: tighten the drain-allowance wording to match: only a call already in progress may still return buffered data; calls entered after the interrupt fail immediately, and queued data remains readable after hid_read_clear_interrupt(). - windows: fail hid_open*() if the interrupt event cannot be created -- a NULL handle would make WaitForMultipleObjects() fail instantly, turning a blocking read into a busy spin returning 0. Findings surfaced by a multi-model adversarial code review. Assisted-by: claude-code:claude-fable-5
|
Looks like this branch has conflicts with the current main branch. But I will try to give this draft PR a test later this week. |
Master landed the full virtual-device test harness (#728), which supersedes this branch's early copy of test_virtual_device.h and the uhid provider. Take master's harness, CMake option and CI step, and port test_read_interrupt.c to its trigger API (input reports are now replayed by the device in response to a Feature report instead of being injected directly). The test stays Linux/hidraw-only for now and is registered as ReadInterrupt_hidraw. Assisted-by: claude-code:claude-opus-5-5
Youw
left a comment
Youw
left a comment
There was a problem hiding this comment.
Merged latest master into the branch (bef57f7). Master's virtual-device harness (#728) superseded this branch's early copy of test_virtual_device.h / the uhid provider, so test_read_interrupt.c was ported to the harness's trigger API; it is registered as ReadInterrupt_hidraw (Linux/hidraw only, as before).
Review below: gpt-6-astra (via Codex), with each finding re-checked against the code and the API usage contract.
- major: Windows: an interrupted
ReadFileis still pending whenhid_close()frees its buffers. This extends Copilot's earlier comment. - major: concurrent
hid_read_interrupt()/hid_read_clear_interrupt()can leave the wake-up object signalled with the flag cleared (linux / netbsd / windows). - minor:
hid_read_test: data race ong_terminatebetween two threads. - minor:
test_read_interrupt.c: the hang path returns while the reader thread still points at a stackctx. - doc: wording of the concurrency contract. Separately, the PR description still says
hid_read_interruptis "the only hidapi function safe to call concurrently", which no longer matches the header.
Follow-up suggestion (not a blocker): test_read_interrupt.c only runs on Linux/hidraw because it uses pthread_timedjoin_np. Most of the new wait logic is in the winapi/darwin/libusb backends, and master now has virtual-device providers for all three. Porting the test to test_platform.h threading and registering it per provider would cover them.
Status of the earlier Copilot review comments (checked against current HEAD)
| Comment | Topic | Status |
|---|---|---|
| r3144280258 | windows: comment spelling | fixed |
| r3144280273 | linux: errno clobbered on open failure |
fixed: hid_close() skips close(-1) |
| r3144280278 | linux: eventfd() errno reported as ENOMEM |
fixed |
| r3144280284 | linux: volatile flag |
fixed: __atomic acquire/release |
| r3144280292 | netbsd: volatile flag |
fixed: __atomic acquire/release |
| r3144280301 | header promises acquire semantics | fixed: atomics / Interlocked / mutex in every backend |
| r3144280307 | hid_read_interrupt() called from a signal handler |
fixed: the handler only sets a flag (new thread race noted inline) |
| r3144280313 | CMake minimum version | fixed |
| r3190018988 | linux: blocking read didn't poll the eventfd | fixed: always polls; covered by ReadInterrupt_hidraw |
| r3190019029 | windows: pending ReadFile vs CancelIo in hid_close() |
still valid: a reader thread that exits is protected by the OS cancelling its I/O at thread rundown; a reader that stays alive is not (details inline) |
| r3190019061 | windows: Ctrl+C stuck in cin.get() |
fixed: stdin is read on a helper thread |
| r3190019081 | std::atomic<bool> in a signal handler |
fixed: now sig_atomic_t (but see the inline note on the thread race) |
| r3190019101 | autotools/meson integration of hid_read_test |
not needed: optional CMake-only tool, like src/tests; meson wraps CMake |
| r3190019132 | contradictory "only function" concurrency doc | fixed in the header; the PR description still says "only" |
| r3190019153 | ASan for the new targets | fixed |
| r3190019180 | hid_is_read_interrupted() documented "-1 on error" |
fixed |
Drafted with Claude Code.
| left in flight; it will resume on the next hid_read_timeout() | ||
| call after hid_read_clear_interrupt(). */ | ||
| register_string_error_to_buffer(&dev->last_read_error_str, L"hid_read_timeout: operation interrupted"); | ||
| return -1; |
There was a problem hiding this comment.
[major] Windows: after an interrupt, the pending ReadFile still owns dev->ol / dev->read_buf, and hid_close() neither cancels it from another thread nor waits for it. This extends Copilot's earlier comment on this line.
After this return -1, the overlapped ReadFile stays pending and keeps owning dev->ol and dev->read_buf. hid_close() (L1472) only calls CancelIo(), which cancels I/O issued by the calling thread. It then calls free_hid_device(), which frees the OVERLAPPED and the buffer without waiting for the request to complete. ReadFile requires both to stay valid until the operation completes.
The documented interrupt → join → close sequence is mostly protected by the OS: when the reader thread exits, Windows cancels its outstanding I/O and waits for it during thread rundown. The API doesn't require the reader to exit, though:
- A pooled/executor worker blocks in
hid_read(); the controller callshid_read_interrupt(). - The worker returns -1, signals "done", and goes back to the pool. The thread stays alive.
- The controller calls
hid_close(). This doesn't overlap any HIDAPI call, so it's contract-valid. CancelIoon the controller thread doesn't touch the worker's request. Its completion can then write into the freedOVERLAPPED/ buffer: use-after-free.
The root cause (CancelIo with no wait in hid_close()) predates this PR, because a timed read that returns 0 also leaves the I/O pending. This PR makes "read still pending, issued by another thread, at close time" the recommended shutdown path, though.
Suggested fix: in hid_close(), if dev->read_pending, call CancelIoEx(dev->device_handle, &dev->ol) and then GetOverlappedResult(dev->device_handle, &dev->ol, &bytes, TRUE) before freeing anything.
|
|
||
| int HID_API_EXPORT hid_read_clear_interrupt(hid_device *dev) | ||
| { | ||
| __atomic_store_n(&dev->interrupted, 0, __ATOMIC_RELEASE); |
There was a problem hiding this comment.
[major] Concurrent hid_read_interrupt() / hid_read_clear_interrupt() can leave the wake-up object signalled while the flag is cleared. The same pattern is in netbsd/hid.c (pipe) and windows/hid.c (manual-reset event); mac/libusb are fine because both steps happen under dev->mutex.
The flag and the wake-up object are updated in two separate steps, so the pair isn't atomic:
T1: hid_read_interrupt() T2: hid_read_clear_interrupt()
interrupted = 1
interrupted = 0
read(efd) -> EAGAIN (nothing to drain yet)
write(efd, 1)
End state: hid_is_read_interrupted() returns 0, but the eventfd (pipe / event) stays readable. Every later hid_read*() call passes the entry check, wakes up at once on the interrupt fd and returns -1 "operation interrupted". This lasts until someone calls hid_read_clear_interrupt() again. No read has to be in flight, so this isn't the "undefined timing" case that the clear docs carve out. The header explicitly allows hid_read_interrupt() to run concurrently with any other function on the device.
Suggested fix: either serialize interrupt/clear (a small mutex / critical section around flag + signal), or treat the wake-up object only as a hint in the read path. On an interrupt-fd/event wake-up, re-check the flag; if it's 0, drain/reset the wake-up object and keep waiting for the remaining timeout.
| read is not interrupted by the signal. */ | ||
| std::thread input_waiter([]() { | ||
| std::cin.get(); /* Enter or EOF */ | ||
| g_terminate = 1; |
There was a problem hiding this comment.
[minor] g_terminate is now written by this helper thread and read by the main thread's polling loop (L163). volatile sig_atomic_t is only defined for communication between a signal handler and the thread it interrupted. Between two threads it's a plain data race, which is UB in C++11, even though mainstream compilers handle it in practice.
Suggested fix: use std::atomic<int> (or std::atomic<bool>) with static_assert(ATOMIC_INT_LOCK_FREE == 2, ...), which keeps the signal handler async-signal-safe. That covers both this race and the earlier lock-freedom concern.
|
|
||
| if (joined != 0) { | ||
| printf(" CHECK FAILED: timed read was not interrupted (hang)\n"); | ||
| return -1; |
There was a problem hiding this comment.
[minor] On a join timeout, this returns while the reader thread still holds &ctx, a stack local. L163 in the blocking-read scenario has the same problem. If the read returns later (when a report arrives, or when the virtual device is destroyed at the end of main()), reader_fn writes ctx.result into a dead stack frame that another scenario may already be using. main() also goes on to hid_exit() / test_virtual_device_destroy() with the reader still inside hid_read_timeout(). This only happens when the test is already failing, but it can turn a clean FAIL into a crash or ASan report that hides the real cause.
Suggested fix: on a hang, print the failure and exit(1) right away. Alternatively, heap-allocate the context, leak it on purpose and skip the remaining scenarios.
| feature/output report functions, and all other operations on @p dev | ||
| continue to work normally. | ||
|
|
||
| This function may be called concurrently with another function |
There was a problem hiding this comment.
[doc] A few notes on the concurrency contract:
- "may be called concurrently with another function operating on the same device" should exclude
hid_close()(andhid_exit()). The caller still has to keepdevvalid for the duration of the call. - It would help to say explicitly which of the three new functions may overlap each other.
hid_is_read_interrupted()is described as "suitable for cross-thread observation", andhid_read_clear_interrupt()is only recommended not to overlap a read. Mayhid_read_clear_interrupt()overlaphid_read_interrupt()? See the race noted onlinux/hid.c. @returns ... Call hid_error(dev) to get the failure reason(L465-466): from the interrupting thread,hid_error(dev)isn't safe while another thread is operating on the device. Also, no backend currently ever returns -1 here. Consider documenting that the call doesn't fail, or not pointing athid_error(dev)for this function.
New functions on hid_device, implemented across all five backends (linux, libusb, mac, windows, netbsd):
hid_read_interrupt - asynchronously cancel a blocked read
hid_is_read_interrupted - query the sticky interrupt state
hid_read_clear_interrupt - clear the state, allowing reads to resume
hid_read_interrupt is the only hidapi function safe to call concurrently with another function on the same device. Other device operations (write, feature/output reports, info getters) are unaffected. Allows a dedicated reader thread to be stopped without busy-polling on a short timeout.
New hid_read_test tool, to check new functionality.
Resolves: #146
Assisted-by: Claude:claude-opus-4.7