Multi-threaded usage with Unity

[taldim]

Hi,

I'm using this library for an AI in a Unity game. Unity is supposed to use only one thread by default, but using this library the planning is done in another thread.

When I debug, I get the same error every time:
InvalidOperationException: Collection was modified; enumeration operation may not execute.
System.Collections.Generic.Dictionary2+Enumerator[TKey,TValue].MoveNext () (at <a40b0ad4a868437393ad434631fb6ff1>:0) System.Linq.Enumerable.ToDictionary[TSource,TKey,TElement] (System.Collections.Generic.IEnumerable1[T] source, System.Func2[T,TResult] keySelector, System.Func2[T,TResult] elementSelector, System.Collections.Generic.IEqualityComparer1[T] comparer) (at <53701cec7cfb4b5ba84d9e7813b871a8>:0) System.Linq.Enumerable.ToDictionary[TSource,TKey,TElement] (System.Collections.Generic.IEnumerable1[T] source, System.Func2[T,TResult] keySelector, System.Func2[T,TResult] elementSelector) (at <53701cec7cfb4b5ba84d9e7813b871a8>:0)
MountainGoap.CopyDictionaryExtensionMethod.Copy (System.Collections.Generic.Dictionary2[TKey,TValue] dictionary) (at Assets/mountain-goap-0.10.0/MountainGoap/Internals/CopyDictionaryExtensionMethod.cs:19) MountainGoap.ActionNode..ctor (MountainGoap.Action action, System.Collections.Generic.Dictionary2[TKey,TValue] state, System.Collections.Generic.Dictionary`2[TKey,TValue] parameters) (at Assets/mountain-goap-0.10.0/MountainGoap/Internals/ActionNode.cs:21)
MountainGoap.Planner.Plan (MountainGoap.Agent agent, System.Single costMaximum) (at Assets/mountain-goap-0.10.0/MountainGoap/Internals/Planner.cs:26)
MountainGoap.Agent.b__62_0 () (at Assets/mountain-goap-0.10.0/MountainGoap/Agent.cs:187)
System.Threading.ThreadHelper.ThreadStart_Context (System.Object state) (at :0)
System.Threading.ExecutionContext.RunInternal (System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, System.Object state, System.Boolean preserveSyncCtx) (at :0)
System.Threading.ExecutionContext.Run (System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, System.Object state, System.Boolean preserveSyncCtx) (at :0)
System.Threading.ExecutionContext.Run (System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, System.Object state) (at :0)
System.Threading.ThreadHelper.ThreadStart () (at :0)
UnityEngine.<>c:b__0_0(Object, UnhandledExceptionEventArgs)

It seems that the error claims that I change the state of an agent in the middle of an enumeration.
This is probably done by changing the state in another thread.
I have only one agent in the system, and the state is changed only in the sensors (that I add to the agent in the constructor), and using the build-in mechanism for postconditions.
There aome keys (strings) for the state that I access both in the sensors and the pos-conditions, maybe this is the problem.
From the documentation it seems that the sensors should change the agent's state, so I assume a sensor should not run in parallel to an Enumaration

I can understand why if the executor will change the agent's state that might cause the problem, but I made sure that the executor only reads the state, and never changes it.

What might be the problem?

[caesuric]

Can you give me some more details about the issue? I suspect that you are modifying the state outside of a sensor based on what you are telling me, but it is hard to know without seeing some code examples.

If you need a reference for implementation in Unity, I have a full working real-time Unity game that uses MountainGoap in the monster AI without issues here: https://github.com/caesuric/familiar-quest/blob/master/FamiliarQuestMainGame/Assets/Scripts/AI/MGMonsterAI.cs

[taldim]

Thank you for the fast reply!

I read your example and my code, and I found two things that I may suspect:

1. On the executor I do not change the state, but I do access it. In your example you only do agent.state[X].Equals(Y), but I pass the state to a checker function, for example:
   bool checker( Dictionary<string, object?> state){
   if (state[LOOT] is not int loot)
   return false;
   return loot > 4;
   }

ExecutionStatus executor(Agent agent, Action action){
....
if(checker(agent.State)) return ExecutionStatus.Succeeded;
return ExecutionStatus.Failed;
}

2. In the state dictionary, I put some Monobehaviour objects (In the value part, not the key). Maybe this is problematic since the Unity main thread changes their content in the main thread? I can put them in the "memory" and not in the "state", but if I do so, how do I access the memory inside a Cost callback function?

[caesuric]

I don't think that's quite the issue. If you look at my example, I have agent.State["playersInFieldOfVision"] = fov.players; where fov.players is a List<GameObject>. So it should be possible to store a list of GameObjects or MonoBehaviours in state.

After your follow up, I do have one idea, which is that Unity itself may be modifying the collection. When a GameObject is destroyed, it often shows up as null in lists afterwards, which could count as modifying the collection. Are you creating/destroying game objects or components outside of the executors? This could potentially cause the issue.

I'm not sure yet if the bug is in my code or yours, but one thing I often do to handle "collection modified while looping" type bugs is to call .ToList() on the object, even if it's already a list. This ensures that you are working with a copy and not the original, and protects you from this error in almost all cases. In the case of Dictionaries there is a similar pattern but I can't remember what it is... it might be that you need to call .Keys.ToList() or something similar, or just use a thread-safe dictionary. Or perhaps I need to check my code to ensure that I'm doing this in the proper places!

I'll check my code and see if I'm handling this properly.

[caesuric]

On review, I think this may be my mistake for not using a ConcurrentDictionary for state. The actual line of the error is when I try to make a copy of the state dictionary for purposes of iterating through it afterwards and avoiding threading issues. On a cursory review, it seems like Dictionary.ToDictionary() is perhaps not as thread safe as List.ToList().

For now, I'd suggest you double-check to make sure Unity isn't destroying game objects or components related to the ones stored in state, but even if it is, switching to a ConcurrentDictionary should probably fix this, and this library should ideally be able to handle items in state being destroyed by Unity anyway. 😄

EDIT: Stupid workaround until I fix this -- if you put a List of Unity objects into state instead of a Unity object directly, I don't think this bug will happen at all. That's what I'm doing in the Familiar Quest code example and I have never run into this.

[taldim]

Yes, Actually I do put in the state value a Unity object, and the error does occur after the object is destroyed.
I'm aware that Unity overloads the "== null" operator for destroyed objects, and in my code I check if the monobehaviour object is null, and then I return ExecutionStatus.Failed;
For example, if the action is to go pick-up a loot, and someone else has picked it up, the loot object is destroyed.

I will use the workaroud and update to the latest version when you fix it.
Thank you!

[caesuric]

How's the workaround functioning for you? Does putting the object in a list cause you any issues so far?

If it's not hurting anything right now, then I'm hoping to wait to roll out this fix since it will be a breaking change, and I like to batch those up with other breaking changes when possible for release management purposes.

[taldim]

Actually the workaround didn't work, the error kept on occuring.
When looking at the stack trace, it only happened in the constructor of ActionNode, in the State = state.Copy().
The error occurd even if I wrapped it in a list, or even had a "Context" class created, that the agent has an instance of. Even changes within the variables of this class caused the exception.
I think this exception occurs even if some internal variables of the state object are modified.

What solved it for me is to replace the line
State = state.Copy();
With the following:
while(true){
try{
State = state.Copy();
break;
}
catch (System.Exception e) {

            }
        }

Threre is probably a better way to fix this issue, but this is enough for me to continue programming for now, the exception (or any eception from your project) has occured since.
When you release a new version I would gladly merge it into the game.

Thank you!

[caesuric]

Thanks for letting me know! I'll incorporate the fix sooner rather than later if it's still breaking things for you.

[caesuric]

@taldim could you please see if this branch helps with your issue? #21

[caesuric]

I believe the PR I merged should fix this.