From 7671fe2d942fd3338bbf37eab705bf2fd6ae8619 Mon Sep 17 00:00:00 2001 From: nolt Date: Mon, 27 Jul 2026 00:00:56 +0200 Subject: [PATCH] Serialize player context mutations against the periodic save A player's progress is persisted from two unrelated flows: action handlers triggered by incoming packets (sequential per connection) and the periodic save, which runs on an independent timer. Action handlers mutate tracked entities with plain field and collection writes - for example crafting toggles item.ItemOptions, and item stacking and NPC selling delete item rows - which bypass the persistence context's own lock. When such a mutation runs while SaveChangesAsync enumerates the change tracker, the save throws (collection-modified, or a DbUpdateConcurrency "affected 0 rows" since the context has no concurrency tokens). SaveChanges is atomic, so every following save fails too and the whole session never persists: on relog the player rolls back, losing progress and items. Add a per-player re-entrant persistence lock and acquire it around both the packet-handling funnel and SaveProgressAsync, so a player's mutations and saves can never overlap. The lock is re-entrant per asynchronous flow (an instance AsyncLocal), so an inline save inside an already-serialized handler does not deadlock; and it is per player, so a trade still acquires the trading partner's lock separately. Add regression tests covering mutual exclusion and re-entrancy of the lock. --- src/GameLogic/Player.cs | 87 ++++++++++++- src/GameServer/RemoteView/RemotePlayer.cs | 2 +- .../PersistenceLockTest.cs | 114 ++++++++++++++++++ 3 files changed, 199 insertions(+), 4 deletions(-) create mode 100644 tests/MUnique.OpenMU.Tests/PersistenceLockTest.cs diff --git a/src/GameLogic/Player.cs b/src/GameLogic/Player.cs index 5b9928b..a7f316e 100644 --- a/src/GameLogic/Player.cs +++ b/src/GameLogic/Player.cs @@ -54,6 +54,21 @@ public class Player : AsyncDisposable, IBucketMapObserver, IAttackable, IAttacke private readonly AsyncLock _moveLock = new(); private readonly AsyncLock _experienceLock = new(); + /// + /// Serializes context mutations done by this player's action handlers against the periodic and + /// disconnect progress saves, which run on an independent timer flow. See + /// . + /// + private readonly AsyncLock _persistenceLock = new(); + + /// + /// Tracks, per asynchronous flow, whether is already held, so the + /// lock can be re-entered (Nito's is not reentrant). It is an instance + /// field on purpose: reentrancy must be tracked per player, so a flow holding player A's lock + /// still acquires player B's lock (e.g. during a trade) instead of wrongly skipping it. + /// + private readonly AsyncLocal _persistenceLockHeld = new(); + private readonly Walker _walker; private readonly AppearanceDataAdapter _appearanceData; @@ -1879,12 +1894,78 @@ public class Player : AsyncDisposable, IBucketMapObserver, IAttackable, IAttacke /// Success of the save operation. public async ValueTask SaveProgressAsync(CancellationToken cancellationToken = default) { - if (!this.IsTemplatePlayer) + if (this.IsTemplatePlayer) { - return await this.PersistenceContext.SaveChangesAsync(cancellationToken).ConfigureAwait(false); + return true; } - return true; + return await this.RunPersistenceExclusiveAsync( + () => this.PersistenceContext.SaveChangesAsync(cancellationToken), + cancellationToken).ConfigureAwait(false); + } + + /// + /// Runs the given operation while holding this player's persistence lock, so that context + /// mutations and progress saves for the player never run concurrently. + /// + /// + /// The periodic progress save () runs on an + /// independent timer flow. Action handlers mutate tracked entities with plain field/collection + /// writes (e.g. crafting toggling item.ItemOptions) which bypass the persistence context's + /// own lock; if such a mutation runs while enumerates the + /// change tracker, the save throws (collection-modified / DbUpdateConcurrency) and every following + /// save fails too, so the whole session is lost on relog. Serializing the packet handler funnel + /// and the save against each other closes that window. The lock is re-entrant per asynchronous + /// flow, so an inline save inside an already-serialized handler does not deadlock. + /// + /// The result type of the operation. + /// The operation to run exclusively. + /// The cancellation token. + /// The result of the operation. + public async ValueTask RunPersistenceExclusiveAsync(Func> operation, CancellationToken cancellationToken = default) + { + if (this._persistenceLockHeld.Value) + { + return await operation().ConfigureAwait(false); + } + + using var l = await this._persistenceLock.LockAsync(cancellationToken).ConfigureAwait(false); + this._persistenceLockHeld.Value = true; + try + { + return await operation().ConfigureAwait(false); + } + finally + { + this._persistenceLockHeld.Value = false; + } + } + + /// + /// Runs the given operation while holding this player's persistence lock. + /// See for the rationale. + /// + /// The operation to run exclusively. + /// The cancellation token. + /// A value task which completes when the operation completed. + public async ValueTask RunPersistenceExclusiveAsync(Func operation, CancellationToken cancellationToken = default) + { + if (this._persistenceLockHeld.Value) + { + await operation().ConfigureAwait(false); + return; + } + + using var l = await this._persistenceLock.LockAsync(cancellationToken).ConfigureAwait(false); + this._persistenceLockHeld.Value = true; + try + { + await operation().ConfigureAwait(false); + } + finally + { + this._persistenceLockHeld.Value = false; + } } /// diff --git a/src/GameServer/RemoteView/RemotePlayer.cs b/src/GameServer/RemoteView/RemotePlayer.cs index 2c0b584..d7687f7 100644 --- a/src/GameServer/RemoteView/RemotePlayer.cs +++ b/src/GameServer/RemoteView/RemotePlayer.cs @@ -151,7 +151,7 @@ public class RemotePlayer : Player, IClientVersionProvider, IHasIpAddress this.Logger.LogDebug("[C->S] {0}", buffer.ToArray().AsString()); } - await this.MainPacketHandler.HandlePacketAsync(this, buffer).ConfigureAwait(false); + await this.RunPersistenceExclusiveAsync(() => this.MainPacketHandler.HandlePacketAsync(this, buffer)).ConfigureAwait(false); } finally { diff --git a/tests/MUnique.OpenMU.Tests/PersistenceLockTest.cs b/tests/MUnique.OpenMU.Tests/PersistenceLockTest.cs new file mode 100644 index 0000000..1ca00dc --- /dev/null +++ b/tests/MUnique.OpenMU.Tests/PersistenceLockTest.cs @@ -0,0 +1,114 @@ +// +// Licensed under the MIT License. See LICENSE file in the project root for full license information. +// + +namespace MUnique.OpenMU.Tests; + +using System.Threading; + +/// +/// Tests for the per-player persistence lock (), +/// which serializes a player's context mutations against its periodic/disconnect progress saves so they +/// can never run concurrently. Without it, a mutation running during SaveChangesAsync corrupts the +/// change tracker and rolls the whole session back. +/// +[TestFixture] +public class PersistenceLockTest +{ + /// + /// Verifies that concurrent exclusive operations for the same player never overlap. + /// + [Test] + public async Task ConcurrentAccessIsSerializedAsync() + { + var player = await PlayerTestHelper.CreatePlayerAsync().ConfigureAwait(false); + var concurrent = 0; + var overlapDetected = false; + + async ValueTask BodyAsync() + { + if (Interlocked.Increment(ref concurrent) > 1) + { + overlapDetected = true; + } + + await Task.Delay(1).ConfigureAwait(false); + Interlocked.Decrement(ref concurrent); + } + + var tasks = Enumerable.Range(0, 50) + .Select(_ => player.RunPersistenceExclusiveAsync(BodyAsync).AsTask()) + .ToArray(); + await Task.WhenAll(tasks).ConfigureAwait(false); + + Assert.That(overlapDetected, Is.False, "Two exclusive operations for the same player ran at the same time."); + } + + /// + /// Verifies that re-entering the lock from within an already-held exclusive scope does not deadlock + /// (an inline save inside a packet handler is exactly this case). + /// + [Test] + public async Task ReentrantAccessDoesNotDeadlockAsync() + { + var player = await PlayerTestHelper.CreatePlayerAsync().ConfigureAwait(false); + var executed = 0; + + var run = player.RunPersistenceExclusiveAsync(async () => + { + Interlocked.Increment(ref executed); + await player.RunPersistenceExclusiveAsync(async () => + { + Interlocked.Increment(ref executed); + await Task.Yield(); + }).ConfigureAwait(false); + }).AsTask(); + + // If reentrancy deadlocked, this would hang; fail fast instead of blocking the suite. + await run.WaitAsync(TimeSpan.FromSeconds(5)).ConfigureAwait(false); + + Assert.That(executed, Is.EqualTo(2)); + } + + /// + /// Verifies that a re-entrant exclusive operation still runs while another flow holds the lock: + /// the outer flow keeps the lock, an independent flow must wait, and the re-entrant call inside the + /// outer flow proceeds without waiting for itself. + /// + [Test] + public async Task IndependentFlowWaitsWhileLockIsHeldAsync() + { + var player = await PlayerTestHelper.CreatePlayerAsync().ConfigureAwait(false); + var otherEntered = false; + var holderHasLock = new TaskCompletionSource(); + var mayRelease = new TaskCompletionSource(); + + // Holder runs on its own flow and keeps the lock until signalled. + var holder = Task.Run(() => player.RunPersistenceExclusiveAsync(async () => + { + holderHasLock.SetResult(); + + // A re-entrant call from the holding flow must NOT block on the lock we already hold. + await player.RunPersistenceExclusiveAsync(() => ValueTask.CompletedTask).ConfigureAwait(false); + + await mayRelease.Task.ConfigureAwait(false); + }).AsTask()); + + await holderHasLock.Task.WaitAsync(TimeSpan.FromSeconds(5)).ConfigureAwait(false); + + // Competing flow started from an INDEPENDENT context (does not inherit the reentrancy flag). + var other = Task.Run(() => player.RunPersistenceExclusiveAsync(() => + { + otherEntered = true; + return ValueTask.CompletedTask; + }).AsTask()); + + await Task.Delay(50).ConfigureAwait(false); + Assert.That(otherEntered, Is.False, "An independent flow entered while the lock was held."); + + mayRelease.SetResult(); + await holder.WaitAsync(TimeSpan.FromSeconds(5)).ConfigureAwait(false); + await other.WaitAsync(TimeSpan.FromSeconds(5)).ConfigureAwait(false); + Assert.That(otherEntered, Is.True, "The competing flow never ran after the lock was released."); + } +}