Recently i’ve discovered a strange effect: a coroutine throws SynchronizationLockException (albeit it should run on Unity main thread so having a lock inside it should not be a problem). Could someone enlighten me about what’s going on (details follow)?
So, we have a Coroutine:
private IEnumerator NotifyInternal()
{
lock (observersLock)
{
notificationInProgress = true;
// ... some unimportant stuff with yields and
// ... nested coroutines sometimes
notificationInProgress = false;
}
}
This coroutine is called like that:
public void Notify()
{
if (!notificationInProgress) StartCoroutine(NotifyInternal());
}
Also, locking object (it that’s the case) it just a plain object private object observersLock = new object();
As far as i understand, all of these should happen on Unity’s main thread. And since lock is just a syntactic sugar over Monitor class, locking and unlocking should happen on the same thread and there should be matching number of locks and unlocks.
So, am I missing something here?
So far i’ve worked these theories:
Somewhere inside the runtime part of coroutine’s code is really running on a thread pool and that’s why part after yield can be run on other thread. (i doubt it in fact, because if that’s the case, many other things could break)
During yield (and thus saving the execution context) the runtime does destructing some objects (maybe including the underlying Monitor, which leads to premature unlocking).
Hence lock implies using try/finally inside, and try/catch does not work inside coroutines (another question is why?) something in IL code is just broken in such a case.
No, that’s not the case at all ^^. Yes coroutines run on the main thread. However the code in a coroutine is split up into separate chunks which actually finish whenever you hit a yield statement. So having a lock that overarches a yield statement means that the lock would exist even when the current “coroutine chunk” has finished. You should never do that anyways.locks should be hold only for the shortest amount of time possible. In most cases you just lock, copy relevant data into local variables and release the lock. Of course we don’t know what you’re actually doing here. However if you actually want to stop a background thread until it’s needed again, you usually want to use a WaitHandle inside the thread and have the coroutine signal it to indicate the thread to start working. You may use two wait handles if the coroutine / main thread should actually wait for the thread to finish its job.
Though as I said, this highly depends on the usecase which you said is not important ^^.
Coroutines are not threads. They do not safe the execution context like a thread would. They ARE an execution context and represent a statemachine instead. See my coroutine crash course for more details.
As I said, your coroutine is not a “method” but it literally torn apart at yield statements into synchronous statemachine steps. So a lock that contains a yield should never be used as the continuation of the statemachine / iterator is not guaranteed in any way. So this is a very good way to introduce dead locks out of nowhere.
It’s possible that Unity actually checks for locks being active when an iterator stage returns to the scheduler. Though we don’t know the details about Unity’s coroutine scheduler. Maybe Unity itself wraps the scheduler in it’s own monitor section and that may mess up the monitor you’re using.
What exactly is “observersLock” and are you sure that object NEVER changes? By change I mean that it is replaced by another object. What type is observersLock?
Yeah, i understand that and that is a purpose - to prevent background threads from interacting with this class while coroutine is running. In fact, coroutine enumerates some collection and i’m trying to prevent this collection modification while it goes (and no, i don’t want to copy collection before enumerating because this will imply a bunch of new allocations on heap, which are undesirable here).
Yes, you’re right, i’ve used wrong formulation. On “saving a context” I meant “saving all needed variables into underlying class (you’ve mentioned in your course) and passing control to Unity’s coroutine scheduler”,. something like thatt.
And that’s also understandable. In fact, here i’m trying to achieve something like “do Monitor.Enter in one place, run a long action and do Monitor.Exit on that actions’ callback”. That’s a big fragile (yeah) and i understand that. The question is, why it breaks, not deadlocks.
Well, that’s kind of information i’d like to know. Maybe someone from Unity Technologies have an answer, that should be interesting.
That’s a plain System.Object which is created once and is used only for this lock (i’ve tried to lock on the enumerated collection itself, but dropped this idea because collection could change in the middle).
Well, what Unity version do you use? I just tried a repo-case and wasn’t able to reproduce the issue. So there must be something in your code that does something strange. I created a normal coroutine that yields in between the lock and I created a thread that also locks on the same object. No issues. My test project is in Unity version 2021.3.14f1
Test case
using System.Collections;
using System.Threading;
using UnityEngine;
public class CoroutineLock : MonoBehaviour
{
Thread th;
void Start()
{
th = new Thread(MyThread);
th.Start();
StartCoroutine(Test());
}
object lockObj = new object();
IEnumerator Test()
{
yield return null;
Debug.Log("start");
// aquires the lock for about 5 seconds
for (int i = 0; i < 5; i++)
{
lock (lockObj)
{
yield return new WaitForSeconds(1f);
Debug.Log("coroutine " +i);
}
yield return new WaitForSeconds(0.1f);
}
Debug.Log("finished");
}
void MyThread()
{
//about 10 seconds in the ideal case
for(int i = 0; i < 50; i++)
{
lock(lockObj)
{
Thread.Sleep(100);
}
Thread.Sleep(100);
Debug.Log("thread " + i);
}
Debug.Log("thread finished");
}
}
So this does run normally when testing in the editor. Do you use async stuff on a threadpool or something like that? The exception you got specifically would tell you that you try to release a lock you don’t even have at that point.
That was very kind of you to make repro project. I’m using Unity 2020.3.48f1.
Yeah, in the editor it works for me as well. The problem appears on Android build. I’ll try to make minimal reproduction of the case, just later.
Also yes, exception tells us that we don’t have that lock, hence i’ve assumed something is destructing Monitor object (releasing the lock) on coroutine exit.
Hmm, just tried my test script on my Galaxy A03s and it runs equally well. I added a log callback and added the whole log to a list of strings which I quickly displayed in OnGUI in a scrollview ^^. I also checked adb logcat and no exceptions. So it must be something specific to your code.
Well, i’ve done some investigation on my side and it seems that bug appears when i call Notify() not from Unity main thread.
Of course, that’s a bug (because one should not call Unity API on thread other than Unity’s main). And it seems reasonable that coroutine functionality breaks with different errors in such a case.
I’ve expected here a more specific exception (something like “You should call Unity API from main thread only”) though. Also interesting is that everything (including coroutine) seems working fairly well except for lock mechanism.
My guess about “what’s happening inside” is that first part of coroutine (with implicit Monitor.Enter call) is executed on thread which started the coroutine. But everything after first yield executes on main thread, hence SynchronizationLockException when trying to call Monitor.Exit on another thread.