Skip to content

Fix signal handling and add atomic operations for thread safety#800

Merged
Youw merged 1 commit intoread-interruptfrom
claude/review-pr-799-fixes-G5tbo
May 5, 2026
Merged

Fix signal handling and add atomic operations for thread safety#800
Youw merged 1 commit intoread-interruptfrom
claude/review-pr-799-fixes-G5tbo

Conversation

@Youw
Copy link
Copy Markdown
Member

@Youw Youw commented May 5, 2026

Summary

This PR improves thread safety and signal handling across the HIDAPI library by replacing volatile variables with proper atomic operations and refactoring signal handling to be async-signal-safe.

Key Changes

Signal Handling Improvements

  • hid_read_test/main.cpp: Refactored signal handling to be async-signal-safe by:
    • Replacing direct hid_read_interrupt() calls in signal handler with atomic flag setting
    • Using sigaction() on non-Windows platforms without SA_RESTART to allow cin.get() to return on signal
    • Keeping std::signal() for Windows compatibility
    • Added conditional compilation for signal.h inclusion (Windows doesn't need it)

Atomic Operations

  • linux/hid.c, netbsd/hid.c: Replaced volatile int interrupted with proper atomic operations:
    • Changed dev->interrupted = 1 to __atomic_store_n(&dev->interrupted, 1, __ATOMIC_RELEASE)
    • Changed if (dev->interrupted) to if (__atomic_load_n(&dev->interrupted, __ATOMIC_ACQUIRE))
    • Changed reads to use __atomic_load_n(&dev->interrupted, __ATOMIC_ACQUIRE)
    • This ensures proper memory ordering and thread synchronization

Bug Fixes

  • linux/hid.c: Fixed potential errno corruption in new_hid_device() by saving errno before free() call
  • linux/hid.c: Improved error handling in hid_open_path() to preserve and report the actual error from new_hid_device()
  • linux/hid.c: Added safety check in hid_close() to only close device_handle if it's valid (>= 0)
  • windows/hid.c: Fixed typos in comments

Build System

  • CMakeLists.txt:
    • Lowered minimum CMake version from 3.8 to 3.1.3 for broader compatibility
    • Replaced target_compile_features() with set_target_properties() for more explicit C++ standard configuration (CXX_STANDARD, CXX_STANDARD_REQUIRED, CXX_EXTENSIONS)

Implementation Details

The atomic operations use acquire/release semantics to ensure proper synchronization between the signal handler thread and the main reading thread without requiring full sequential consistency, which improves performance while maintaining correctness.

https://claude.ai/code/session_01JB9WGTp5xipxBFMgcJa8XT

- windows/hid.c: fix typos in hid_is_read_interrupted comment.
- linux/hid.c: skip close() in hid_close() when device_handle < 0
  to avoid clobbering errno on the open-failure path.
- linux/hid.c: preserve eventfd() errno across free() in
  new_hid_device(); report the real errno from hid_open_path()
  instead of overwriting with ENOMEM.
- linux/hid.c, netbsd/hid.c: replace `volatile int interrupted`
  with __atomic_load_n/__atomic_store_n (acquire/release) so the
  cross-thread guarantee actually holds. The pipe/eventfd handles
  the wakeup; the atomic covers hid_is_read_interrupted and the
  early-out check.
- hid_read_test/main.cpp: signal handler no longer calls
  hid_read_interrupt() (not async-signal-safe on libusb/mac).
  Handler now sets an atomic flag; main thread, woken by EINTR
  from cin.get(), calls hid_read_interrupt(). Use sigaction
  without SA_RESTART on POSIX.
- hid_read_test/CMakeLists.txt: lower cmake_minimum_required to
  3.1.3 to match the rest of the repo; switch from
  target_compile_features(cxx_std_11) (CMake 3.8+) to
  set_target_properties(CXX_STANDARD 11) (CMake 3.1+).

https://claude.ai/code/session_01JB9WGTp5xipxBFMgcJa8XT
@Youw Youw merged commit 17fcf9a into read-interrupt May 5, 2026
20 of 21 checks passed
@Youw Youw deleted the claude/review-pr-799-fixes-G5tbo branch May 5, 2026 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants