From a0d72b422c55c00714b1f6f05bfa0fbbfdf3db19 Mon Sep 17 00:00:00 2001 From: Ezequiel Zacca Date: Sun, 18 Oct 2026 20:55:31 -0300 Subject: [PATCH] fix(npc): don't respawn an NPC which was disposed while waiting to respawn A killed NPC schedules its respawn after the respawn delay. When it got disposed in the meantime (e.g. its spawn area was deleted, or the map or mini game was torn down), the pending respawn still initialized it and put it back into the area of interest manager of the map. The respawn is now skipped when the NPC is disposed or disposing. For this check to work, AsyncDisposable.DisposeAsync now marks the instance as disposed, too - before, IsDisposed stayed false after DisposeAsync, so a second DisposeAsync released the resources again. --- src/GameLogic/AsyncDisposable.cs | 5 +- src/GameLogic/NPC/AttackableNpcBase.cs | 6 + .../AsyncDisposableTest.cs | 66 +++++++++++ .../MonsterRespawnTest.cs | 108 ++++++++++++++++++ 4 files changed, 183 insertions(+), 2 deletions(-) create mode 100644 tests/MUnique.OpenMU.Tests/AsyncDisposableTest.cs create mode 100644 tests/MUnique.OpenMU.Tests/MonsterRespawnTest.cs diff --git a/src/GameLogic/AsyncDisposable.cs b/src/GameLogic/AsyncDisposable.cs index 1ecf285033..484c58df2c 100644 --- a/src/GameLogic/AsyncDisposable.cs +++ b/src/GameLogic/AsyncDisposable.cs @@ -32,8 +32,9 @@ public async ValueTask DisposeAsync() try { await this.DisposeAsyncCore().ConfigureAwait(false); - this.Dispose(true); - GC.SuppressFinalize(this); + + // Disposes the managed resources and marks this instance as disposed. + this.Dispose(); } finally { diff --git a/src/GameLogic/NPC/AttackableNpcBase.cs b/src/GameLogic/NPC/AttackableNpcBase.cs index e538208094..1828191d57 100644 --- a/src/GameLogic/NPC/AttackableNpcBase.cs +++ b/src/GameLogic/NPC/AttackableNpcBase.cs @@ -373,6 +373,12 @@ private async ValueTask RespawnAsync() { try { + if (this.IsDisposed || this.IsDisposing) + { + // It got disposed while waiting for the respawn, e.g. because its spawn area was removed. + return; + } + if (!this.ShouldRespawn) { await this.RemoveFromMapAndDisposeAsync().ConfigureAwait(false); diff --git a/tests/MUnique.OpenMU.Tests/AsyncDisposableTest.cs b/tests/MUnique.OpenMU.Tests/AsyncDisposableTest.cs new file mode 100644 index 0000000000..759acb4836 --- /dev/null +++ b/tests/MUnique.OpenMU.Tests/AsyncDisposableTest.cs @@ -0,0 +1,66 @@ +// +// Licensed under the MIT License. See LICENSE file in the project root for full license information. +// + +namespace MUnique.OpenMU.Tests; + +using MUnique.OpenMU.GameLogic; + +/// +/// Tests for the . +/// +[TestFixture] +public class AsyncDisposableTest +{ + /// + /// Tests that an instance is marked as disposed after . + /// + [Test] + public async Task IsDisposedAfterDisposeAsync() + { + var disposable = new TestDisposable(); + + await disposable.DisposeAsync().ConfigureAwait(false); + + Assert.That(disposable.IsDisposed, Is.True); + Assert.That(disposable.IsDisposing, Is.False); + } + + /// + /// Tests that disposing an instance a second time doesn't release its resources again. + /// + [Test] + public async Task SecondDisposeAsyncDoesNothingAsync() + { + var disposable = new TestDisposable(); + + await disposable.DisposeAsync().ConfigureAwait(false); + await disposable.DisposeAsync().ConfigureAwait(false); + + Assert.That(disposable.AsyncDisposeCount, Is.EqualTo(1)); + Assert.That(disposable.DisposeCount, Is.EqualTo(1)); + } + + private sealed class TestDisposable : AsyncDisposable + { + public int AsyncDisposeCount { get; private set; } + + public int DisposeCount { get; private set; } + + protected override ValueTask DisposeAsyncCore() + { + this.AsyncDisposeCount++; + return base.DisposeAsyncCore(); + } + + protected override void Dispose(bool disposing) + { + if (disposing) + { + this.DisposeCount++; + } + + base.Dispose(disposing); + } + } +} diff --git a/tests/MUnique.OpenMU.Tests/MonsterRespawnTest.cs b/tests/MUnique.OpenMU.Tests/MonsterRespawnTest.cs new file mode 100644 index 0000000000..825871004d --- /dev/null +++ b/tests/MUnique.OpenMU.Tests/MonsterRespawnTest.cs @@ -0,0 +1,108 @@ +// +// Licensed under the MIT License. See LICENSE file in the project root for full license information. +// + +namespace MUnique.OpenMU.Tests; + +using Moq; +using MUnique.OpenMU.DataModel.Configuration; +using MUnique.OpenMU.GameLogic; +using MUnique.OpenMU.GameLogic.Attributes; +using MUnique.OpenMU.GameLogic.NPC; +using MUnique.OpenMU.Pathfinding; +using MonsterAttribute = MUnique.OpenMU.Persistence.BasicModel.MonsterAttribute; +using MonsterDefinition = MUnique.OpenMU.Persistence.BasicModel.MonsterDefinition; + +/// +/// Tests for the respawn of a killed . +/// +[TestFixture] +public class MonsterRespawnTest +{ + private static readonly TimeSpan RespawnDelay = TimeSpan.FromMilliseconds(100); + + private static readonly Point SpawnPoint = new(100, 100); + + /// + /// Tests that a killed monster respawns on its map after the respawn delay. + /// + [Test] + public async Task KilledMonsterRespawnsAsync() + { + var gameContext = GameContextTestHelper.CreateGameContext(); + var killer = await PlayerTestHelper.CreatePlayerAsync(gameContext).ConfigureAwait(false); + var monster = await CreateMonsterAsync(gameContext).ConfigureAwait(false); + + await monster.ReflectDamageAsync(killer, 1_000_000).ConfigureAwait(false); + Assert.That(monster.IsAlive, Is.False); + + var deadline = DateTime.UtcNow.AddSeconds(10); + while (!monster.IsAlive && DateTime.UtcNow < deadline) + { + await Task.Delay(50).ConfigureAwait(false); + } + + Assert.That(monster.IsAlive, Is.True); + Assert.That(monster.CurrentMap.GetNpcsInRange(SpawnPoint, 1), Does.Contain(monster)); + } + + /// + /// Tests that a killed monster which is disposed while it waits to respawn, + /// e.g. because its spawn area got deleted, doesn't come back to the map. + /// + [Test] + public async Task KilledMonsterDoesNotRespawnAfterDisposeAsync() + { + var gameContext = GameContextTestHelper.CreateGameContext(); + var killer = await PlayerTestHelper.CreatePlayerAsync(gameContext).ConfigureAwait(false); + var monster = await CreateMonsterAsync(gameContext).ConfigureAwait(false); + var map = monster.CurrentMap; + var monsterId = monster.Id; + + await monster.ReflectDamageAsync(killer, 1_000_000).ConfigureAwait(false); + await monster.DisposeAsync().ConfigureAwait(false); + + await Task.Delay(RespawnDelay * 5).ConfigureAwait(false); + + Assert.That(monster.IsAlive, Is.False); + Assert.That(map.GetNpcsInRange(SpawnPoint, 1), Does.Not.Contain(monster)); + Assert.That(map.GetAttackablesInRange(SpawnPoint, 1), Does.Not.Contain(monster)); + Assert.That(map.GetObject(monsterId), Is.Not.SameAs(monster)); + } + + private static async ValueTask CreateMonsterAsync(IGameContext gameContext) + { + // A unique id, because the attributes of monster definitions are cached by their id. + var monsterDefinition = new MonsterDefinition + { + Id = Guid.NewGuid(), + ObjectKind = NpcObjectKind.Monster, + RespawnDelay = RespawnDelay, + }; + monsterDefinition.Attributes.Add(new MonsterAttribute { AttributeDefinition = Stats.MaximumHealth, Value = 100 }); + var map = await gameContext.GetMapAsync(0).ConfigureAwait(false); + var spawnArea = new MonsterSpawnArea + { + MonsterDefinition = monsterDefinition, + GameMap = map!.Definition, + SpawnTrigger = SpawnTrigger.Automatic, + X1 = SpawnPoint.X, + Y1 = SpawnPoint.Y, + X2 = SpawnPoint.X, + Y2 = SpawnPoint.Y, + Quantity = 1, + }; + + var monster = new Monster( + spawnArea, + monsterDefinition, + map, + NullDropGenerator.Instance, + new Mock().Object, + gameContext.PlugInManager, + gameContext.PathFinderPool); + monster.Initialize(); + await map.AddAsync(monster).ConfigureAwait(false); + return monster; + } +}