MultiThread + Mutex + Gadget-Command = Bug?

[pcfreak]

well.. had some trouble to find the root of the problem at first to make a
short example of the bug...

Code: Select all

Declare.l thread1(var.l)
Declare.l thread2(var.l)
Declare.l thread3()

Global lock.l
lock = CreateMutex()

OpenWindow(0,0,0,320,40,"Window",#PB_Window_SystemMenu|#PB_Window_MinimizeGadget|#PB_Window_ScreenCentered)

If CreateGadgetList(WindowID(0))
 TextGadget(1, 10, 10, 300, 20, "")
EndIf

CreateThread(@thread1(), 0)
CreateThread(@thread2(), 0)

CreateThread(@thread1(), 0)
CreateThread(@thread2(), 0)

; this will cause a deadlock if thread3() is called..
; if thread3() was removed from the loop no deadlock occurs
Repeat : thread3() : Until WindowEvent()=#PB_Event_CloseWindow

Procedure.l thread1(var.l)
 Repeat
  LockMutex(lock)
   SetGadgetText(1, "thread1")
  UnlockMutex(lock)
  Delay(1)
 ForEver
EndProcedure

Procedure.l thread2(var.l)
 Repeat
  LockMutex(lock)
   SetGadgetText(1, "thread2")
  UnlockMutex(lock)
  Delay(1)
 ForEver
EndProcedure

Procedure.l thread3()
 LockMutex(lock)
  SetGadgetText(1, "thread3")
 UnlockMutex(lock)
 Delay(1)
EndProcedure

test it with thread-safe option turned on.
if you remove the thread3() call as stated in the comment it will work just fine,
but getting no response at all after compile if i let the thread3() call as it.
i don't exactly know what the real cause of this is but anyway, need this to be working!

[netmaestro]

You have a LockMutex in the main loop. This is a mistake because LockMutex waits and nothing must be allowed to stop the main loop for any significant time as the Window events will pile up and cause a hang.

For locks in the main loop, use TryLockMutex, which will return success or not and if you get success, do the codeblock followed by UnlockMutex. If 0 is returned, you simply keep looping and processing window events until you get a successful lock:

Code: Select all

Procedure.l thread3() 
 If TryLockMutex(lock) 
   SetGadgetText(1, "thread3") 
   UnlockMutex(lock) 
 EndIf
EndProcedure

Also, put a delay in your main loop to allow the other threads a timeslice.

[freak]

Sending Windows messages across threads causes a wait until the message was
processed by the target thread. (which only happens in (Wait)WindowEvent())

So if you modify a gadget in a different thread the target thread must have a running
event loop or you will get a deadlock.

In your situation:
- thread holds mutex lock and calls SetGadgetText()
- thread sends message and waits for main thread to call WindowEvent()
- main thread is waiting for the mutex to be available before continuing with the next WindowEvent()
- deadlock

TryLockMutex() is a solution here, but it is best to have no mutex locking in the message loop as
it ensures that such a deadlock (which is not always obvious) cannot happen.

[netmaestro]

Hehe you read my mind:

Code: Select all

#SetGadgetText = #WM_APP+1

Declare.l thread1(var.l) 
Declare.l thread2(var.l) 
Declare.l thread3() 

Global lock.l, threadsrunning = #True
lock = CreateMutex() 

OpenWindow(0,0,0,320,40,"Window",#PB_Window_SystemMenu|#PB_Window_MinimizeGadget|#PB_Window_ScreenCentered) 

If CreateGadgetList(WindowID(0)) 
 TextGadget(1, 10, 10, 300, 20, "") 
EndIf 

t1 = CreateThread(@thread1(), 0) 
t2 = CreateThread(@thread2(), 0) 

Repeat
  ev = WaitWindowEvent(1)
  Select ev
    Case #SetGadgetText
      SetGadgetText(EventwParam(), PeekS(EventlParam()))
    Default
      thread3() 
  EndSelect
 Until ev=#PB_Event_CloseWindow 

threadsrunning = #False
WaitThread(t1)
WaitThread(t2)
End

Procedure.l thread1(var.l) 
 Repeat 
  LockMutex(lock) 
    PostMessage_(WindowID(0), #SetGadgetText, 1, @"thread1") 
  UnlockMutex(lock) 
  Delay(1) 
 Until Not threadsrunning
EndProcedure 

Procedure.l thread2(var.l) 
 Repeat 
  LockMutex(lock) 
  PostMessage_(WindowID(0), #SetGadgetText, 1, @"thread2")  
  UnlockMutex(lock) 
  Delay(1) 
 Until Not threadsrunning
EndProcedure 

Procedure.l thread3() 
  If TryLockMutex(lock) 
    SetGadgetText(1, "thread3") 
    UnlockMutex(lock) 
    Delay(1)
  EndIf
EndProcedure

and everyone seems to be getting equal time. Mind what freak said about not having a lock in the main loop at all, it's good advice. It costs nothing here because we're only doing one SetGadgetText, but if it were more time consuming, it would need its own thread or the loop will clog up.

[pcfreak]

isn't there a better solution than TryLockMutex()?
'cause this way makes it quite hard to synchronize the window thread with
other threads or to use shared memory blocks within different threads...

[hellhound66]

Removed.

[pcfreak]

the problem is that this won't help me if i want to access shared memory
data from threads and the window event loop if the threads are using
gadget commands...
the only solution i can see so far is to avoid such thing by putting all
functions which would access such memory blocks in an extra thread.
but that makes the whole thing quite tricky and overloaded. i had hope
to find a better way to solve this problem...

[netmaestro]

memory allocated via AllocateMemory is available to all threads and procedures, for variables, lists and arrays use Global or Shared.

[freak]

You just have to keep in mind that the gadget commands cause thread syncronisation too.
Just think of it as another mutex. There are plenty of ways to avoid a deadlock,
just as you would when you have two different mutex objects that need to be considered.

For example, whats the problem with TryLockMutex() ? The only difference is that the main thread will spend its "wait"-time processing
the window events instead of just locking in the LockMutex() command.
The main thread can still access the shared memory when the mutex is available.

On the other hand, why does the main thread need to constantly access the shared memory ?
Usually in a case like this, you have the main thread only to keep the gui alive and do all the
work in other threads.

And finally, you could simply protect the shared memory with a different mutex than the one used for the "SetGadget...()" access.
There is no problem if the main loop spends some wait time for a different mutex
when it unlocks *at some point* so it gets back to the event handling.
Only if you trigger the event while holding the mutex lock that the main thread
waits for is what causes the deadlock.
Of course in this scenario you again have 2 thread synchronisation objects that
you have to be careful about to avoid deadlocks.


The best is still to have no mutex lock in the main thread at all imho. Let it take care
of processing the events only and handle everything else from other threads.
This way is the least likely one to cause deadlocks.