WIP: RFC: Add hid_read_interrupt API for thread-safe read cancellation #799

archiver-bot · 2026-04-26T17:28:54-05:00

Youw commented

2026-04-26 17:28:54 -05:00

(Migrated from github.com)

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

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

👍 1

copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed

2026-04-26 17:34:48 -05:00

copilot-pull-request-reviewer[bot] (Migrated from github.com) left a 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(), and hid_read_clear_interrupt() to the public header and implements them across all backends.
Updates backend hid_read_timeout() implementations to return -1 with an “operation interrupted” read error when interruption is requested.
Adds a new optional hid_read_test tool 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.

## 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()`, and `hid_read_clear_interrupt()` to the public header and implements them across all backends. - Updates backend `hid_read_timeout()` implementations to return `-1` with an “operation interrupted” read error when interruption is requested. - Adds a new optional `hid_read_test` tool 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. <details> <summary>Show a summary per file</summary> | 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. | </details> --- 💡 <a href="/libusb/hidapi/new/master?filename=.github/instructions/*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.

hid_read_test/CMakeLists.txt Outdated

					
				@ -0,0 +1,54 @@

				cmake_minimum_required(VERSION 3.1.3...3.25 FATAL_ERROR)

The CMake minimum version here (3.8) is higher than the repository’s top-level minimum (3.1.3). Enabling HIDAPI_BUILD_HID_READ_TEST will therefore make the overall build require a newer CMake than advertised. Consider aligning this cmake_minimum_required() with the top-level requirement unless this tool genuinely needs newer CMake features.

cmake_minimum_required(VERSION 3.1.3 FATAL_ERROR)

The CMake minimum version here (3.8) is higher than the repository’s top-level minimum (3.1.3). Enabling `HIDAPI_BUILD_HID_READ_TEST` will therefore make the overall build require a newer CMake than advertised. Consider aligning this `cmake_minimum_required()` with the top-level requirement unless this tool genuinely needs newer CMake features. ```suggestion cmake_minimum_required(VERSION 3.1.3 FATAL_ERROR) ```

hid_read_test/main.cpp

					
				@ -0,0 +59,4 @@

				    /* async-signal-safe: a single store to a volatile sig_atomic_t. The main

				       thread polls g_terminate and calls hid_read_interrupt() from there. */

				    g_terminate = 1;

				}

on_signal() calls hid_read_interrupt() directly from a signal handler. That is not async-signal-safe on POSIX (e.g. mac/libusb implementations take a pthread mutex/condvar), and can deadlock or crash if the signal interrupts code holding the same lock. Consider making the handler only set an atomic flag (or write to a self-pipe) and have the main thread (or a dedicated signal-wait thread using sigwait) call hid_read_interrupt() from a safe context.

`on_signal()` calls `hid_read_interrupt()` directly from a signal handler. That is not async-signal-safe on POSIX (e.g. mac/libusb implementations take a pthread mutex/condvar), and can deadlock or crash if the signal interrupts code holding the same lock. Consider making the handler only set an atomic flag (or write to a self-pipe) and have the main thread (or a dedicated signal-wait thread using `sigwait`) call `hid_read_interrupt()` from a safe context.

hidapi/hidapi.h

					
				@ -426,0 +473,4 @@

							Returns the current interrupt state set by @ref hid_read_interrupt

							and @ref hid_read_clear_interrupt. Suitable for cross-thread

							observation (read with acquire semantics).

The comment promises cross-thread observation with acquire semantics, but at least the Linux and NetBSD backends implement the interrupt state as a plain int/volatile int without atomics or locking. Either strengthen the implementations to actually provide atomic/acquire semantics everywhere, or relax/reword this guarantee in the public header to match what is implemented.

			and @ref hid_read_clear_interrupt. This reports the library's
			current interrupt flag for @p dev, but does not by itself provide
			any additional cross-thread synchronization or memory-ordering
			guarantees.

The comment promises cross-thread observation with acquire semantics, but at least the Linux and NetBSD backends implement the interrupt state as a plain `int`/`volatile int` without atomics or locking. Either strengthen the implementations to actually provide atomic/acquire semantics everywhere, or relax/reword this guarantee in the public header to match what is implemented. ```suggestion and @ref hid_read_clear_interrupt. This reports the library's current interrupt flag for @p dev, but does not by itself provide any additional cross-thread synchronization or memory-ordering guarantees. ```

linux/hid.c

dev->interrupted is accessed concurrently from hid_read_interrupt(), hid_read_clear_interrupt(), and hid_read_timeout() using plain (volatile) loads/stores. volatile does not make this thread-safe in C; this introduces a data race/UB and breaks the API guarantee that hid_read_interrupt() is safe to call concurrently with a blocked read. Use an actual atomic (C11 stdatomic.h, compiler atomics) or protect the flag with a mutex, so both the state and the wakeup mechanism have well-defined cross-thread semantics.

`dev->interrupted` is accessed concurrently from `hid_read_interrupt()`, `hid_read_clear_interrupt()`, and `hid_read_timeout()` using plain (volatile) loads/stores. `volatile` does not make this thread-safe in C; this introduces a data race/UB and breaks the API guarantee that `hid_read_interrupt()` is safe to call concurrently with a blocked read. Use an actual atomic (C11 `stdatomic.h`, compiler atomics) or protect the flag with a mutex, so both the state and the wakeup mechanism have well-defined cross-thread semantics.

linux/hid.c

new_hid_device() can now fail because eventfd() failed (e.g., EMFILE/ENFILE/EPERM), but the caller (hid_open_path) still treats any NULL return as an allocation failure and overwrites errno/global error accordingly. Consider preserving the real errno from eventfd() and reporting a more accurate error (or providing a fallback wakeup mechanism) so device opens don’t fail with a misleading "Couldn't allocate memory" message.

	if (dev->interrupt_efd < 0) {
		int saved_errno = errno;
		free(dev);
		errno = saved_errno;

`new_hid_device()` can now fail because `eventfd()` failed (e.g., EMFILE/ENFILE/EPERM), but the caller (`hid_open_path`) still treats any `NULL` return as an allocation failure and overwrites `errno`/global error accordingly. Consider preserving the real `errno` from `eventfd()` and reporting a more accurate error (or providing a fallback wakeup mechanism) so device opens don’t fail with a misleading "Couldn't allocate memory" message. ```suggestion if (dev->interrupt_efd < 0) { int saved_errno = errno; free(dev); errno = saved_errno; ```

linux/hid.c

					
				@ -1146,1 +1159,4 @@

						return -1;

					}

					int bytes_read;

In the open-failure path, calling hid_close(dev) before formatting the error will clobber errno because hid_close() unconditionally calls close(dev->device_handle) even when device_handle == -1. That makes the reported error message likely show EBADF instead of the real open() failure. Capture/restore errno around hid_close(), or make hid_close() skip close() when the handle is negative.

In the open-failure path, calling `hid_close(dev)` before formatting the error will clobber `errno` because `hid_close()` unconditionally calls `close(dev->device_handle)` even when `device_handle == -1`. That makes the reported error message likely show `EBADF` instead of the real `open()` failure. Capture/restore `errno` around `hid_close()`, or make `hid_close()` skip `close()` when the handle is negative.

netbsd/hid.c

					
				@ -52,9 +52,15 @@ struct hid_device_ {

					wchar_t *last_read_error_str;

dev->interrupted is read in hid_read_timeout() and written in hid_read_interrupt()/hid_read_clear_interrupt() without any synchronization. Marking it volatile does not make these operations atomic or race-free; this is undefined behavior if hid_read_interrupt() is called from another thread, which is the main purpose of the new API. Switch to an atomic flag or guard access with a mutex (and keep the pipe strictly for wakeups).

`dev->interrupted` is read in `hid_read_timeout()` and written in `hid_read_interrupt()`/`hid_read_clear_interrupt()` without any synchronization. Marking it `volatile` does not make these operations atomic or race-free; this is undefined behavior if `hid_read_interrupt()` is called from another thread, which is the main purpose of the new API. Switch to an atomic flag or guard access with a mutex (and keep the pipe strictly for wakeups).

windows/hid.c

Spelling/grammar in the comment: “enought” → “enough”, and “very common” likely intended as “common”.

	// TODO: common but not the most efficient way to check this;
	// move to C11 atomics when the time comes.
	// For now this is portable and efficient enough.

Spelling/grammar in the comment: “enought” → “enough”, and “very common” likely intended as “common”. ```suggestion // TODO: common but not the most efficient way to check this; // move to C11 atomics when the time comes. // For now this is portable and efficient enough. ```

copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed

2026-05-05 11:26:11 -05:00

copilot-pull-request-reviewer[bot] (Migrated from github.com) left a 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.

## Pull request overview Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments. --- 💡 <a href="/libusb/hidapi/new/master?filename=.github/instructions/*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.

CMakeLists.txt

					
				@ -96,3 +107,3 @@

				    if(NOT MSVC)

				        # MSVC doesn't recognize those options, other compilers - requiring it

				        foreach(HIDAPI_TARGET hidapi_winapi hidapi_darwin hidapi_hidraw hidapi_libusb hidtest_hidraw hidtest_libusb hidtest)

				        foreach(HIDAPI_TARGET hidapi_winapi hidapi_darwin hidapi_hidraw hidapi_libusb hidtest_hidraw hidtest_libusb hidtest hid_read_test_hidraw hid_read_test_libusb hid_read_test test_read_interrupt)

When HIDAPI_ENABLE_ASAN=ON, the new hid_read_test* targets are not added to the sanitizer target list below, unlike the existing example/test executables. That makes this new shutdown test tool much less useful for catching the memory bugs this PR could introduce.

When `HIDAPI_ENABLE_ASAN=ON`, the new `hid_read_test*` targets are not added to the sanitizer target list below, unlike the existing example/test executables. That makes this new shutdown test tool much less useful for catching the memory bugs this PR could introduce.

CMakeLists.txt

					
				@ -97,3 +108,3 @@

				        # MSVC doesn't recognize those options, other compilers - requiring it

				        foreach(HIDAPI_TARGET hidapi_winapi hidapi_darwin hidapi_hidraw hidapi_libusb hidtest_hidraw hidtest_libusb hidtest)

				        foreach(HIDAPI_TARGET hidapi_winapi hidapi_darwin hidapi_hidraw hidapi_libusb hidtest_hidraw hidtest_libusb hidtest hid_read_test_hidraw hid_read_test_libusb hid_read_test test_read_interrupt)

				            if(TARGET ${HIDAPI_TARGET})

This only wires the new tool into the CMake build. The other supported source build paths still do not reference hid_read_test (for example Makefile.am:42, configure.ac:249, and the Meson wrapper at meson.build:1-23), so the PR's new tool is not actually buildable or distributable outside the CMake path.

This only wires the new tool into the CMake build. The other supported source build paths still do not reference `hid_read_test` (for example `Makefile.am:42`, `configure.ac:249`, and the Meson wrapper at `meson.build:1-23`), so the PR's new tool is not actually buildable or distributable outside the CMake path.

hid_read_test/main.cpp

					
				@ -0,0 +1,172 @@

				/*******************************************************

This is documented as async-signal-safe, but std::atomic<bool>::store is only safe in a signal handler when the atomic is lock-free, which is not guaranteed for atomic<bool>. On targets where it falls back to an internal lock, pressing Ctrl+C can deadlock inside the handler.

This is documented as async-signal-safe, but `std::atomic<bool>::store` is only safe in a signal handler when the atomic is lock-free, which is not guaranteed for `atomic<bool>`. On targets where it falls back to an internal lock, pressing Ctrl+C can deadlock inside the handler.

hid_read_test/main.cpp

					
				@ -0,0 +132,4 @@

				#ifdef _WIN32

				    std::signal(SIGINT, on_signal);

				#ifdef SIGTERM

				    std::signal(SIGTERM, on_signal);

The Windows signal handler only sets g_terminate, but that flag is never observed anywhere and this branch does not have the POSIX sigaction(..., sa_flags = 0) logic that makes std::cin.get() return on a signal. In practice Ctrl+C can leave the main thread stuck in cin.get() and never call hid_read_interrupt(), so the advertised shutdown path is unreliable on Windows.

The Windows signal handler only sets `g_terminate`, but that flag is never observed anywhere and this branch does not have the POSIX `sigaction(..., sa_flags = 0)` logic that makes `std::cin.get()` return on a signal. In practice `Ctrl+C` can leave the main thread stuck in `cin.get()` and never call `hid_read_interrupt()`, so the advertised shutdown path is unreliable on Windows.

hidapi/hidapi.h

					
				@ -423,6 +423,91 @@ extern "C" {

						*/

These docs say hid_read_interrupt() is the only hid_device function that may be called concurrently with another device operation, but hid_is_read_interrupted() is immediately documented below as suitable for cross-thread observation. That makes the new thread-safety contract self-contradictory for callers.

These docs say `hid_read_interrupt()` is the only `hid_device` function that may be called concurrently with another device operation, but `hid_is_read_interrupted()` is immediately documented below as suitable for cross-thread observation. That makes the new thread-safety contract self-contradictory for callers.

All five backends implement this as a pure flag read and only ever return 0 or 1; there is no error path and no backend sets hid_error(dev) here. Documenting -1 on error as part of the public contract is therefore misleading for API consumers.

All five backends implement this as a pure flag read and only ever return 0 or 1; there is no error path and no backend sets `hid_error(dev)` here. Documenting `-1 on error` as part of the public contract is therefore misleading for API consumers.

linux/hid.c

					
				@ -1153,2 +1164,2 @@

						   properly report device disconnection through read() when

						   in non-blocking mode.  */

					{

						/* Always poll() - even for a blocking read (milliseconds < 0) -

This only polls the interrupt eventfd when milliseconds >= 0. A normal blocking hid_read()/hid_read_timeout(..., -1) still goes straight to read(), so hid_read_interrupt() cannot wake the common blocking-read path on the hidraw backend and the reader thread will keep hanging until the device produces input or disconnects.

This only polls the interrupt eventfd when `milliseconds >= 0`. A normal blocking `hid_read()`/`hid_read_timeout(..., -1)` still goes straight to `read()`, so `hid_read_interrupt()` cannot wake the common blocking-read path on the hidraw backend and the reader thread will keep hanging until the device produces input or disconnects.

windows/hid.c

Returning here leaves the overlapped ReadFile owning dev->ol and dev->read_buf after the reader thread exits. The documented hid_read_interrupt() -> join reader -> hid_close() flow is then unsafe on Windows, because hid_close() still uses CancelIo, which only cancels I/O issued by the calling thread, so the pending read started on the reader thread may still be in flight while those buffers are being freed.

Returning here leaves the overlapped `ReadFile` owning `dev->ol` and `dev->read_buf` after the reader thread exits. The documented `hid_read_interrupt()` -> join reader -> `hid_close()` flow is then unsafe on Windows, because `hid_close()` still uses `CancelIo`, which only cancels I/O issued by the calling thread, so the pending read started on the reader thread may still be in flight while those buffers are being freed.

archiver-bot added 1 commit

2026-10-11 01:49:58 -05:00

Merge branch 'master' into read-interrupt bef57f77ea

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

This pull request is marked as a work in progress.

View command line instructions

Checkout

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

git fetch -u origin 'read-interrupt:read-interrupt'

git switch 'read-interrupt'

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

git merge --no-ff 'read-interrupt'

git switch 'read-interrupt'

git rebase 'master'

git switch 'master'

git merge --ff-only 'read-interrupt'

git switch 'read-interrupt'

git rebase 'master'

git switch 'master'

git merge --no-ff 'read-interrupt'

git switch 'master'

git merge --squash 'read-interrupt'