Monitor (Threading) Locking Problem

Started by
9 comments, last by Lunatix 11 years, 11 months ago
Hey Everyone!

I have a little problem with my game... i have a worker thread for updating and calculating objects (matrices for opengl) in the background.
This static class called "Driver" holds a thread handle and two lists - one for camera's, one for other objects. The "Driver" checks continously for changes of the objects, iteration through the two lists. If an object has changes, its "Calculate()" method is called by the "Driver".

Each time of iteration through one of the Lists, the list is locked:


Monitor.Enter(CameraList);
try{
foreach (BaseCamera Cam in CameraList){
if (!(Cam.isLocked) && (Cam.isCalcRequired())){
Cam.onUpdate();
}
}
}finally{
Monitor.Exit(CameraList);
}
Monitor.Enter(ObjectList);
try{
foreach (BaseObject obj in ObjectList){
if (!(obj.isLocked) && (obj.isCalcRequired())){
obj.onUpdate();
}
}
}finally{
Monitor.Exit(ObjectList);
}


All fine. But sometimes I get a deadlock when adding an object - i call a static "AddObject" method which looks like this:

public static bool AddObject(BaseObject Object){
if (Object.Ident == ObjectIdent.Camera){
Monitor.Enter(CameraList);
try{
CameraList.Add((BaseCamera)Object);
return true;
}finally{
Monitor.Exit(CameraList);
}
}else{
Monitor.Enter(ObjectList);
try{
ObjectList.Add(Object);
return true;
}finally{
Monitor.Exit(ObjectList);
}
}

return false;
}


I think, the deadlock appears, because the WorkerThread iterates to fast and the AddObject function is unable to lock one of the lists. But to solve the problem, i had to add "Thread.Sleep(100)" after updating both lists... is there a simple way to avoid such problems? Sleeping for 100Ms makes everything very laggy ;)

P.s.: I tried other variants with Semaphores and Mutex but none of them solved the deadlock problem =/
Advertisement
You can simplify your Monitor.Enter -> try -> finally -> Monitor.Exit pattern with C#'s "lock" keyword. (This won't fix the deadlock, but it will simplify your code)

When you encounter a deadlock, suspend the process in the debugger. You should be able to see where each thread is stuck, and backtrack what's causing it from there. There is too little code shown for me to speculate what is causing it.

Sleeping is never a proper solution for deadlocks anyway; it will just decrease the chances of deadlocking rather than completely fix it. Besides, exactly as you said, the performance hit is unacceptable. However, you can sometimes use sleeps INSIDE the locked section of code to cause the deadlocks to happen more easily when you want to discover what's causing them.
I tested it with the sleeps in the locked sections, and i got an exception: "object synchronization method was called from an unsynchronized block of code". It happens, when I clicked on the openfile dialog of my editor and call a static function, which creates and loads some new objects - which call the AddObject method. The Form is the editors main form... some code:


public static bool AddObject(BaseObject Object){
if (Object.Ident == ObjectIdent.Camera){
Monitor.Enter(CameraList);
try{
Thread.Sleep(20);
CameraList.Add((BaseCamera)Object);
return true;
}finally{
Monitor.Exit(CameraList);
}
}else{
Monitor.TryEnter(ObjectList,50);
try{
ObjectList.Add(Object);
return true;
}finally{
Monitor.Exit(ObjectList); //<- exception is thrown here
}
}

return false;
}


And some code of the form:

public partial class MainForm : Form{
delegate void SetTextCallback(string text); //tried it with a callback
private void openToolStripMenuItem_Click(object sender, EventArgs e){
OpenPartDlg.InitialDirectory = Application.StartupPath+"../";
if (OpenPartDlg.ShowDialog(this) == DialogResult.OK){
if (InvokeRequired){
LoadPartCallback d = new LoadPartCallback(LoadModel);
Invoke(d, new object[] {OpenPartDlg.FileName});
}else{
LoadModel(OpenPartDlg.FileName);
}
}
}
private void LoadModel(string FileName){
switch(ModelChunk.LoadModelChunk(FileName)){ //_______ LoadModelChunk is a static function which calls the chunk loader function and also creates new Objects like meshes
case ModelChunk.CreateResult.PathNotValid:
MessageBox.Show("Unable to create file", "Error");
break;
case ModelChunk.CreateResult.NameNotValid:
MessageBox.Show("Please define a valid name", "Error");
break;
}
}


This is the method which causes the deadlock:

namespace LunaGameEngine.Graphics.Object{
public class Mesh : BaseMesh{
public Mesh(bool UseDefaultPipe = true, int nTexCoordChannels = 1, bool _UseVbo = false){
UseVbo = _UseVbo;
if (nTexCoordChannels > 0){
TexCoords = new RenderBuffer[nTexCoordChannels];
for(int n=0; n<nTexCoordChannels; n++){
TexCoords[n] = new RenderBuffer();
}
}
BaseDriver.AddObject(this); //At this point, the object should be inserted into the drivers calulation list(s)
if (UseDefaultPipe){
RenderPipe.AddObject(this);
}
}



And some of the BaseDriver class:

public class BaseDriver{
public static bool Initialize(int _flags){
if (!isInitialized){
isInitialized = true;
if (_flags == Luna.AutoCalc){
updateThread = new Thread(Execute);
}
}
return true;
}
public static void Execute(){
Luna.ConsoleMessage("Execute");
try{
isPaused = false;
while (!ThreadStop && updateThread.ThreadState == ThreadState.Running){
Monitor.Enter(CalcLock);
try{
try{
doCalculations();
}catch(ThreadInterruptedException e){
Luna.ExceptionMessage(e);
break;
}
}finally{
Monitor.Exit(CalcLock);
}

Thread.Sleep(10);
}
isPaused = true;
Luna.ConsoleMessage("Driver stopped");
}catch(ThreadAbortException e){
Luna.ExceptionMessage(e);
}
}
public static void doCalculations(){
Thread.Sleep(10);
Monitor.Enter(CameraList);
try{
Thread.Sleep(20);
foreach (BaseCamera Cam in CameraList){
if (!(Cam.isLocked) && (Cam.isCalcRequired())){
Cam.onUpdate();
}
}
}finally{
Monitor.Exit(CameraList);
}

Monitor.Enter(ObjectList);
try{
foreach (BaseObject obj in ObjectList){
if (!(obj.isLocked) && (obj.isCalcRequired())){
obj.onUpdate();
}
}
}finally{
Monitor.Exit(ObjectList);
}
}
public static void Start(){
if (updateThread != null){
updateThread.Start();
}
}
public static void Stop(){
if (updateThread != null){
updateThread.Abort();
}
}



I hope this helps... and i know about the lock keyword, but does it really the same as try...finally ? Because finally guarantees that the mutex/sync object will be unlocked. The strange thing is, that it works good when calling AddObject from the programs main thread and from another small "CreateNew" form (but this form is not bound to the main form and closes when the user clicks "ok")...

I hope this helps... and i know about the lock keyword, but does it really the same as try...finally ? Because finally guarantees that the mutex/sync object will be unlocked.
[/quote]
From the documentation of Monitor.Enter():

Use a C# try…finally block (Try…Finally in Visual Basic) to ensure that you release the monitor, or use the C# lock statement (SyncLock statement in Visual Basic), which wraps the Enter and Exit methods in a try…finally block.
[/quote]
(emphasis added).


Sleeping is never a proper solution for deadlocks anyway; it will just decrease the chances of deadlocking rather than completely fix it
[/quote]
Quoted for emphasis.


I tested it with the sleeps in the locked sections, and i got an exception: "object synchronization method was called from an unsynchronized block of code"
[/quote]
That TryEnter looks suspicious. You should be checking its return value.
[font=arial,helvetica,sans-serif]
Cam.onUpdate();

obj.onUpdate();
[/quote]
Does any of these two calls lock inside?

P.s.: I tried other variants with Semaphores and Mutex but none of them solved the deadlock problem[/quote]

The proper way to fix this is to light 4 candles.


Deadlocks occur when locks are acquired in different order. If there are two threads acquiring A->B and B->A, then a deadlock will occur. If locks are always acquired in same order, it won't.


The overall design is incredibly fragile as well. It can be made to work right, but requires a lot of infrastructure in place. Event-based callbacks are essentially a no-go for threaded code.

Instead, there's a simple solution:
List<Something> updates;

while (running) {
List<Callbacks> callbacks;
lock(updates) {
// add everything from updates to objectList
// add all added objects to callbacks
updates.clear();
}

// fire onAdded events for all items in callbacks

doCalculations();
}

void addObject(...) {
lock(updates) {
updates.add(...);
}
}
[/font]


[font=arial,helvetica,sans-serif]Such design is robust and cannot deadlock. Converting it to actual C# code left as exercise to the reader.[/font]


I tested it with the sleeps in the locked sections, and i got an exception: "object synchronization method was called from an unsynchronized block of code"

That TryEnter looks suspicious. You should be checking its return value.
[/quote]

Indeed! This is exactly what will cause that exception; If TryEnter fails (or if you forget to call Enter/TryEnter at all), calling Exit throws that exception. The reason it is failing now is because your sleep in the other thread is keeping the wait handle locked for a much longer time, allowing TryEnter to fail more easily.
Okay, Antheus & Nypyren, i got that =)
I changed the Driver class like Antheus said and everything works fine now - except one thing:

why should i fire an event "onAdded" when the Object is always in my ObjectList? Or... should each object which detects that it has to be updated by the driver fire an "Add" or "Calulate" event? And in the meantime, the driver thread will pause?
why should i fire an event "onAdded" when the Object is always in my ObjectList? Or... should each object which detects that it has to be updated by the driver fire an "Add" or "Calulate" event? [/quote]

I merely demonstrated on how to dispatch callbacks. Instead of doing it from main loop from inside a lock (each callback may take a long time, acquire locks and such), you instead defer them so they can run later outside of a lock.

The events you have are onUpdate(), executed from inside a lock. Instead, prefer to use the example above. While inside a lock, put the objects to be notified into a queue, then notify them without holding a lock.
The easiest way to solve this is have two lists; one for read, one for write.

When you add something to the scene construct it add as it to the 'write' queue.

Later, at a fixed point in your frame, flip the queues so that read becomes write and write becomes read then process all the 'add' requests from the (now) read queue, clear it and then just carry on like normal.

The 'add' and 'swap' functions will need to share a mutex but that's about it.

edit:
Heck, you could even go one step further and use System.Collections.Concurrent.ConcurrentQueue<T>

use cq.Enqueue() to add objects to the queue from your call backs
then in your 'update' loop simply a loop which calls 'TryDeque' to extract objects from it, either for all entries or for a set number of you want to limit it.

Your locks are gone, your problem is simplified :)

The 'add' and 'swap' functions will need to share a mutex but that's about it.

For instance...

private object swapLockObject = new Object();
private List<Things> readList;
private List<Things> writeList;

void SwapLists() {
lock(swapLockObject) {
List<things> tmp = readList;
readList = writeList;
writeList = tmp;
}

foreach(Thing t in readList) {
GiveHappyCancer(t);
}
}

void AddThing(Thing t) {
lock(swapLockObject) {
writeList.Add(t);
}
}

In time the project grows, the ignorance of its devs it shows, with many a convoluted function, it plunges into deep compunction, the price of failure is high, Washu's mirth is nigh.

This topic is closed to new replies.

Advertisement