suriyun-production / mmorpg-kit-docs

This is document for MMORPG KIT project (https://www.assetstore.unity3d.com/#!/content/110188?aid=1100lGeN)
https://suriyun-production.github.io/mmorpg-kit-docs
48 stars 11 forks source link

[Possible Fix] Duplicate Characters / Connection Rejected by Server / Can't Interact #1896

Closed Monty001 closed 1 year ago

Monty001 commented 1 year ago

I've been trying to work our what triggers this for a while now and today I was able to reproduce it in a default kit.

It's triggered by an invalid item index in either a storage (or campfire etc) or the characters inventory when moving items between. The kits prefabs for items makes this harder to recreate then if you have an interface with either click to move or close proximity dragging.

You can however recreate this quite easily. Set the inventory to simple and then attempt to move the last item from NonEquipItems to storage multiple times in quick succession, or by moving the last item in storageItems to the player inn quick succession. I found it easier to reproduce using a campfire, but its all the same process.

There are paths in MoveItemToStorage and MoveItemFromStorage which can cause failure if the index is invalid. and If the client is capable of sending two messages with the same last index (easily repeatable if you set an onclick as move to or from storage) and the item being clicked multiple times is the last item in the NonEquipItems or storeage items the server will throw an out of range exception.

Following this, the character will no longer be able to interact with the world. If they warp, they wont' be able to leave the mapserver, if the log out they will get "Connection Rejected by Server" and then after retrying they will enter the game alongside their duplicate self. If they log out again, they will log in where their original was with another duplicate continually making more until that mapserver is rebooted. They are effectively also unable to progress.

image2 image

There's probably a more elegant way to fix it, but just adding index checks in those methods at the start does the job,

            if (inventoryType == InventoryType.NonEquipItems)
            {
                if (inventoryItemIndex >= playerCharacter.NonEquipItems.Count)
                {
                    gameMessage = UITextKeys.UI_ERROR_INVALID_ITEM_INDEX;
                    return false;
                }
            }

            if (storageItemIndex >= storageItems.Count)
            {
                gameMessage = UITextKeys.UI_ERROR_INVALID_ITEM_INDEX;
                return false;
            }

Here's a stack trace from a 180c build

ERROR No Tag [2023-02-08 15:56:05] - ArgumentOutOfRangeException: Index was out of range. Must be non-negative and less than the size of the collection. Parameter name: index System.Collections.Generic.List1[T].get_Item (System.Int32 index) (at <75633565436c42f0a6426b33f0132ade>:0) System.Collections.ObjectModel.Collection1[T].get_Item (System.Int32 index) (at <75633565436c42f0a6426b33f0132ade>:0) MultiplayerARPG.CharacterInventoryExtensions.MoveItemToStorage (MultiplayerARPG.IPlayerCharacterData playerCharacter, System.Boolean storageIsLimitWeight, System.Single storageWeightLimit, System.Boolean storageIsLimitSlot, System.Int32 storageSlotLimit, System.Collections.Generic.IList1[T] storageItems, System.Int32 storageItemIndex, MultiplayerARPG.InventoryType inventoryType, System.Int32 inventoryItemIndex, System.Int32 inventoryItemAmount, System.Byte equipSlotIndexOrWeaponSet, MultiplayerARPG.UITextKeys& gameMessage) (at <5dd3b50e606143b29b8006154066f7ac>:0) MultiplayerARPG.MMO.MMOServerStorageMessageHandlers.HandleRequestMoveItemToStorage (LiteNetLibManager.RequestHandlerData requestHandler, MultiplayerARPG.RequestMoveItemToStorageMessage request, LiteNetLibManager.RequestProceedResultDelegate1[TResponse] result) (at <5dd3b50e606143b29b8006154066f7ac>:0) UnityEngine.Debug:LogException(Exception) Cysharp.Threading.Tasks.UniTaskScheduler:PublishUnobservedTaskException(Exception) MultiplayerARPG.MMO.<HandleRequestMoveItemToStorage>d__5:MoveNext() Cysharp.Threading.Tasks.CompilerServices.AsyncUniTaskVoid1:Run() Cysharp.Threading.Tasks.AwaiterActions:Continuation(Object) Cysharp.Threading.Tasks.UniTaskCompletionSourceCore1:TrySetResult(AsyncResponseData1) Cysharp.Threading.Tasks.CompilerServices.AsyncUniTask2:SetResult(AsyncResponseData1) MultiplayerARPG.MMO.d159:MoveNext() Cysharp.Threading.Tasks.CompilerServices.AsyncUniTask2:Run() Cysharp.Threading.Tasks.AwaiterActions:Continuation(Object) Cysharp.Threading.Tasks.UniTaskCompletionSourceCore1:TrySetResult(AsyncResponseData1) Cysharp.Threading.Tasks.CompilerServices.AsyncUniTask2:SetResult(AsyncResponseData`1) LiteNetLibManager.d232:MoveNext() Cysharp.Threading.Tasks.CompilerServices.AsyncUniTask2:Run() Cysharp.Threading.Tasks.Internal.ContinuationQueue:RunCore() Cysharp.Threading.Tasks.Internal.ContinuationQueue:Run()`

insthync commented 1 year ago

Thank you

Monty001 commented 1 year ago

No problem, there's no way I would have developed and released what I have without you. I've learned an incredible amount from your code base.

So my thanks goes to you.

insthync commented 1 year ago

Applied to 1.80c3, but something like this may occur later in other places, easiest way to avoid it is to add try-catch cover functions which having problems