MessageManagerLock and thread shutdown

#1

I’ve written a thread that uses MessageManagerLock to change the UI. However, I sometimes get a deadlock when I try to shut down the thread.

The thread looks something like this:

[code]void hwthread::run()
{
DBG(“hwthread::run”);

loop:
wait(50);
if (threadShouldExit())
goto exit;

…message loop could potentially acquire the lock here…

// grab the lock and do something useful
{
MessageManagerLock mmlock;
…do stuff here…
}

goto loop;

exit:
DBG(“hwthread exiting”);
return;
}
[/code]

The call to stopThread happens in the context of the message loop (a buttonClicked callback). So - if the MessageManager happens to grab the lock as shown above and then wait for the thread to exit, the thread will never exit - it’s stuck waiting for the lock.

If there was a MessageManagerUnlock class, then it would be straightforward - I could just unlock the message loop temporarily while stopping the thread.

Other than that, I’m stumped. For this particular case, thread I can just use a Timer or an ActionBroadcaster instead.

But, I’m curious - anyone got any bright ideas? Am I just being dense?

Matt

#2

Good lord above! :shock:

goto!

#3

Yeah, those gotos did raise my eyebrow too, but not as much as the problem you’ve come across.

I could add lock/unlock methods to messagemanager, but that makes the whole thing unstable, because if you unlock the main event thread then all the other threads will get a chance to execute, not just the one you’re interested in…

I guess what you really want is to make the MessageManagerLock fail if it can’t get the lock within a certain time - then you could loop, trying to get the lock until it either works or the threadShouldExit flag gets set.

Looks like I should have a bit of a re-think of this stuff!

#4

I don’t like your code, as it is sure to deadlock…

void hwthread::run()
{
   DBG("hwthread::run");
   
loop:
   wait(50);
   // This check should be done while the lock is held not here
   if (threadShouldExit())
      goto exit;
   
   ....message loop could potentially acquire the lock here....

   // grab the lock and do something useful
   {
      MessageManagerLock mmlock;
      .....do stuff here.....
   }
   
   goto loop;

exit:
   DBG("hwthread exiting");
   return;
}

I would have written :

void hwthread::run()
{
   DBG("hwthread::run");
   
   bool bExit = threadShouldExit();

   while (!bExit)
   {
        
       // grab the lock and do something useful
        {
           MessageManagerLock mmlock;
           .....do stuff here.....
           bExit = threadShouldExit();
        }
 
       ....message loop could potentially acquire the lock here....
       wait(50);
    }
   
   DBG("hwthread exiting");
   return;
}

The reason is quite simple, in your code, there is no warranty that the thread won’t be interrupted between the “goto loop” and “goto exit” code.
If it is, then the other thread might wait for your UI thread to answer a message. Your UI thread is waiting to know if it can exit (not in your sample code but I guess it is what you are doing), so it won’t answer the message from the other thread => deadlock.

If you move the threadShouldExit code inside the locked section, you can make sure that :

  1. Once you are in the locked section, the other thread can not send you a message (because it will requires locking a locked lock which is impossible)
  2. You will be sure that the value you are reading is good at the reading time (while you are not in your code). For example, if the thread is interrupted inside the threadShouldExit function, and the value is changed inside another thread at the same time, then the result is surely weird (like say the UI thread reads 0, the other thread writes 1, but is then interrupted, and the UI thread write back its 0 because its cache hold 0). This is because reading and writing are not atomic on x86 processor (nor PPC, etc…) by default.

If you want more information, you can use the thread deadlock detector I’ve written here.

For sure, the idea is always the same:

  1. as long as you want to access a shared data (like the thread stop condition), you should lock it.
  2. Locking should always occurs in the same order in all thread
  3. Deadlock are naughty boys (they always show up when you are not looking for them)
#5

That’s a lot better, but the problem’s still there.

If the UI thread calls stopThread(), this will signal that the thread should exit, but it won’t release the lock while it’s waiting for the thread to stop. So the thread can’t enter its locked section to find out whether it should quit.

#6

Yeah, that’s exactly what he is experiencing. The solution is to move the stopThread() call outside the locked area (and caching the result like I did in my example). This has to be done in the code he haven’t disclosed.

Another solution might be to broadcast a signal (through pthread_cond_wait/broadcast under linux, an SetEvent under Windows, etc…) and have an interruptible wait in the lock manager.

Anyway, stopThread shouldn’t wait for the thread to stop with the lock locked (so maybe you should change stopThread method to have a “lock” reference argument so it could unlock it while waiting for the thread to really stop (and signal it has stopped)). I don’t know what stopThread code does.

In my code, I usually have 2 objects for sync. The first one is used to protect the thread data, and the second one is signaled just before leaving the thread.
The thread never waits on the second object so other threads can safely wait on it as long as they have released the thread’s first object for waiting for finish. (This solved a week of debugging under Windows while waiting for a thread to finish using WaitForSingleObject(hThread, INFINITE) in a DLLMain Process_Detach handler that deadlocked because Windows usually call the handler when a thread detach, and the handler was already waiting for the thread to finish… Using a second object prevented Windows from stalling).

#7

Sure, but the real problem here is that he can’t release the lock before calling stopThread(), because it’s the message manager’s internal callback lock. This is always held by the UI thread during an event callback, and can’t be released because that’d be dangerous.

#8

Exactly.

I use gotos because I spend a lot of time writing embedded assembly language and that’s how I think. I didn’t realize structured programming was still in vogue. :slight_smile:

Looks like MessageManagerLock won’t work for my purposes; I’ll use Timer, ActionBroadcaster, or ChangeBroadcaster.

Thanks for the comments.

Matt

#9

Okay, I haven’t understood this way.

So maybe you should try to avoid locking MessageManagerLock (and setup your own lock) (if you still want to use a thread, but the Timer solution might work too).

So, Jules, you’ll have to refactor this.

#10