diff --git a/src/NosCore.GameObject/Services/BazaarService/BazaarService.cs b/src/NosCore.GameObject/Services/BazaarService/BazaarService.cs index fc812325c..7d6fdc9da 100644 --- a/src/NosCore.GameObject/Services/BazaarService/BazaarService.cs +++ b/src/NosCore.GameObject/Services/BazaarService/BazaarService.cs @@ -6,6 +6,8 @@ using Json.More; using NodaTime; +using NosCore.Core.Concurrency; +using System.Collections.Concurrent; using NodaTime.Serialization.SystemTextJson; using NosCore.Dao.Interfaces; using NosCore.Data.Dto; @@ -26,6 +28,13 @@ public class BazaarService(IBazaarRegistry bazaarRegistry, IDao itemInstanceDao, IClock clock) : IBazaarService { + // Channels reach these through BazaarHub, and SignalR dispatches calls concurrently, so + // read-check-write on a listing is a lost update waiting to happen. One lock per listing + // rather than one for the bazaar, so unrelated trades still run in parallel. + private static readonly ConcurrentDictionary Claims = new(); + + private static AsyncLock ClaimLock(long id) => Claims.GetOrAdd(id, _ => new AsyncLock()); + public List GetBazaar(long id, byte? index, byte? pageSize, BazaarListType? typeFilter, byte? subTypeFilter, byte? levelFilter, byte? rareFilter, byte? upgradeFilter, long? sellerFilter) { @@ -154,6 +163,7 @@ public List GetBazaar(long id, byte? index, byte? pageSize, BazaarLi public async Task DeleteBazaarAsync(long id, short count, string requestCharacterName, long? requestCharacterId = null) { + using var claim = await ClaimLock(id).AcquireAsync(); var bzlink = bazaarRegistry.GetById(id); if (bzlink == null) { @@ -243,6 +253,7 @@ public async Task AddBazaarAsync(Guid itemInstanceId, long characte public async Task ModifyBazaarAsync(long id, Json.Patch.JsonPatch bzMod) { + using var claim = await ClaimLock(id).AcquireAsync(); var item = bazaarRegistry.GetById(id); if ((item?.BazaarItem == null) || (item.BazaarItem?.Amount != item.ItemInstance?.Amount)) { diff --git a/src/NosCore.PacketHandlers/Bazaar/CBuyPacketHandler.cs b/src/NosCore.PacketHandlers/Bazaar/CBuyPacketHandler.cs index 145d2dca6..7d9b4a1de 100644 --- a/src/NosCore.PacketHandlers/Bazaar/CBuyPacketHandler.cs +++ b/src/NosCore.PacketHandlers/Bazaar/CBuyPacketHandler.cs @@ -55,21 +55,25 @@ public override async Task ExecuteAsync(CBuyPacket packet, ClientSession clientS { if (clientSession.Character.Gold >= price) { - clientSession.Character.Gold -= price; - await clientSession.SendPacketAsync(clientSession.Character.GenerateGold()); - - var itemInstance = await itemInstanceDao.FirstOrDefaultAsync(s => s!.Id == bz.ItemInstance.Id); - var item = itemProvider.Convert(itemInstance!); - item.Id = Guid.NewGuid(); - var newInv = - clientSession.Character.InventoryService.AddItemToPocket( - InventoryItemInstance.Create(item, clientSession.Character.CharacterId)); - await clientSession.SendPacketAsync(newInv!.GeneratePocketChange()); - + // Claim the listing before anything is paid for or created. The listing + // is shared across channels, so two buyers can both pass the checks + // above; only one can win this call, and the loser must leave with + // neither the gold gone nor an item made. var remove = await bazaarHttpClient.DeleteBazaarAsync(packet.BazaarId, packet.Amount, clientSession.Character.Name); if (remove) { + clientSession.Character.Gold -= price; + await clientSession.SendPacketAsync(clientSession.Character.GenerateGold()); + + var itemInstance = await itemInstanceDao.FirstOrDefaultAsync(s => s!.Id == bz.ItemInstance.Id); + var item = itemProvider.Convert(itemInstance!); + item.Id = Guid.NewGuid(); + var newInv = + clientSession.Character.InventoryService.AddItemToPocket( + InventoryItemInstance.Create(item, clientSession.Character.CharacterId)); + await clientSession.SendPacketAsync(newInv!.GeneratePocketChange()); + await clientSession.HandlePacketsAsync(new[] { new CBListPacket { Index = 0, ItemVNumFilter = new List() } }); await clientSession.SendPacketAsync(new RCBuyPacket(bz.SellerName!) { @@ -95,7 +99,14 @@ await clientSession.SendPacketAsync(new SayiPacket return; } - logger.LogError(logLanguage[LogLanguageKey.BAZAAR_BUY_ERROR]); + // Someone else took it first, most likely from another channel. + logger.LogInformation(logLanguage[LogLanguageKey.BAZAAR_BUY_ERROR]); + await clientSession.SendPacketAsync(new ModaliPacket + { + Type = 1, + Message = Game18NConstString.OfferUpdated + }); + await clientSession.HandlePacketsAsync(new[] { new CBListPacket { Index = 0, ItemVNumFilter = new List() } }); } else { diff --git a/test/NosCore.PacketHandlers.Tests/Bazaar/CBuyPacketHandlerTests.cs b/test/NosCore.PacketHandlers.Tests/Bazaar/CBuyPacketHandlerTests.cs index dd7c0b460..37b88da10 100644 --- a/test/NosCore.PacketHandlers.Tests/Bazaar/CBuyPacketHandlerTests.cs +++ b/test/NosCore.PacketHandlers.Tests/Bazaar/CBuyPacketHandlerTests.cs @@ -302,5 +302,22 @@ private void ShouldReceiveInsufficientGoldMessage() var packet = (ModaliPacket?)Session.LastPackets.FirstOrDefault(s => s is ModaliPacket); Assert.IsTrue(packet?.Message == Game18NConstString.InsufficientGoldAvailable); } + + [TestMethod] + public async Task LosingTheRaceForAListingCostsNeitherGoldNorMakesAnItem() + { + // The listing lives on the master server and every channel reads it, so two buyers + // can both pass the price and amount checks. Only one wins the claim. + Session.Character.Gold = 500; + BazaarHttpClient! + .Setup(b => b.DeleteBazaarAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) + .ReturnsAsync(false); + + await CbuyPacketHandler!.ExecuteAsync(new CBuyPacket { BazaarId = 0, Amount = 1, Price = 50 }, Session); + + Assert.AreEqual(500, Session.Character.Gold, "the loser must keep their gold"); + Assert.AreEqual(0, Session.Character.InventoryService.Count, "the loser must not receive an item"); + } + } }