Skip to content

Commit dd0848d

Browse files
authored
fix(input): harden poll-loop staging and subsystem robustness (#116)
1 parent f6cac43 commit dd0848d

11 files changed

Lines changed: 588 additions & 47 deletions

File tree

AGENTS.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -358,7 +358,7 @@ PATH="/c/msys64/mingw64/bin:$PATH" ./build/mingw-debug/tests/DetourModKit_tests.
358358
| HookManager | SRWLOCK-backed reader/writer locks; two-phase shutdown (disable under shared lock, clear under exclusive lock), both phases walking `m_hook_creation_order` in reverse so inline/mid hooks layered on one address unwind newest-first instead of in bucket order; `m_mutator_gate` blocks new mutators (including all VMT operations) during teardown; CAS on `m_shutdown_called` serializes shutdown/remove_all_hooks; double-checked fast-fail on `m_shutdown_called` in all mutators; every public mutation or teardown entry point also fails closed (`HookError::ReentrantCallRejected`, a false/zero result, or a logged no-op for void lifecycle calls) when the per-thread reentrancy guard is set, so a call from inside a `with_*` callback returns or no-ops instead of recursively acquiring the non-recursive lock; the batch toggles (`enable_hooks`/`disable_hooks`/`*_all`) collect their log lines under the lock and emit them after release so a synchronous sink flush or a blocking async overflow never stalls an exclusive acquirer; destructor fallback (when `DMK_Shutdown()` was not called) acquires `m_mutator_gate` exclusively, flips `m_shutdown_called`, drains readers via exclusive `m_hooks_mutex`, then clears the maps -- under loader lock it pins the module and swaps each map's contents into heap storage allocated via `new (std::nothrow)` so the storage outlives the destructor without ever draining, mirroring the leak-on-loader-lock discipline used in `Logger::shutdown_internal` and `ConfigWatcher::~ConfigWatcher` | `shared_lock` SRWLOCK reader for `with_inline_hook()` |
359359
| Logger | `atomic<shared_ptr>` for lock-free async reads; `shutdown_internal` and `disable_async_mode` are safe across repeated shutdown / enable_async_mode cycles: when the writer thread has to be detached under loader lock, the module is pinned and the `shared_ptr<AsyncLogger>` is moved into a per-call permanent cell (normal path: `new (std::nothrow)`; fallback path: non-CRT permanent storage), so a heap allocation failure cannot drop the last handle while the writer may still be running | Single atomic load on log level check |
360360
| AsyncLogger | Lock-free MPMC queue (Vyukov-style); post-join drain on shutdown (at most one message per producer can be lost in the nanosecond race between drain and force-zero -- accepted trade-off to avoid atomic overhead on every enqueue); timestamp caching in write batches | Atomic sequence numbers per slot |
361-
| InputPoller | Atomic `m_active_states[]` array | `shared_lock` (uncontended SRWLOCK reader) guarding the `m_active_states` pointer swap + `memory_order_relaxed` load per binding |
361+
| InputPoller | Atomic `m_active_states[]` array; the poll thread re-reserves its deferred-callback staging vector to the live binding count each cycle and stages it under a catch, so a runtime binding growth past the startup reserve cannot reallocate-then-throw out of the `jthread` body; a failed callback batch is dropped, not fatal | `shared_lock` (uncontended SRWLOCK reader) guarding the `m_active_states` pointer swap + `memory_order_relaxed` load per binding; keyboard/mouse reads route through a poll-thread-private per-cycle `KeyStateCache` so each distinct VK gets one coherent `GetAsyncKeyState` sample per cycle, not one call per binding reference |
362362
| InputManager | `mutex` for lifecycle, `atomic<shared_ptr<InputPoller>>` for reads | `atomic<shared_ptr<InputPoller>>` acquire-load, then the poller's `shared_lock` + relaxed load (not lock-free) |
363363
| InputIntercept (internal `src/input_intercept.*`) | File-scope atomics shared between the poll thread and the game's threads (XInput callers, window message thread); owns its safetyhook InlineHooks directly (not via HookManager) because the poll thread reads the trampoline and the hook lifetime is coupled to the poll thread; consume-until-release latch and wheel-pulse state are poll-thread-private; teardown skipped under loader lock (detours left installed against the pinned module) | Lock-free atomic loads in each detour; allocation-free, non-throwing detour bodies |
364364
| Memory cache | Sharded `SRWLOCK` + epoch-based shutdown | Shared reader locks per shard |
@@ -421,3 +421,5 @@ These are called at 60+ fps from game hook callbacks. Never add allocations, exc
421421
- **Do not key** a cache store and its invalidation/eviction by different addresses or shard-selection functions. Eviction must use the same canonical key and the same containment lookup as insertion and read, or entries silently survive invalidation (see `Memory::invalidate_range`, which scans every shard because storage is sharded by query address, not region base).
422422
- **Do not let** a queue or backlog fed at an external event rate grow without bound -- clamp the pending count to a documented ceiling (see `Input` wheel-notch `MAX_WHEEL_PENDING`).
423423
- **Do not let** an async/deferred sink diverge from its synchronous counterpart's configuration (timestamp format, level, etc.) -- carry the same settings through, as `Logger::enable_async_mode` does for the timestamp format.
424+
- **Do not open** a shared append-mode file with `GENERIC_WRITE` plus a one-time `SetFilePointer(FILE_END)` seek -- request `FILE_APPEND_DATA` so the OS positions every write at the current end of file atomically and concurrent appenders cannot interleave or overwrite each other (see `WinFileStreamBuf::open`). Truncating (`out`) mode keeps `GENERIC_WRITE` + `CREATE_ALWAYS`.
425+
- **Do not drop** the device source when formatting an off-table input code -- a non-keyboard code must serialize in the source-tagged hex form (`Mouse:0xNN`) that `parse_input_name` reads back, or it silently round-trips to a keyboard key (see `format_input_code` / `parse_input_name`).

README.md

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -259,6 +259,7 @@ See the [Config Hot-Reload Guide](docs/config-hot-reload/README.md) for the thre
259259
- Hash-map-backed `is_binding_active()` query for low-overhead cross-thread state reads (e.g., from render hooks at 60+ fps)
260260
- SRWLOCK-backed reader/writer synchronization for live binding reshapes, using the same native Windows lock primitive as hook registries and memory caches
261261
- Multiple bindings per name for multi-combo hotkeys
262+
- Keyboard and mouse virtual-key reads are memoized once per distinct VK per poll cycle, giving every binding in that cycle one coherent sample while avoiding duplicate `GetAsyncKeyState` calls
262263
- Lock-free `is_running()` via atomic flag
263264
- O(1) reverse name lookup for `input_code_to_name()`
264265
@@ -1007,13 +1008,15 @@ The configuration system recognizes the following named input codes (case-insens
10071008
| **Function keys** | `F1`-`F24` |
10081009
| **Navigation** | `Left`, `Right`, `Up`, `Down`, `Home`, `End`, `PageUp`, `PageDown`, `Insert`, `Delete` |
10091010
| **Common** | `Space`, `Enter`, `Escape`, `Tab`, `Backspace`, `CapsLock`, `NumLock`, `ScrollLock`, `PrintScreen`, `Pause` |
1011+
| **Windows / Menu** | `LWin`, `RWin`, `Apps` (alias `Menu`) |
1012+
| **OEM punctuation** | `Semicolon`, `Equals`, `Comma`, `Minus`, `Period`, `Slash`, `Grave` (aliases `Backtick`, `Tilde`; the usual console hotkey), `LBracket`, `Backslash`, `RBracket`, `Apostrophe` (alias `Quote`) |
10101013
| **Numpad** | `Numpad0`-`Numpad9`, `NumpadAdd`, `NumpadSubtract`, `NumpadMultiply`, `NumpadDivide`, `NumpadDecimal` |
10111014
| **Mouse** | `Mouse1` (left), `Mouse2` (right), `Mouse3` (middle), `Mouse4`, `Mouse5` |
10121015
| **Mouse wheel** | `WheelUp`, `WheelDown`, `WheelLeft`, `WheelRight` (trigger-only, Press mode) |
10131016
| **Gamepad** | `Gamepad_A`, `Gamepad_B`, `Gamepad_X`, `Gamepad_Y`, `Gamepad_LB`, `Gamepad_RB`, `Gamepad_LT`, `Gamepad_RT`, `Gamepad_Start`, `Gamepad_Back`, `Gamepad_LS`, `Gamepad_RS`, `Gamepad_DpadUp`, `Gamepad_DpadDown`, `Gamepad_DpadLeft`, `Gamepad_DpadRight` |
10141017
| **Gamepad sticks** | `Gamepad_LSUp`, `Gamepad_LSDown`, `Gamepad_LSLeft`, `Gamepad_LSRight`, `Gamepad_RSUp`, `Gamepad_RSDown`, `Gamepad_RSLeft`, `Gamepad_RSRight` |
10151018
1016-
Hex VK codes with `0x` prefix (e.g., `0x72` for F3) are also accepted and default to keyboard input.
1019+
Hex VK codes with `0x` prefix (e.g., `0x72` for F3) are also accepted and default to keyboard input. A code that has no table name but is not a keyboard code is written back to the INI in a source-tagged hex form (`Mouse:0xFE`, `Gamepad:0x800`, `MouseWheel:0x9`) and parsed back to the same device source, so a non-keyboard code survives a config round-trip instead of decaying to a keyboard key.
10171020
10181021
## Gamepad Compatibility
10191022

docs/tests/README.md

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -303,6 +303,18 @@ These tests enable the test-only `debug_snapshot_use_count()` accessor via `#def
303303

304304
The decoder header lives under `src/` (not the public include tree), so the test file adds `src/` to its include path and uses `DetourModKit::detail::` directly.
305305

306+
## Input Tests
307+
308+
`tests/test_input.cpp` covers public input-code parsing/formatting plus `InputPoller` / `InputManager` lifecycle and live binding reshapes. The poll-loop hot-path helpers are covered with focused tests because the real loop reads process input state:
309+
310+
| Test | What it proves |
311+
| ---- | -------------- |
312+
| `KeyStateCacheTest.ProbesEachDistinctVkOncePerCycle` | The per-cycle VK cache invokes the injected probe once for repeated reads of one VK, while distinct VKs get distinct samples. |
313+
| `KeyStateCacheTest.ResetReArmsForNextCycle` | `reset()` clears the cycle snapshot so the next poll cycle samples the VK again. |
314+
| `KeyStateCacheTest.CachesUpStateWithoutReProbing` | A released key is cached as a real sampled state, not confused with "not yet probed". |
315+
| `KeyStateCacheTest.OutOfRangeVkReadsAsNotPressedWithoutProbing` | Invalid VK codes fail closed and never call the probe. |
316+
| `InputPollerPollLoopSafety.BindingGrowthPastStartupReserveKeepsPollThreadAlive` | Live binding growth past the startup callback-staging reserve does not stop the poll thread. |
317+
306318
## Input Interception Tests
307319

308320
`tests/test_input_intercept.cpp` exercises the internal header `src/input_intercept.hpp`, the opt-in active-input layer that backs mouse-wheel capture and gamepad passthrough suppression for `InputPoller`. It uses the same `src/`-on-include-path pattern as the decoder tests. The unit tests target the two pure state machines that carry no Win32 dependency, so each is driven by direct calls with hand-supplied state:
@@ -332,6 +344,7 @@ The window-procedure subclass and the `XInputGetState` inline hook are exercised
332344
| `InterceptWndProcTest.ConsumeSwallowsOwnedWheelMessages` | With consume off the notch is latched and forwarded to the game's predecessor procedure; with consume on it is still latched but swallowed so the game never sees it. |
333345
| `InterceptWndProcTest.WmNcDestroySelfHealsAndAllowsResubclass` | Destroying the subclassed window marks the subclass uninstalled via `WM_NCDESTROY`, so a recreated window (the fullscreen-toggle case) can be re-subclassed. |
334346
| `InterceptWndProcTest.UninstallRestoresPredecessorAtTopOfChain` | When the detour is still the top of the window-procedure chain, `uninstall()` restores the exact saved predecessor. |
347+
| `InterceptWndProcTest.PollerDropsCallbackStagingCopyFailureAndContinues` | A wheel edge drives the poll-loop `PendingCallback` staging path with a callback whose copy throws; the failed callback batch is dropped, the poll thread stays alive, and a later edge dispatches normally. |
335348
| `InterceptXInputTest.InstallHooksExportAndTrampolineRoundTrips` | Installing hooks the real `XInputGetState` export, publishes a non-null trampoline, is idempotent, routes a call through the detour into the trampoline, and restores the prologue on `uninstall()`. |
336349
| `InterceptDisarmTest.PollerDisarmsWheelConsumeAfterClearBindings` | A standalone `InputPoller` with a consume wheel binding arms the swallow flag; `clear_bindings(false)` (the loader-lock-safe hot-reload reset) lets the poll loop disarm it on a later cycle so the game regains its wheel. |
337350

include/DetourModKit/input_codes.hpp

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -190,10 +190,13 @@ namespace DetourModKit
190190
*
191191
* Recognized name formats:
192192
* - Keyboard: "A"-"Z", "0"-"9", "F1"-"F24", "Ctrl", "Shift", "Alt",
193-
* "Space", "Enter", "Escape", "Tab", "Backspace", etc.
193+
* "Space", "Enter", "Escape", "Tab", "Backspace", Windows/menu keys ("LWin", "RWin", "Apps"), and OEM
194+
* punctuation ("Grave"/"Backtick"/"Tilde", "Semicolon", "Comma", "Period", "Slash", etc.)
194195
* - Mouse: "Mouse1" (left) through "Mouse5" (XButton2)
195196
* - Mouse wheel: "WheelUp", "WheelDown", "WheelLeft", "WheelRight"
196197
* - Gamepad: "Gamepad_A", "Gamepad_B", "Gamepad_LB", "Gamepad_LT", etc.
198+
* - Source-tagged hex (the inverse of format_input_code's off-table form): "Mouse:0xFE",
199+
* "Gamepad:0x800", "MouseWheel:0x9", "Keyboard:0xFF".
197200
*
198201
* @param name The input name to resolve.
199202
* @return std::optional<InputCode> The resolved code, or std::nullopt if unrecognized.
@@ -209,8 +212,10 @@ namespace DetourModKit
209212

210213
/**
211214
* @brief Formats an InputCode as a human-readable string.
212-
* @details Returns the canonical name if the code is in the lookup table, otherwise falls back to a hexadecimal
213-
* representation (e.g., "0x72").
215+
* @details Returns the canonical name if the code is in the lookup table. Off-table codes fall back to hex: a
216+
* Keyboard code emits bare hex ("0x72"), while any other source is tagged with its device name
217+
* ("Mouse:0xFE", "Gamepad:0x800") so the source is not lost and parse_input_name can reconstruct the same
218+
* code on a config round-trip. Untagged bare hex always parses back as Keyboard.
214219
* @param code The input code to format.
215220
* @return std::string Formatted string.
216221
*/

0 commit comments

Comments
 (0)