harden session lifecycle and prevent double disconnects - #574
Conversation
WalkthroughRefactors session disconnect and disposal flows. Adds reentrancy guard and caller-info logging to network Disconnect; exposes AccountId/CharacterId on core Session. Strengthens exception handling and send guards. GameSession replaces public identity properties with internal handling and introduces a dispose state machine with centralized, safe teardown and save logic. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Session as Core Session
participant Pipeline as Recv/Send Pipelines
participant Client as Network Client
Caller->>Session: Disconnect(caller,line,file)
alt first-time disconnect
Session->>Session: Set disconnecting flag
Session->>Pipeline: Signal completion
Pipeline-->>Session: Fault? log error
Session->>Client: CloseClient()
Session->>Session: Dispose (try/catch around join/close)
else already disconnecting
Session->>Caller: Return (no-op)
end
note over Session,Client: SendInternal/SendRaw check disconnecting flag and skip sends when set
sequenceDiagram
autonumber
actor Shutdown as Shutdown/GC
participant GSession as GameSession
participant Base as Base Session
participant Managers as Guild/Buddy/Party/...
participant Storage as Save/Config/Persistence
Shutdown->>GSession: Dispose()
alt state: active -> disposing
GSession->>GSession: Capture fieldTickSnapshot
GSession->>Base: Dispose()
loop per component
GSession->>Storage: TrySaveComponent(...)
Storage-->>GSession: Success/Exception (logged)
end
par safe disposals
GSession->>Managers: SafeDispose(...)
and
GSession->>GSession: SaveCacheConfig(fieldTickSnapshot)
end
GSession->>GSession: Set state -> disposed
else disposing/disposed
GSession->>Shutdown: Return (no-op)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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
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/Session/GameSession.cs (1)
743-808: Reentrancy guard can strand the network socket when pre-base cleanup throws
gameDisposeStateflips to1before we touchScheduler.Stop(),LeaveField(), etc. If any of those steps throw before we hitbase.Dispose(disposing);, we land in thecatch, log the error, and then thefinallyblock drivesgameDisposeStatestraight to2. Every futureDispose(including the finalizer) will now bail out at the guard, so the baseSession.Disposenever runs—meaning the underlying socket thread (Complete(),thread.Join,CloseClient, see Maple2.Server.Core/Network/Session.cs:83-108) is left alive. Previously, a second dispose attempt would have finished the cleanup; the new guard prevents that safety net.We should only mark the session as fully disposed once the base dispose succeeds, and reset the guard if we fail before that so another pass can retry. Something like:
- if (Interlocked.CompareExchange(ref gameDisposeState, 1, 0) != 0) return; - // begin dispose - - // Snapshot values needed after teardown - long fieldTickSnapshot = Field?.FieldTick ?? Environment.TickCount64; + if (Interlocked.CompareExchange(ref gameDisposeState, 1, 0) != 0) return; + bool baseDisposed = false; + long fieldTickSnapshot = Field?.FieldTick ?? Environment.TickCount64; ... - base.Dispose(disposing); + base.Dispose(disposing); + baseDisposed = true; ... - } catch (Exception ex) { - Logger.Error(ex, "Error during session cleanup for {Player}", PlayerName); + } catch (Exception ex) { + Logger.Error(ex, "Error during session cleanup for {Player}", PlayerName); + if (!baseDisposed) { + Interlocked.Exchange(ref gameDisposeState, 0); + } } finally { ... - Interlocked.Exchange(ref gameDisposeState, 2); + if (baseDisposed) { + Interlocked.Exchange(ref gameDisposeState, 2); + } }This keeps the reentrancy guard while still guaranteeing the base resources are eventually torn down.
🧹 Nitpick comments (1)
Maple2.Server.Core/Network/Session.cs (1)
120-123: Log after winning the disconnect guardCalling
Logger.Information("Disconnected …")before theInterlocked.Exchangemeans every re-entrant call logs “Disconnected …” even when we immediately return because another thread already tore the session down. That makes production logs noisy and obscures who actually performed the teardown. Please move the log (or change its wording) so we only emit the Info-level entry once we win the guard; you can keep an extra Debug log for filtered duplicates if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Maple2.Server.Core/Network/Session.cs(11 hunks)Maple2.Server.Game/Session/GameSession.cs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
Maple2.Server.Core/Network/Session.cs (1)
Maple2.Server.Core/Network/QueuedPipeScheduler.cs (1)
Complete(14-14)
Maple2.Server.Game/Session/GameSession.cs (5)
Maple2.Tools/Scheduler/EventQueue.cs (1)
InvokeAll(84-113)Maple2.Server.Game/GameServer.cs (1)
OnDisconnected(73-78)Maple2.Server.Core/Network/Session.cs (2)
Dispose(84-87)Dispose(89-109)Maple2.Database/Storage/Game/DatabaseRequest.cs (4)
Dispose(34-37)BeginTransaction(16-18)Commit(20-28)SaveChanges(30-32)Maple2.Database/Storage/Game/GameStorage.cs (5)
GameStorage(11-80)GameStorage(22-40)Request(42-49)Request(51-79)Request(53-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: build
2 participants
Summary by CodeRabbit
Bug Fixes
Refactor