Skip to content

fix: back reader/writer locks with a shared SRWLOCK wrapper#108

Merged
tkhquang merged 2 commits into
mainfrom
fix/srw-shared-mutex
Jun 11, 2026
Merged

fix: back reader/writer locks with a shared SRWLOCK wrapper#108
tkhquang merged 2 commits into
mainfrom
fix/srw-shared-mutex

Conversation

@tkhquang

@tkhquang tkhquang commented Jun 11, 2026

Copy link
Copy Markdown
Owner

Summary

  • Extracts the file-local SRWLOCK wrapper from src/memory.cpp into a shared opaque detail::SrwSharedMutex (include/DetourModKit/srw_shared_mutex.hpp) usable from public headers without exposing Windows headers.
  • Migrates HookManager, InputPoller, and the module-range cache off std::shared_mutex, which on MinGW is backed by winpthreads' pthread_rwlock_t and corrupts internal state under high reader contention.
  • Fixes the unguarded InputPoller::binding_count() read (now taken under the binding reader lock) and stops InputManager::binding_count() from holding the lifecycle mutex across the poller call.
  • Adds a dedicated SRW wrapper test suite (reader overlap, try-lock failure modes, unlock release proof, blocked-writer handoff) and a concurrent query/reshape stress test for InputManager.
  • Updates AGENTS.md and README.md to match.

Summary by CodeRabbit

  • New Features

    • Added comprehensive synchronization testing for concurrent reader/writer scenarios.
    • Added new concurrent input binding test with cardinality updates during polling.
  • Documentation

    • Updated thread-safety model documentation with Windows-native locking terminology.
    • Enhanced input system documentation with synchronization details.
  • Refactor

    • Improved internal synchronization primitives for enhanced performance and consistency across hook and input management subsystems.

@tkhquang tkhquang self-assigned this Jun 11, 2026
@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@tkhquang, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 38 minutes and 43 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more credits in the billing tab to continue.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 7074a031-de38-49a7-a8d7-9fe0ca8526c1

📥 Commits

Reviewing files that changed from the base of the PR and between dab5573 and d0ab93d.

📒 Files selected for processing (1)
  • CMakeLists.txt
📝 Walkthrough

Walkthrough

This PR introduces SrwSharedMutex, a Windows SRWLOCK-backed shared mutex wrapper, and systematically replaces std::shared_mutex throughout the codebase in HookManager, InputPoller/InputManager, and ModuleRangeCache. New concurrency tests validate the wrapper's behavior, and documentation is updated to reflect the SRWLOCK-based synchronization model.

Changes

SrwSharedMutex introduction and migration

Layer / File(s) Summary
SrwSharedMutex header declaration
include/DetourModKit/srw_shared_mutex.hpp
Declares non-copyable, non-movable SrwSharedMutex wrapper supporting std::shared_lock/std::unique_lock with exclusive lock/try_lock/unlock and shared lock_shared/try_lock_shared/unlock_shared methods, backed by pointer-sized aligned storage for the SRWLOCK state.
SrwSharedMutex Windows SRWLOCK implementation
src/srw_shared_mutex.cpp
Implements SrwSharedMutex by wrapping Windows SRWLOCK APIs with placement-new constructor, exclusive lock/try_lock/unlock using AcquireSRWLockExclusive/ReleaseSRWLockExclusive, shared lock_shared/try_lock_shared/unlock_shared using AcquireSRWLockShared/ReleaseSRWLockShared, plus layout and trivial-destructibility assertions.
HookManager header: replace std::shared_mutex with SrwSharedMutex
include/DetourModKit/hook_manager.hpp
Updates header includes, changes m_hooks_mutex and m_mutator_gate member types from std::shared_mutex to detail::SrwSharedMutex, replaces all std::shared_lock<std::shared_mutex> and std::unique_lock<std::shared_mutex> with SrwSharedMutex equivalents in with_vmt_method, with_inline_hook, with_mid_hook, try_with_mid_hook, and lock_hooks_shared_reentrant helper; refines callback execution and reentrancy documentation to reference SRW reader lock semantics.
HookManager implementation: replace std::shared_mutex with SrwSharedMutex
src/hook_manager.cpp
Updates destructor, shutdown, and all hook/VMT mutation methods (create_inline_hook, create_mid_hook, remove_hook, remove_all_hooks, enable/disable_hook/hooks/all_hooks, create/remove/apply/remove_vmt_hook, remove_vmt_method, remove_vmt_from_object, remove_all_vmt_hooks) to acquire locks on SrwSharedMutex instead of std::shared_mutex while preserving double-checked shutdown patterns, two-phase disable/clear sequences, and reentrancy guard handling.
InputPoller/InputManager: replace std::shared_mutex with SrwSharedMutex
include/DetourModKit/input.hpp, src/input.cpp
Changes InputPoller's m_bindings_rw_mutex from std::shared_mutex to detail::SrwSharedMutex; updates binding_count() to acquire shared lock when reading binding count; refines InputManager::binding_count() to capture poller under mutex then call poller method outside lock; updates documentation describing lock coverage during binding-evaluation pass.
ModuleRangeCache: replace std::shared_mutex with SrwSharedMutex
src/memory.cpp
Includes SrwSharedMutex header, removes local SrwSharedMutex class definition, replaces ModuleRangeCache's m_cache_mutex from std::shared_mutex to SrwSharedMutex, updates module_range_for read and write lock types from std::shared_lock<std::shared_mutex>/std::unique_lock<std::shared_mutex> to SrwSharedMutex equivalents.
SrwSharedMutex concurrency test suite
tests/test_srw_shared_mutex.cpp
Introduces five concurrency-focused test cases: validates shared readers overlap and try-lock succeeds while held, exclusive try-lock fails under shared reader, shared try-lock fails under exclusive writer, try-lock/unlock and try_lock_shared/unlock_shared succeed in correct sequence, and blocked writer acquires only after reader releases; includes static_asserts enforcing non-copyable/non-movable contract.
Input binding cardinality concurrency regression test
tests/test_input.cpp
Adds multi-threaded test concurrently executing rapid update_binding_combos() mutations with varying cardinality while reader threads poll binding_count() and is_binding_active(), tracking any out-of-bounds cardinality observations to verify data-race-free behavior under SrwSharedMutex synchronization.
Documentation and test comments
AGENTS.md, README.md, tests/test_hook_integration.cpp
Adds SrwSharedMutex to AGENTS.md public headers list and test suite overview; updates thread safety model table to use SRWLOCK reader/writer terminology; clarifies performance-critical paths and synchronization boundaries; updates README.md input system section to document SRWLOCK-backed binding reshape synchronization; refines QueryAccessorsAreReentrantFromCallback test comment wording.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • tkhquang/DetourModKit#105: Modifies HookManager shutdown and mutator gate locking patterns in src/hook_manager.cpp that directly overlap with this PR's lock type migration.
  • tkhquang/DetourModKit#106: Updates HookManager's lock_hooks_shared_reentrant() reentrancy-aware helper that this PR also modifies for SrwSharedMutex integration.
  • tkhquang/DetourModKit#69: Modifies HookManager shutdown/destructor concurrency logic for the same m_mutator_gate and m_hooks_mutex that this PR migrates to SrwSharedMutex.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: replacing std::shared_mutex with a shared SRWLOCK wrapper across HookManager, InputPoller, and other components.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@tkhquang tkhquang merged commit 4707f86 into main Jun 11, 2026
4 checks passed
@tkhquang tkhquang deleted the fix/srw-shared-mutex branch June 11, 2026 18:27
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.

1 participant