Error "InvalidOperationException: Collection was modified; enumeration operation may not execute." in NetworkRoomManager

[joshuameurey]

Describe the bug
Sometimes when changing scene when Mirror is up an running we face the following error:
"**InvalidOperationException: Collection was modified; enumeration operation may not execute.**"

Full stack

InvalidOperationException: Collection was modified; enumeration operation may not execute.
System.Collections.Generic.List`1+Enumerator[T].MoveNextRare () (at <a3b02d6f9b494355b946095ea1f25c54>:0)
System.Collections.Generic.List`1+Enumerator[T].MoveNext () (at <a3b02d6f9b494355b946095ea1f25c54>:0)
Mirror.NetworkRoomManager.OnServerSceneChanged (System.String sceneName) (at Assets/Mirror/Components/NetworkRoomManager.cs:366)
Mirror.NetworkManager.FinishLoadSceneHost () (at Assets/Mirror/Core/NetworkManager.cs:1077)
Mirror.NetworkManager.FinishLoadScene () (at Assets/Mirror/Core/NetworkManager.cs:1020)
Mirror.NetworkManager.UpdateScene () (at Assets/Mirror/Core/NetworkManager.cs:998)
Mirror.NetworkManager.LateUpdate () (at Assets/Mirror/Core/NetworkManager.cs:288)

[IMPORTANT] How can we reproduce the issue, step by step:
Please tell us how to reproduce your issue, STEP BY STEP, with one of our built in examples.
-> Connect to online game scene, single player (host) is enough
-> Change scene to another (keeping the connectivity)
-> Go back to first online opened scene: the error occurs
Any bug that can be reproduced, can be fixed.
If we can't reproduce it, we can't fix.

Expected behavior
No error at all.

Code quick review
the method public override void OnServerSceneChanged(string sceneName) from NetworkRoomManager.cs (line 366) is doing this:
foreach (PendingPlayer pending in pendingPlayers) SceneLoadedForPlayer(pending.conn, pending.roomPlayer);

But in the method called during this foreach iteration we have (same source, line 131):

if (Utils.IsSceneActive(RoomScene))
{
    // cant be ready in room, add to ready list
    PendingPlayer pending;
    pending.conn = conn;
    pending.roomPlayer = roomPlayer;
    pendingPlayers.Add(pending);
    return;
}

Screenshots
N/A

Desktop (please complete the following information):

• OS: Windows 11 Pro
• Build target: Windows
• Unity version: 2022.3.14f1
• Mirror branch: official 86.13.4

Additional context
Maybe replacing foreach by a for should be considered ?

[MrGadget1024]

Can't reproduce with the Room example that ships with Mirror...what step am I missing?

[joshuameurey]

Hi, as you may know, it's not allowed in C# to add/remove/change order a list within a foreach iteration. The code we have here do this in certain case (did you test a NetworkRoomManager, this one produces the error).

Since I reproduce this every time, I've adapted the code in this way:

• Add "protected virtual" to method "void SceneLoadedForPlayer(NetworkConnectionToClient conn, GameObject roomPlayer)" (NetworkRoomManager, line 121)
• Add an override in my own NetworkManager to bypass the code present in "public override void OnServerSceneChanged(string sceneName)" (NetworkRoomManager line 361) doing this:

public override void OnServerSceneChanged(string sceneName)
{
    if (sceneName != RoomScene)
    {
        // call SceneLoadedForPlayer on any players that become ready while we were loading the scene.
        for (int index = pendingPlayers.Count - 1; index >= 0; index--)
        {
            PendingPlayer pending = pendingPlayers[index];
            SceneLoadedForPlayer(pending.conn, pending.roomPlayer);
        }

        pendingPlayers.Clear();
    }

    OnRoomServerSceneChanged(sceneName);
}

[MrGadget1024]

Hi, as you may know, it's not allowed in C# to add/remove/change order a list within a foreach iteration. The code we have here do this in certain case (did you test a NetworkRoomManager, this one produces the error).

The Room example uses NetworkRoomManager, and doesn't produce this error. Try it yourself.

I know about not changing a collection from within a foreach...I'm asking how you're getting into the condition to produce the error.

[miwarnec]

Hey @joshuameurey this can happen if your game script accidentally modifies the collection while mirror is using it.
I recently helped another project to reproduce this and build 'SafeCollections' for C#:
https://github.com/miwarnec/CSharp-SafeCollections

Swap the List/Dictionary/etc. with SafeList/Dictionary/etc. and try again.
It will tell you the two places where the list is modified, which then helps debug.

[joshuameurey]

Hi, thanks for this feedback @miwarnec, I'll give it a try ! Again, many thanks.

[miwarnec]

If you manage to reproduce it and think it's a mirror bug, then please reopen the issue!