Drain send queue on disconnect and ensure session cleanup#630
Drain send queue on disconnect and ensure session cleanup#630AngeloTadeucci merged 2 commits intomasterfrom
Conversation
Make send worker a field and drain the send queue during disconnect by calling sendQueue.CompleteAdding() and joining the worker thread, allowing queued packets (e.g. migration) to be delivered before close. Remove disconnecting checks in SendRaw/SendWorker so the worker can finish draining even when a disconnect is in progress. Move/ensure session.Disconnect() calls into finally blocks across several handlers (DungeonManager, ChannelHandler, QuitHandler, GameSession, CharacterManagementHandler) to guarantee cleanup. Also fix the send worker thread variable usage when starting the thread.
📝 WalkthroughWalkthroughRefactors Session send-worker lifecycle and tightens session cleanup: introduces a dedicated send worker thread, drains and joins the send queue on disconnect, relaxes early SendRaw checks, and adds finally-driven guaranteed session.Disconnect() calls across migration and login handlers. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant Session as Session
participant SendWorker as SendWorker\n(thread)
participant Socket as NetworkSocket
rect rgba(200,220,255,0.5)
Client->>Session: Enqueue packet / SendRaw()
Session->>SendWorker: Signal sendQueue
SendWorker->>Socket: Write packets
Socket-->>SendWorker: ACK/complete
end
rect rgba(200,255,200,0.5)
Client->>Session: Request Disconnect()
Session->>SendWorker: Stop request (dispose)
Session->>SendWorker: Drain sendQueue
SendWorker->>Socket: Flush remaining packets
SendWorker-->>Session: Thread join (timeout)
Session-->>Client: Disconnected
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Maple2.Server.Game/PacketHandlers/ChannelHandler.cs (1)
41-51:⚠️ Potential issue | 🟠 MajorError path now disconnects the player, which may be a UX regression.
With the
finallyblock, ifMigrateOutfails (RPC exception), the player is disconnected from the game entirely. Previously, the error path likely kept the session alive so the player could remain on their current channel after seeing the error notice.Consider whether the error path should preserve the session. If so, the
finallyblock approach doesn't work here — you'd need the explicit disconnect only on success:Proposed fix: disconnect only on success
try { // ... migration request ... session.Send(MigrationPacket.GameToGame(endpoint, response.Token, session.Field?.MapId ?? 0)); session.State = SessionState.ChangeChannel; + session.Disconnect(); } catch (RpcException ex) { Logger.Error(ex, "Failed to migrate to channel {Channel}", channel); session.Send(NoticePacket.MessageBox(new InterfaceText("Channel is unavailable, close the channel list and try again."))); // Update the client with the latest channel list. ChannelsResponse response = World.Channels(new ChannelsRequest()); session.Send(ChannelPacket.Load(response.Channels)); - } finally { - session.Disconnect(); }
🧹 Nitpick comments (1)
Maple2.Server.Core/Network/Session.cs (1)
306-329:SendInternalstill guards withdisconnecting == 1, preventing new packets from being enqueued after disconnect begins.This is correct — only the already-queued packets should be drained, not new ones. The
packet.Take(length).ToArray()copy is needed since the buffer may be pooled/reused.One minor observation:
lastSentPackets[op] = packet.Take(length).ToArray()at Line 317 andbyte[] packetCopy = packet.Take(length).ToArray()at Line 322 allocate two separate copies. You could reusepacketCopyfor both.♻️ Avoid double allocation
private void SendInternal(byte[] packet, int length) { if (disposed || disconnecting == 1) return; `#if` DEBUG LogSend(packet, length); `#endif` + byte[] packetCopy = packet.Take(length).ToArray(); // Track last sent packet by opcode if (length >= 2) { var op = (SendOp) (packet[1] << 8 | packet[0]); - // Store a copy to avoid mutation issues - lastSentPackets[op] = packet.Take(length).ToArray(); + lastSentPackets[op] = packetCopy; } // Queue the raw packet for background processing - // Make a copy since the caller may reuse the buffer - byte[] packetCopy = packet.Take(length).ToArray(); try { sendQueue.Add((packetCopy, length)); } catch (InvalidOperationException) {
Replace a single Thread.Sleep(25) with a retry loop that sleeps 10ms and calls queue.InvokeAll up to 10 times until the scheduled action runs. This reduces flakiness by handling timing imprecision in CI environments.
|
@AngeloTadeucci, the issue from #628 still occurs even after this change. |
Weird I tried replicating and wasn't able to! I used 2 clients logging in into the same account and a bunch of other stuff. Did you pull the changes right? |
My environment was borked, fresh repo fixed it. Apologies! |
3 participants
Summary by CodeRabbit