Reduced network polling and unblocked bot stop#14
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 13 minutes and 1 second. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors packet processing to a signal-driven dispatcher in NetBase, updates Client/Server to signal the dispatcher, adds a CancelAction(int timeout) with immediate-send behavior for timeout<=0, and rewrites Bot start/stop to an async worker with coordinated shutdown and repeated immediate cancel attempts. ChangesSignal-Driven Packet Processing Architecture
Sequence Diagram(s)sequenceDiagram
participant Client/Server
participant Protocol
participant NetBase
participant DispatcherThread
participant Socket
Client/Server->>Protocol: Recv()/Send(packet)
Protocol->>NetBase: Enqueue packet / complete send
NetBase->>DispatcherThread: SignalPacketDispatcher()
DispatcherThread->>NetBase: ProcessQueuedPackets()
NetBase->>Socket: _socket.Send(outgoing) // only if connected & dispatch enabled
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Library/RSBot.Core/Bot.cs (1)
56-79:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix Bot.Start/Stop lifecycle race and async
Task.Factory.StartNewusage inLibrary/RSBot.Core/Bot.cs
Stop()can return early becauseRunningis set only inside the scheduled delegate; a Start→Stop right afterStart()can leave the newly createdTokenSourceuncanceled and the worker running.- The loop checks the mutable
TokenSourcefield, and bothTask.Delay(100)calls are not cancellation-aware, so a stop/start can swapTokenSourcewhile an old loop is mid-delay, allowing multiple loops to tick.Task.Factory.StartNew(async ...)with an async lambda is a known pitfall: it produces a nested task shape that should beUnwrap’d,LongRunningonly applies to the synchronous portion before the firstawait, and the returned task being ignored can hide exceptions/teardown.Suggested fix
- TokenSource = new CancellationTokenSource(); - - Task.Factory.StartNew( - async e => + var tokenSource = new CancellationTokenSource(); + TokenSource = tokenSource; + Running = true; + + _ = Task.Run( + async () => { - Running = true; + var token = tokenSource.Token; EventManager.FireEvent("OnStartBot"); Botbase.Start(); - while (!TokenSource.IsCancellationRequested) + while (!token.IsCancellationRequested) { if (!Game.Ready) { - await Task.Delay(100); + await Task.Delay(100, token); continue; } Botbase.Tick(); - await Task.Delay(100); + await Task.Delay(100, token); } - }, - TokenSource.Token, - TaskCreationOptions.LongRunning + }, + tokenSource.Token );🤖 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/Bot.cs` around lines 56 - 79, Start/Stop races and misuse of Task.Factory.StartNew are causing orphaned background loops and lost exceptions: set Running true before scheduling, create and store a CancellationTokenSource and the resulting worker Task (e.g., TokenSource and WorkerTask) atomically, capture the token in a local variable inside the worker (var token = TokenSource.Token) and use cancellation-aware waits (await Task.Delay(100, token)), avoid async void/nested Task by using Task.Run(async token => { ... }, token) or Task.Factory.StartNew(() => WorkerLoopAsync(token), token).Ensure Stop() cancels the TokenSource, awaits WorkerTask to completion, and clears Running only after the worker finishes; also reference Botbase.Tick, EventManager.FireEvent("OnStartBot") and check token.IsCancellationRequested in the loop to exit promptly so a Stop→Start cannot leave old loops running.
🤖 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/SkillManager.cs`:
- Around line 639-643: The CancelAction fast-path sends the 0xB074 cancel packet
and returns without registering an AwaitCallback, but 0xB074 responses are
currently matched purely by ResponseOpcode and can satisfy unrelated waiters
(e.g., CastSkillOld/CastSkillAt/CastBuff/SpawnedItem.Pickup); modify
CancelAction (and the AwaitCallback registration for 0xB074) so responses are
request-correlated instead of opcode-only — e.g., include the action/skill id or
a unique request token from the outgoing packet in the AwaitCallback predicate
(or attach it to the AwaitCallback as a filter) so only callbacks whose request
id matches the incoming 0xB074 packet are completed, or alternatively serialize
0xB074 waiters by ensuring only one outstanding 0xB074 waiter at a time in
SkillManager to prevent cross-completion.
In `@Library/RSBot.Core/Network/Socket/NetBase.cs`:
- Around line 149-159: The dispatcher thread is permanently lost after a
transient disconnect because Server.Disconnect() flips IsClosing true causing
the loop in NetBase.Dispatcher to exit but never joins/clears _dispatcherThread,
so StartNetWorker() won't start a new one; fix this by ensuring the dispatcher
lifecycle is properly cleaned up and recreated: in the disconnect/shutdown path
(Server.Disconnect()/NetBase) join/abort the running _dispatcherThread and set
_dispatcherThread = null (or otherwise signal and wait for thread exit) so
StartNetWorker() can create a fresh thread, or alternatively adjust
StartNetWorker() to check thread.IsAlive before skipping creation; reference
symbols: _dispatcherThread, IsClosing, Server.Disconnect(), StartNetWorker(),
ProcessQueuedPackets(), _packetSignal.
---
Outside diff comments:
In `@Library/RSBot.Core/Bot.cs`:
- Around line 56-79: Start/Stop races and misuse of Task.Factory.StartNew are
causing orphaned background loops and lost exceptions: set Running true before
scheduling, create and store a CancellationTokenSource and the resulting worker
Task (e.g., TokenSource and WorkerTask) atomically, capture the token in a local
variable inside the worker (var token = TokenSource.Token) and use
cancellation-aware waits (await Task.Delay(100, token)), avoid async void/nested
Task by using Task.Run(async token => { ... }, token) or
Task.Factory.StartNew(() => WorkerLoopAsync(token), token).Ensure Stop() cancels
the TokenSource, awaits WorkerTask to completion, and clears Running only after
the worker finishes; also reference Botbase.Tick,
EventManager.FireEvent("OnStartBot") and check token.IsCancellationRequested in
the loop to exit promptly so a Stop→Start cannot leave old loops running.
🪄 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: 88d3f549-841b-481b-a1b4-3067fbe0bc98
📒 Files selected for processing (6)
Botbases/RSBot.Training/TrainingBase.csLibrary/RSBot.Core/Bot.csLibrary/RSBot.Core/Components/SkillManager.csLibrary/RSBot.Core/Network/Socket/Client.csLibrary/RSBot.Core/Network/Socket/NetBase.csLibrary/RSBot.Core/Network/Socket/Server.cs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Bot.cs`:
- Around line 135-151: CancelActionOnStop can throw a NullReferenceException
because Game.Player may become null while the background Task checks
Game.Player.InAction; update the async loop in CancelActionOnStop to safely
handle a null player by capturing a local reference or using a null-conditional
check (e.g., check Game?.Player != null or use Game.Player?.InAction) before
accessing InAction, and keep the existing Running and
SkillManager.CancelAction(0) behavior so the loop returns when the bot is
running or there is no player.
🪄 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: cf03f317-f927-4f5f-9d9c-2960d455fdf9
📒 Files selected for processing (6)
Botbases/RSBot.Training/TrainingBase.csLibrary/RSBot.Core/Bot.csLibrary/RSBot.Core/Components/SkillManager.csLibrary/RSBot.Core/Network/Socket/Client.csLibrary/RSBot.Core/Network/Socket/NetBase.csLibrary/RSBot.Core/Network/Socket/Server.cs
💤 Files with no reviewable changes (1)
- Botbases/RSBot.Training/TrainingBase.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- Library/RSBot.Core/Components/SkillManager.cs
- Library/RSBot.Core/Network/Socket/NetBase.cs
kis1yi
merged commit a9deb4c
into
Silkroad-Developer-Community:main
1 of 2 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Performance Improvements
Bug Fixes