libusb: Fix high CPU usage in callback thread #783

2026-03-30T04:30:08-05:00

Copilot commented

2026-03-30 04:30:08 -05:00

(Migrated from github.com)

Callback_thread used hidapi_thread_cond_timedwait with an absolute
timespec already in the past, causing a tight spin loop. Replaced
with cond_wait + predicate pattern so the thread sleeps until an
event or a shutdown signal arrives.
Multithreading fixes folded in along the way:

Queue mutex moved from libusb_thread to callback_thread (its
natural condvar partner), with signal-under-mutex to avoid a
lost-wakeup race at shutdown.
process_hotplug_event() now runs under hid_hotplug_context.mutex
to match hid_hotplug_register_callback()'s HID_API_HOTPLUG_ENUMERATE
traversal of hid_hotplug_context.devs — fixes a use-after-free
where a concurrent disconnect could free a list node mid-traversal.
devs teardown moved to hid_internal_hotplug_cleanup() (after
hidapi_thread_join) so every access to devs happens under
hid_hotplug_context.mutex.
hid_hotplug_register_callback() now frees devs on the
libusb_hotplug_register_callback() error path, which previously
leaked.

Closes: #782

Assisted-by: Copilot:claude-sonnet-4.6
Assisted-by: Copilot:claude-opus-4.6
Assisted-by: Claude:claude-opus-4.7

Callback_thread used hidapi_thread_cond_timedwait with an absolute timespec already in the past, causing a tight spin loop. Replaced with cond_wait + predicate pattern so the thread sleeps until an event or a shutdown signal arrives. Multithreading fixes folded in along the way: - Queue mutex moved from libusb_thread to callback_thread (its natural condvar partner), with signal-under-mutex to avoid a lost-wakeup race at shutdown. - process_hotplug_event() now runs under hid_hotplug_context.mutex to match hid_hotplug_register_callback()'s HID_API_HOTPLUG_ENUMERATE traversal of hid_hotplug_context.devs — fixes a use-after-free where a concurrent disconnect could free a list node mid-traversal. - devs teardown moved to hid_internal_hotplug_cleanup() (after hidapi_thread_join) so every access to devs happens under hid_hotplug_context.mutex. - hid_hotplug_register_callback() now frees devs on the libusb_hotplug_register_callback() error path, which previously leaked. Closes: #782 Assisted-by: Copilot:claude-sonnet-4.6 Assisted-by: Copilot:claude-opus-4.6 Assisted-by: Claude:claude-opus-4.7

Youw (Migrated from github.com) reviewed

2026-03-30 04:48:19 -05:00

.gitignore

					
				@ -33,4 +33,6 @@ doxygen/html/

				# Visual Studio Code + CMake

				.vscode/


# Local build directory
build/

```suggestion # Local build directory build/ ```

Youw (Migrated from github.com) reviewed

2026-03-30 04:52:49 -05:00

libusb/hid.c

@copilot why do we need to send a signal with mutex locked?

copilot-swe-agent[bot] (Migrated from github.com) reviewed

2026-03-30 04:55:26 -05:00

libusb/hid.c

It prevents a lost wakeup that would cause a deadlock. Without the mutex, there's a race between the predicate check (line 1223) and cond_wait (line 1224):

callback_thread evaluates hotplug_cbs as non-NULL → enters inner while body
On another thread: deregister sets hotplug_cbs = NULL
hotplug_thread loop exits, sends cond_signal → no one is waiting yet, signal is lost
callback_thread enters cond_wait → blocks forever
hidapi_thread_join at line 1279 → deadlock

Holding the mutex ensures the signal is either delivered before cond_wait (predicate re-checked on next iteration) or after (thread wakes up).

It prevents a lost wakeup that would cause a deadlock. Without the mutex, there's a race between the predicate check (line 1223) and `cond_wait` (line 1224): 1. `callback_thread` evaluates `hotplug_cbs` as non-NULL → enters inner while body 2. On another thread: deregister sets `hotplug_cbs = NULL` 3. `hotplug_thread` loop exits, sends `cond_signal` → no one is waiting yet, signal is lost 4. `callback_thread` enters `cond_wait` → blocks forever 5. `hidapi_thread_join` at line 1279 → deadlock Holding the mutex ensures the signal is either delivered before `cond_wait` (predicate re-checked on next iteration) or after (thread wakes up).

Youw commented

2026-03-30 05:02:25 -05:00

(Migrated from github.com)

@copilot+claude-sonnet-4.6 We used to handle all evens in a queue while holding the lock on a mutex, and now we lock it only when manipulating the queue, but not when handling the event. Is there a benefit in that approach?

Youw (Migrated from github.com) reviewed

2026-03-30 05:03:30 -05:00

libusb/hid.c

@copilot+claude-opus-4.6 Shouldn't we also ensure the exit condition is set? I.e. hid_hotplug_context.hotplug_cbs = NULL? If it is already meet - need a hint/comment about that (but preferably set that condition explicitly).

@copilot+claude-opus-4.6 Shouldn't we also ensure the exit condition is set? I.e. `hid_hotplug_context.hotplug_cbs = NULL`? If it is already meet - need a hint/comment about that (but preferably set that condition explicitly).

copilot-swe-agent[bot] (Migrated from github.com) reviewed

2026-03-30 05:05:28 -05:00

libusb/hid.c

The exit condition is already met: the while (hid_hotplug_context.hotplug_cbs) loop in hotplug_thread only exits when hotplug_cbs is NULL, so by the time we reach that signal it is guaranteed to be NULL. The signal is needed solely to wake callback_thread from cond_wait so it can re-evaluate the (now-false) predicate and exit.

Added a comment to make this explicit in 5d7a7c6.

The exit condition is already met: the `while (hid_hotplug_context.hotplug_cbs)` loop in `hotplug_thread` only exits when `hotplug_cbs` is NULL, so by the time we reach that signal it is guaranteed to be NULL. The signal is needed solely to wake `callback_thread` from `cond_wait` so it can re-evaluate the (now-false) predicate and exit. Added a comment to make this explicit in 5d7a7c6.

Copilot commented

2026-03-30 05:05:39 -05:00

(Migrated from github.com)

@copilot+claude-sonnet-4.6 We used to handle all evens in a queue while holding the lock on a mutex, and now we lock it only when manipulating the queue, but not when handling the event. Is there a be...

Yes — releasing the lock during event processing avoids two problems:

Blocking the producer: hid_libusb_hotplug_callback (which runs on hotplug_thread) must acquire the same mutex to enqueue events. If we held the lock during the (potentially long) callback invocation, the producer would stall for that entire duration. Releasing it lets new events be enqueued concurrently while an older one is being dispatched.
Deadlock prevention: If user callback code calls back into HIDAPI (e.g., hid_hotplug_deregister_callback), that call will attempt to acquire callback_thread.mutex. Holding it during processing would deadlock.

The tradeoff is that hid_hotplug_context.devs (the known-device list maintained inside process_hotplug_event) is modified without the lock, but that is safe because only callback_thread reads or writes devs — events are still processed serially within the single consumer thread.

> @copilot+claude-sonnet-4.6 We used to handle all evens in a queue while holding the lock on a mutex, and now we lock it only when manipulating the queue, but not when handling the event. Is there a be... Yes — releasing the lock during event processing avoids two problems: 1. **Blocking the producer**: `hid_libusb_hotplug_callback` (which runs on `hotplug_thread`) must acquire the same mutex to enqueue events. If we held the lock during the (potentially long) callback invocation, the producer would stall for that entire duration. Releasing it lets new events be enqueued concurrently while an older one is being dispatched. 2. **Deadlock prevention**: If user callback code calls back into HIDAPI (e.g., `hid_hotplug_deregister_callback`), that call will attempt to acquire `callback_thread.mutex`. Holding it during processing would deadlock. The tradeoff is that `hid_hotplug_context.devs` (the known-device list maintained inside `process_hotplug_event`) is modified without the lock, but that is safe because only `callback_thread` reads or writes `devs` — events are still processed serially within the single consumer thread.

Youw commented

2026-03-30 05:17:07 -05:00

(Migrated from github.com)

@copilot+claude-sonnet-4.6 I also see that devs is accessed from hid_hotplug_register_callback, even though it is done before the thread is created, maybe move that initialisation inside of the thread (to match cleanup of that variable at the end of the thread).
And would be great to explicitly mark that variable that it should only be accessed from that thread.

@copilot+claude-sonnet-4.6 I also see that `devs` is accessed from `hid_hotplug_register_callback`, even though it is done before the thread is created, maybe move that initialisation inside of the thread (to match cleanup of that variable at the end of the thread). And would be great to explicitly mark that variable that it should only be accessed from that thread.

Copilot commented

2026-03-30 05:24:04 -05:00

(Migrated from github.com)

@copilot+claude-sonnet-4.6 I also see that devs is accessed from hid_hotplug_register_callback, even though it is done before the thread is created, maybe move that initialisation inside of the th...

Done in 3f4721e. devs is now initialized at the start of callback_thread (paired with its cleanup at the end), and its struct comment marks it as "Only accessed from callback_thread." The HID_API_HOTPLUG_ENUMERATE case in hid_hotplug_register_callback now enumerates devices locally (freed after use) so it never touches the thread-owned devs.

> @copilot+claude-sonnet-4.6 I also see that `devs` is accessed from `hid_hotplug_register_callback`, even though it is done before the thread is created, maybe move that initialisation inside of the th... Done in 3f4721e. `devs` is now initialized at the start of `callback_thread` (paired with its cleanup at the end), and its struct comment marks it as "Only accessed from callback_thread." The `HID_API_HOTPLUG_ENUMERATE` case in `hid_hotplug_register_callback` now enumerates devices locally (freed after use) so it never touches the thread-owned `devs`.

Youw commented

2026-03-30 12:20:02 -05:00

(Migrated from github.com)

@CalcProgrammer1 could you check this change in you environment regarding high CPU usage? it should give even better result.

Youw commented

2026-04-24 04:50:05 -05:00

(Migrated from github.com)

I think this PR has more than enought changes already. Will merge it like this and going to review/update more of this using Claude - it already spotted potential races and UB in current implementation.

👍 1

Youw (Migrated from github.com) approved these changes

2026-04-24 05:54:57 -05:00

Sign in to join this conversation.

No reviewers

Youw