Fix LoginSession not disposing - #430
Conversation
|
Warning Rate limit exceeded@AngeloTadeucci has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 23 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis set of changes introduces heartbeat and disconnect mechanisms across the login, world, and channel services, involving updates to protobuf definitions, service implementations, and session management. Several new gRPC methods and message types are defined and implemented, allowing for heartbeat monitoring and explicit disconnection of sessions. The login and world servers are refactored to use these new RPCs, and session classes are updated to track tick and latency metrics. Additionally, a new packet handler ensures that login sessions are properly disposed of when a quit request is received in the character selection screen. Supporting changes include refactoring the startup logic for the login server and adjustments to player/channel state tracking. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant LoginService
participant WorldService
participant ChannelService
Client->>LoginService: RequestQuit
LoginService->>LoginSession: Disconnect()
LoginSession-->>LoginService: Session disposed
Client->>LoginService: Login
LoginService->>WorldService: AccountInfo (accountId)
WorldService->>ChannelService: Disconnect (characterId)
ChannelService-->>WorldService: DisconnectResponse
WorldService-->>LoginService: PlayerInfoResponse
WorldService->>LoginService: Heartbeat
LoginService->>LoginSession: Send Heartbeat packet
LoginSession-->>LoginService: HeartbeatResponse
WorldService->>ChannelService: Heartbeat (characterId)
ChannelService->>GameSession: Send Heartbeat packet
GameSession-->>ChannelService: HeartbeatResponse
Assessment against linked issues
Suggested labels
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
Maple2.Server.Core/Network/Session.cs (1)
113-118: Consider making heartbeat more robust with async implementation.While the current implementation works, it blocks the thread with
Thread.Sleep. Consider an asynchronous approach for better resource utilization.-protected void Heartbeat() { - while (State == SessionState.Connected) { - Thread.Sleep(TimeSpan.FromMinutes(1)); - Send(RequestPacket.Heartbeat()); - } -} +protected async Task HeartbeatAsync(CancellationToken cancellationToken = default) { + while (State == SessionState.Connected && !cancellationToken.IsCancellationRequested) { + await Task.Delay(TimeSpan.FromMinutes(1), cancellationToken); + if (State == SessionState.Connected) { + Send(RequestPacket.Heartbeat()); + } + } +}Maple2.Server.Core/proto/common.proto (1)
337-343: Consider adding a reason field for administrative disconnections.For audit logging and user communication purposes, it might be helpful to include a reason for the disconnection.
message DisconnectRequest { int64 character_id = 1; + string reason = 2; + bool is_administrative = 3; }Maple2.Server.Login/PacketHandlers/ResponseHeartbeat.cs (1)
11-16: Heartbeat response handler reads ticks but takes no action.The handler reads server and client ticks from the packet but doesn't do anything with them. Consider implementing heartbeat response validation to detect high latency or connection issues.
You could calculate the round-trip time and log or take action if it exceeds a threshold:
public override void Handle(LoginSession session, IByteReader packet) { int serverTick = packet.ReadInt(); int clientTick = packet.ReadInt(); - // should we do something with the ticks? + // Calculate round-trip time + int currentTick = Environment.TickCount; + int roundTripTime = currentTick - serverTick; + + // Log or take action if RTT is too high + if (roundTripTime > 5000) { // 5 seconds threshold + session.Logger.Warning("High latency detected: {RTT}ms", roundTripTime); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
Maple2.Server.Core/Network/Session.cs(2 hunks)Maple2.Server.Core/proto/channel/channel.proto(1 hunks)Maple2.Server.Core/proto/common.proto(1 hunks)Maple2.Server.Core/proto/sync.proto(1 hunks)Maple2.Server.Core/proto/world/world.proto(2 hunks)Maple2.Server.Game/PacketHandlers/ResponseHeartbeat.cs(1 hunks)Maple2.Server.Game/Service/ChannelService.Sync.cs(2 hunks)Maple2.Server.Game/Session/GameSession.cs(2 hunks)Maple2.Server.Login/PacketHandlers/LoginHandler.cs(1 hunks)Maple2.Server.Login/PacketHandlers/QuitHandler.cs(1 hunks)Maple2.Server.Login/PacketHandlers/ResponseHeartbeat.cs(1 hunks)Maple2.Server.Login/Session/LoginSession.cs(3 hunks)Maple2.Server.World/Containers/PlayerInfoLookup.cs(2 hunks)Maple2.Server.World/Service/WorldService.Sync.cs(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
Maple2.Server.Game/PacketHandlers/ResponseHeartbeat.cs (4)
Maple2.Server.Core/Network/Session.cs (3)
Session(25-307)Session(57-80)Session(82-82)Maple2.Server.Login/PacketHandlers/ResponseHeartbeat.cs (2)
ResponseHeartbeat(8-17)Handle(11-16)Maple2.Server.Game/Session/GameSession.cs (3)
GameSession(37-807)GameSession(109-121)GameSession(705-705)Maple2.Server.Login/PacketHandlers/QuitHandler.cs (1)
Handle(11-13)
Maple2.Server.Game/Session/GameSession.cs (1)
Maple2.Server.Core/Network/Session.cs (1)
Heartbeat(113-118)
Maple2.Server.World/Service/WorldService.Sync.cs (7)
Maple2.Server.World/Containers/PlayerInfoLookup.cs (4)
TryGet(50-59)PlayerInfo(107-118)PlayerInfo(143-146)TryGetByAccountId(64-67)Maple2.Database/Storage/Game/GameStorage.User.cs (2)
PlayerInfo(100-140)Home(142-193)Maple2.Model/Game/User/PlayerInfo.cs (3)
PlayerInfo(10-110)PlayerInfo(39-44)PlayerInfo(46-48)Maple2.Server.Game/Service/ChannelService.Sync.cs (3)
Task(10-20)Task(22-25)Task(27-40)Maple2.Server.World/Service/WorldService.Chat.cs (8)
Task(11-33)Task(35-66)Task(68-88)Task(90-110)Task(112-117)Task(119-124)Task(126-146)Task(148-153)Maple2.Server.World/Service/WorldService.Migrate.cs (2)
Task(20-67)Task(69-91)Maple2.Server.World/Containers/ChannelClientLookup.cs (3)
Channel(30-53)Channel(43-52)TryGetClient(109-117)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (26)
Maple2.Server.Core/Network/Session.cs (1)
113-118: Good addition of heartbeat mechanism to maintain connection status.The Heartbeat method provides a clean implementation for sending periodic heartbeat packets to check connection health. The use of a constant 1-minute interval is appropriate for this use case.
Maple2.Server.Core/proto/common.proto (1)
337-343: Good addition of disconnect request/response messages.The newly added DisconnectRequest and DisconnectResponse messages provide a clear protocol for handling disconnections. The character_id field in the request allows targeting specific users, and the success field in the response enables proper error handling.
Maple2.Server.Core/proto/sync.proto (1)
80-80: Good enhancement to enable account-based player lookups.Adding the account_id field to PlayerInfoRequest enables more flexible player information retrieval, particularly useful for account-wide operations and detecting duplicate logins.
Maple2.Server.Core/proto/channel/channel.proto (2)
45-46: Good addition of explicit disconnect RPC.Adding the Disconnect RPC to the Channel service provides a clear mechanism for handling disconnections, which aligns well with the DisconnectRequest and DisconnectResponse messages defined in common.proto.
45-46:Details
✅ Verification successful
Ensure proper implementation in the ChannelService class.
The implementation of this RPC in the ChannelService class should properly validate the character_id, retrieve the session, and call Disconnect() on it. It should also handle error cases appropriately.
🏁 Script executed:
#!/bin/bash # Look for Disconnect implementation in ChannelService # Find ChannelService implementations fd -e cs | grep -i "channelservice" | xargs grep -l "Disconnect" | xargs catLength of output: 4556
Disconnect RPC Implementation Confirmed in ChannelService
TheDisconnectoverride already validatesCharacterId, retrieves theGameSession, throws appropriate gRPC errors when invalid or not found, callssession.Disconnect(), and returns a success response. No further action required.Maple2.Server.Game/Session/GameSession.cs (1)
43-43: Add heartbeat thread to maintain connection health.Adding a heartbeat thread is a good approach for maintaining connection health and detecting disconnections.
Maple2.Server.World/Containers/PlayerInfoLookup.cs (1)
64-67: Good refactoring of TryGetByAccountId method.The method now follows the standard TryGet pattern with a boolean return and an out parameter, making it consistent with other similar methods. This refactoring also improves performance by using FirstOrDefault instead of iterating through all cached players.
Maple2.Server.Login/PacketHandlers/QuitHandler.cs (1)
1-15: Good addition of explicit disconnect request handling.This handler properly processes client disconnect requests by calling the
Disconnect()method on the session. This is essential for managing session lifecycles and ensuring clean disconnections.Maple2.Server.Core/proto/world/world.proto (2)
39-39: LGTM - New AccountInfo RPC added.This RPC method will allow retrieving player information by account ID using existing message types, which is a logical extension of the existing PlayerInfo functionality.
60-61: LGTM - Disconnect RPC with appropriate comment.The new Disconnect RPC implements an important mechanism for managing sessions across services, which will help fix the login session disposal issue mentioned in the PR title.
Maple2.Server.Game/Service/ChannelService.Sync.cs (2)
17-19: LGTM - Return statement reformatted for clarity.The return statement has been reformatted to improve readability without changing the logic.
27-40: LGTM - Disconnect method implementation.The implementation properly validates the input, retrieves the session, and disconnects it. The proper exception handling with descriptive error messages is a plus.
Maple2.Server.Login/Session/LoginSession.cs (4)
38-38: LGTM - Added heartbeat thread field.The private readonly thread field is correctly declared for the heartbeat mechanism.
43-43: LGTM - Heartbeat thread initialization.The heartbeat thread is properly initialized in the constructor, passing the Heartbeat method from the base Session class.
52-52: LGTM - Started heartbeat thread.The heartbeat thread is started at the appropriate point after the session is marked as connected and the server is notified.
127-127: LGTM - Properly joined heartbeat thread during disposal.Joining the heartbeat thread ensures it's properly terminated before the session is disposed, preventing resource leaks.
Maple2.Server.Login/PacketHandlers/LoginHandler.cs (4)
33-48: Refactored authentication logic to centralize credential handling.The authentication logic has been moved to the beginning of the method, which is a good practice as it ensures credentials are verified before proceeding with any command handling. This eliminates duplicate authentication code across different command cases.
50-55: Proper handling of duplicate login sessions.Good implementation of duplicate login detection and handling. The code checks if there's already an existing session for the same account and properly disconnects it before notifying the current session.
57-73: Added cross-server session management.Excellent addition that checks if the player is already logged into a game channel and disconnects them. This prevents having multiple active sessions for the same account across different servers and addresses the PR's main objective of fixing LoginSession disposal.
79-79: Disconnect session after server list is sent.The addition of
session.Disconnect()after listing servers is appropriate since the connection is no longer needed at this point. This helps free up server resources and prevents lingering sessions.Maple2.Server.World/Service/WorldService.Sync.cs (6)
20-21: Refactored response creation to use a helper method.Good refactoring to use the
PlayerInfoResponsehelper method for response creation. This improves code reuse and maintainability.
23-33: Added AccountInfo method to retrieve player info by account ID.This is a valuable addition that enables looking up player information by account ID rather than character ID. This method properly returns an empty response if no player is found, which is consistent with the API design.
36-36: Updated gender validation using pattern matching.The gender validation has been refactored to use modern C# pattern matching syntax, which improves code readability without changing the logic.
49-57: Enhanced parameter validation and improved account lookup.The input validation now uses pattern matching for more concise code. The account lookup logic has been corrected to properly check if a player was found by account ID before returning an empty response.
77-94: Added Disconnect method for character disconnection.This new method is a critical component of the fix for LoginSession disposal. It properly validates the character ID, checks if the player is online and in a valid channel, and then forwards the disconnect request to the appropriate channel client.
96-145: Added helper method for creating PlayerInfoResponse objects.Excellent addition of a helper method to centralize the creation of PlayerInfoResponse objects. This improves code reuse across both the PlayerInfo and AccountInfo methods, and ensures consistent response formatting.
Zintixx
left a comment
Zintixx
left a comment
There was a problem hiding this comment.
I'm not sure if we're implementing Heartbeat correct at the moment.
Right now it looks like we're just sending a heartbeat packet just to send it.
Along with Rabbit's comments, I think this serves as a good indicator if the user is online.
What I mean is we should have worldserver be the master for this.
The packet send to client should not originate from Game/LoginSession.
On worldserver, loop every Player online and send a heartbeat packet every 30 seconds and ensure if they're online. You'll know if they're online if they send a packet back. We send that packet to World and check against their tick and update their tick. If their tick is too distant from the ServerTick, we can disconnect due to latency. If we never receive a response back after maybe 1-2 minutes, we force disconnect the session.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
Maple2.Server.Game/PacketHandlers/ResponseHeartbeat.cs (1)
34-38: Remove unused delta variablesThe
serverDeltaandclientDeltavariables are calculated but never used. Either utilize these variables for drift detection or remove them to avoid confusion.-int serverDelta = serverTick - session.LastServerTick; -int clientDelta = clientTick - session.LastClientTick; session.LastClientTick = clientTick; session.LastServerTick = serverTick;
🧹 Nitpick comments (7)
Maple2.Server.Login/Session/LoginSession.cs (1)
39-42: Added latency tracking fieldsThe addition of these latency tracking fields is good, but consider adding XML documentation comments to explain how these fields are used in the heartbeat mechanism.
+/// <summary> +/// Last server tick received from the client +/// </summary> public int LastServerTick; +/// <summary> +/// Last client tick sent to the server +/// </summary> public int LastClientTick; +/// <summary> +/// Current latency in milliseconds between client and server +/// </summary> public int Latency;Maple2.Server.Login/Service/LoginService.Heartbeat.cs (1)
8-8: Add missing interface implementationThe
LoginServiceis declared as partial, but it's not clear what interface it implements based on this file.Clarify the class definition to show it's implementing the Login.LoginBase service:
-public partial class LoginService { +public partial class LoginService : Login.LoginBase {Maple2.Server.Login/PacketHandlers/ResponseHeartbeat.cs (1)
28-32: Remove unused delta variablesThe
serverDeltaandclientDeltavariables are calculated but never used. Either utilize these variables for drift detection or remove them to avoid confusion.-int serverDelta = serverTick - session.LastServerTick; -int clientDelta = clientTick - session.LastClientTick; session.LastClientTick = clientTick; session.LastServerTick = serverTick;Maple2.Server.Game/Service/ChannelService.Heartbeat.cs (1)
8-20: Add logging for heartbeat operationsConsider adding logging for heartbeat operations to help with diagnostics. This could include logging when sessions are not found or when heartbeats are successfully processed.
public override Task<HeartbeatResponse> Heartbeat(HeartbeatRequest request, ServerCallContext context) { if (request.CharacterId == 0) { + logger.LogWarning("Heartbeat request received with Character ID 0"); throw new RpcException(new Status(StatusCode.NotFound, "Character ID is 0.")); } if (!server.GetSession(request.CharacterId, out GameSession? session)) { + logger.LogWarning("Heartbeat request for unknown character: {CharacterId}", request.CharacterId); throw new RpcException(new Status(StatusCode.NotFound, "Session not found.")); } + logger.LogTrace("Processing heartbeat for character: {CharacterId}", request.CharacterId); session.Send(RequestPacket.Heartbeat()); return Task.FromResult(new HeartbeatResponse { Success = true, }); }Maple2.Server.Game/PacketHandlers/ResponseHeartbeat.cs (1)
12-23: Improve latency tracking and handlingThe implementation now includes latency threshold checks and session disconnection, which is good. Consider adding logging when a session is disconnected due to high latency.
session.Latency = Environment.TickCount - serverTick; if (session.Latency > Constant.MaxAllowedLatency) { #if !DEBUG + logger.LogWarning("Disconnecting session due to high latency: {Latency}ms > {MaxAllowed}ms", + session.Latency, Constant.MaxAllowedLatency); session.Disconnect(); #endif return; }Maple2.Server.Login/Program.cs (1)
40-44: Port hard-coded through constant – double-check configuration flexibility
ListenAnyIP(Target.GrpcLoginPort …)couples the port to a compile-time constant.
If you want to run multiple instances or allow Ops to override the port, consider binding to configuration:-options.ListenAnyIP(Target.GrpcLoginPort, listen => { +int loginPort = builder.Configuration.GetValue("Login:GrpcPort", Target.GrpcLoginPort); +options.ListenAnyIP(loginPort, listen => {Maple2.Server.World/WorldServer.cs (1)
60-61: Synchronous gRPC call on login service blocks the whole thread
login.Heartbeat(new HeartbeatRequest());performs a blocking call; the thread does
nothing else, so this isn’t catastrophic but it ties up a CLR thread pool thread for
network I/O unnecessarily. Usingawait login.HeartbeatAsync(...)(as shown above)
frees the thread while waiting and follows modern gRPC patterns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
Maple2.Model/Metadata/Constants.cs(1 hunks)Maple2.Server.Core/Constants/Target.cs(1 hunks)Maple2.Server.Core/Network/Session.cs(1 hunks)Maple2.Server.Core/proto/channel/channel.proto(1 hunks)Maple2.Server.Core/proto/common.proto(1 hunks)Maple2.Server.Core/proto/login/login.proto(1 hunks)Maple2.Server.Core/proto/sync.proto(1 hunks)Maple2.Server.Core/proto/world/world.proto(2 hunks)Maple2.Server.Game/PacketHandlers/ResponseHeartbeat.cs(1 hunks)Maple2.Server.Game/Service/ChannelService.Heartbeat.cs(1 hunks)Maple2.Server.Game/Service/ChannelService.Sync.cs(2 hunks)Maple2.Server.Game/Session/GameSession.cs(1 hunks)Maple2.Server.Login/PacketHandlers/LoginHandler.cs(1 hunks)Maple2.Server.Login/PacketHandlers/QuitHandler.cs(1 hunks)Maple2.Server.Login/PacketHandlers/ResponseHeartbeat.cs(1 hunks)Maple2.Server.Login/Program.cs(3 hunks)Maple2.Server.Login/Service/LoginService.Heartbeat.cs(1 hunks)Maple2.Server.Login/Service/LoginService.cs(1 hunks)Maple2.Server.Login/Session/LoginSession.cs(3 hunks)Maple2.Server.World/Containers/PlayerInfoLookup.cs(3 hunks)Maple2.Server.World/Program.cs(2 hunks)Maple2.Server.World/Service/WorldService.Sync.cs(3 hunks)Maple2.Server.World/WorldServer.cs(3 hunks)
✅ Files skipped from review due to trivial changes (3)
- Maple2.Server.Game/Session/GameSession.cs
- Maple2.Server.Core/Constants/Target.cs
- Maple2.Server.Login/Service/LoginService.cs
🚧 Files skipped from review as they are similar to previous changes (10)
- Maple2.Server.Core/proto/sync.proto
- Maple2.Server.Core/Network/Session.cs
- Maple2.Server.Login/PacketHandlers/QuitHandler.cs
- Maple2.Server.Core/proto/world/world.proto
- Maple2.Server.Core/proto/channel/channel.proto
- Maple2.Server.World/Containers/PlayerInfoLookup.cs
- Maple2.Server.Game/Service/ChannelService.Sync.cs
- Maple2.Server.Core/proto/common.proto
- Maple2.Server.Login/PacketHandlers/LoginHandler.cs
- Maple2.Server.World/Service/WorldService.Sync.cs
🧰 Additional context used
🧬 Code Graph Analysis (1)
Maple2.Server.World/WorldServer.cs (2)
Maple2.Server.World/Containers/ChannelClientLookup.cs (4)
Channel(30-53)Channel(43-52)ChannelClientLookup(14-236)TryGetClient(109-117)Maple2.Server.World/Containers/PlayerInfoLookup.cs (5)
Sync(84-105)PlayerInfoLookup(16-153)PlayerInfoLookup(29-43)PlayerInfo(107-118)PlayerInfo(143-146)
🪛 Buf (1.47.2)
Maple2.Server.Core/proto/login/login.proto
6-6: import "common.proto": file does not exist
(COMPILE)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (4)
Maple2.Server.Core/proto/login/login.proto (1)
1-10:Details
✅ Verification successful
Review import path and ensure common.proto exists
The Login service definition looks good, but the static analysis tool reports that "common.proto" doesn't exist.
Verify the existence of the common.proto file and that the import path is correct:
🏁 Script executed:
#!/bin/bash # Check if common.proto exists in the expected location if [ -f "Maple2.Server.Core/proto/common.proto" ]; then echo "common.proto exists in expected location" else echo "common.proto NOT found in expected location" find . -name "common.proto" -type f | head -n 5 fiLength of output: 141
Import path for common.proto is correct
Verified that
Maple2.Server.Core/proto/common.protoexists at the expected location. No changes to the import are required.🧰 Tools
🪛 Buf (1.47.2)
6-6: import "common.proto": file does not exist
(COMPILE)
Maple2.Server.Login/Session/LoginSession.cs (2)
49-55: Session duplicate-checking logic removedThe previous duplicate session detection and disconnection logic has been removed from this method. Ensure that this functionality has been relocated elsewhere to prevent potential session duplication issues.
Could you confirm where the duplicate session handling logic has been relocated? This is important to maintain proper session management.
116-118: Dictionary initialization reformattedThe dictionary initialization has been reformatted for better readability. This is a good improvement.
Maple2.Server.World/Program.cs (1)
49-52: LGTM: Login client registration looks goodThe gRPC client registration for the Login service is properly implemented with appropriate fallback to localhost when the environment variable isn't set.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Maple2.Server.Login/LoginServer.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Maple2.Server.Login/PacketHandlers/ResponseHeartbeatHandler.cs (1)
28-32: Delta calculation is present but unused.The code calculates serverDelta and clientDelta but doesn't use these values. Consider either removing these calculations if they're not needed or documenting their purpose for future use.
- int serverDelta = serverTick - session.ServerTick; - int clientDelta = clientTick - session.ClientTick; session.ClientTick = clientTick; session.ServerTick = serverTick;Alternatively, if these calculations are meant for future use or debugging:
// Calculate deltas for monitoring purposes int serverDelta = serverTick - session.ServerTick; int clientDelta = clientTick - session.ClientTick; session.ClientTick = clientTick; session.ServerTick = serverTick;Maple2.Server.Game/PacketHandlers/ResponseHeartbeatHandler.cs (1)
34-39: Delta calculation is present but unused.The code calculates serverDelta and clientDelta but doesn't use these values. Consider either removing these calculations if they're not needed or documenting their purpose for future use.
- int serverDelta = serverTick - session.ServerTick; - int clientDelta = clientTick - session.ClientTick; session.ClientTick = clientTick; session.ServerTick = serverTick;Alternatively, if these calculations are meant for future use or debugging:
// Calculate deltas for monitoring purposes int serverDelta = serverTick - session.ServerTick; int clientDelta = clientTick - session.ClientTick; session.ClientTick = clientTick; session.ServerTick = serverTick;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Maple2.Server.Core/Network/Session.cs(1 hunks)Maple2.Server.Game/PacketHandlers/ResponseHeartbeatHandler.cs(1 hunks)Maple2.Server.Game/Session/GameSession.cs(3 hunks)Maple2.Server.Login/PacketHandlers/ResponseHeartbeatHandler.cs(1 hunks)Maple2.Server.Login/Session/LoginSession.cs(3 hunks)Maple2.Server.World/Service/WorldService.Sync.cs(3 hunks)Maple2.Server.World/WorldServer.cs(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- Maple2.Server.Core/Network/Session.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- Maple2.Server.Login/Session/LoginSession.cs
- Maple2.Server.Game/Session/GameSession.cs
- Maple2.Server.World/WorldServer.cs
🧰 Additional context used
🧬 Code Graph Analysis (1)
Maple2.Server.Login/PacketHandlers/ResponseHeartbeatHandler.cs (3)
Maple2.Server.Core/Network/Session.cs (3)
Session(21-296)Session(53-76)Session(78-78)Maple2.Server.Game/PacketHandlers/ResponseHeartbeatHandler.cs (2)
ResponseHeartbeatHandler(9-40)Handle(12-39)Maple2.Server.Login/Session/LoginSession.cs (3)
LoginSession(20-135)LoginSession(42-45)LoginSession(120-120)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: format
🔇 Additional comments (15)
Maple2.Server.World/Service/WorldService.Sync.cs (7)
11-21: Good refactoring of PlayerInfo methodThe pattern matching validation is more readable and the extraction of response creation to a private helper method improves code maintainability.
23-33: New AccountInfo method looks goodThis method nicely complements the existing PlayerInfo method by allowing lookups by AccountId instead of CharacterId. Good choice to return an empty response when the account isn't found rather than throwing an exception.
36-36: Good use of pattern matching for validationThe pattern matching syntax makes the validation more concise and readable compared to traditional if statements.
49-49: Pattern matching improves validation readabilityThe updated validation is more concise and consistent with the pattern used throughout the file.
54-54: Improved fallback logic for player lookupGood enhancement to try finding the player by AccountId if the CharacterId lookup fails.
77-94: New Disconnect method looks well-implementedThe disconnect method follows the same pattern as other methods in the class:
- Validates the input parameters
- Checks if the player exists and is online
- Gets the appropriate channel client
- Forwards the request to the channel
This implementation will help ensure that login sessions are properly disposed when needed.
96-145: Good extraction of response creation logicThe new helper method centralizes the response creation logic, which:
- Eliminates code duplication between PlayerInfo and AccountInfo methods
- Makes future updates to the response structure easier to maintain
- Follows the DRY principle
All necessary player information fields are included in the response.
Maple2.Server.Login/PacketHandlers/ResponseHeartbeatHandler.cs (3)
1-8: Imports and namespace look good.The imports and namespace declaration are appropriate for this handler class.
9-11: Handler class setup looks correct.The class is correctly declared as a packet handler for login sessions and properly overrides the OpCode property to handle ResponseHeartbeat packets.
22-26: Initialization logic is correct.The conditional check for uninitialized ticks and their initialization is correctly implemented.
Maple2.Server.Game/PacketHandlers/ResponseHeartbeatHandler.cs (5)
1-8: Imports and namespace look good.The imports and namespace declaration are appropriate for this handler class.
9-11: Handler class setup looks correct.The class is correctly declared as a packet handler for game sessions and properly overrides the OpCode property to handle ResponseHeartbeat packets.
12-23: Latency check and disconnection logic is well implemented.The handler correctly calculates latency and disconnects sessions with excessive latency, with a safeguard for DEBUG mode. This helps maintain server performance by removing high-latency connections.
24-32: Early returns and initialization logic are correct.The handler properly handles the cases where ticks are zero and initializes session tick values when needed.
1-40: Verify the consistency across Login and Game server implementations.The Game server implementation includes latency checking and disconnection, but the Login server implementation does not. This inconsistency could lead to different behavior in handling high-latency connections between the two servers.
Consider updating the Login server handler to include similar latency checking for consistency, or document why the different behavior is intentional.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores