Fixed race on xigncode#16
Conversation
thanks to e1N
📝 WalkthroughWalkthroughThe PR refactors the special-client startup sequence in ChangesClient Startup Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Library/RSBot.Core/Components/ClientManager.cs (1)
219-220: ⚡ Quick winAvoid reallocating the full module buffer on every poll.
With the 8 ms slice on Line 219,
ModuleContains()currently allocates a fresh module-sized buffer on each iteration. For multi-MB clients, one 5 s wait can churn through hundreds of large allocations in the startup hot path. Reuse a caller-owned buffer and only resize it whenModuleMemorySizechanges.Also applies to: 253-259
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Library/RSBot.Core/Components/ClientManager.cs` around lines 219 - 220, The polling loop presently allocates a new module-sized buffer every slice (see sliceMs/maxWaitMs constants) inside ModuleContains(), causing heavy allocation churn for large ModuleMemorySize values; change ModuleContains and its callers in ClientManager to accept a caller-owned byte[] buffer (reuseBuffer) that is allocated once and only resized when ModuleMemorySize changes, pass that buffer into the read/compare logic instead of creating a new array each iteration, and apply the same reuse pattern to the other allocation sites referenced around the 253-259 region so the buffer is grown lazily only when ModuleMemorySize increases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Library/RSBot.Core/Components/ClientManager.cs`:
- Around line 111-112: Change the logic so WaitForXigncodeUnpack returns an
explicit status (bool or enum) and Start() checks that status before calling
ApplyXigncodePatch; if the unpack wait returned failure (timeout or process
exit) then perform the same abort/cleanup path and return false instead of
proceeding to ApplyXigncodePatch. Update any checks around HasExited/timeout in
the block currently around lines 231-240 to use the new status semantics so
StartClientProcess/GeneralManager.StartClientProcess observes false on failure.
Ensure the unique symbols WaitForXigncodeUnpack, ApplyXigncodePatch, and Start
(or GeneralManager.StartClientProcess caller) are updated to propagate and
handle the returned status consistently.
---
Nitpick comments:
In `@Library/RSBot.Core/Components/ClientManager.cs`:
- Around line 219-220: The polling loop presently allocates a new module-sized
buffer every slice (see sliceMs/maxWaitMs constants) inside ModuleContains(),
causing heavy allocation churn for large ModuleMemorySize values; change
ModuleContains and its callers in ClientManager to accept a caller-owned byte[]
buffer (reuseBuffer) that is allocated once and only resized when
ModuleMemorySize changes, pass that buffer into the read/compare logic instead
of creating a new array each iteration, and apply the same reuse pattern to the
other allocation sites referenced around the 253-259 region so the buffer is
grown lazily only when ModuleMemorySize increases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ff6cad49-63c7-440b-b06c-1dab4414dfc7
📒 Files selected for processing (1)
Library/RSBot.Core/Components/ClientManager.cs
| WaitForXigncodeUnpack(sroProcess, pi.hThread); | ||
| ApplyXigncodePatch(sroProcess, pi); |
There was a problem hiding this comment.
Differentiate unpack success from timeout/exit.
Line 111 treats every return from WaitForXigncodeUnpack() as success, but Lines 231-240 also return on HasExited and timeout. That means Start() can still call ApplyXigncodePatch() on a dead process or fall through the success path even though the XIGNCODE wait failed, which bypasses the false-return contract that GeneralManager.StartClientProcess() is relying on. Return a status from the helper and abort/cleanup on failure instead of patching unconditionally.
Also applies to: 231-240
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Library/RSBot.Core/Components/ClientManager.cs` around lines 111 - 112,
Change the logic so WaitForXigncodeUnpack returns an explicit status (bool or
enum) and Start() checks that status before calling ApplyXigncodePatch; if the
unpack wait returned failure (timeout or process exit) then perform the same
abort/cleanup path and return false instead of proceeding to ApplyXigncodePatch.
Update any checks around HasExited/timeout in the block currently around lines
231-240 to use the new status semantics so
StartClientProcess/GeneralManager.StartClientProcess observes false on failure.
Ensure the unique symbols WaitForXigncodeUnpack, ApplyXigncodePatch, and Start
(or GeneralManager.StartClientProcess caller) are updated to propagate and
handle the returned status consistently.
1 participant
thanks to e1N
Summary by CodeRabbit