Threads, crashes, and EnterCriticalSection

paulr · Post by **paulr** » Mon Jan 17, 2005 8:03 pm

Code updated For 5.20+

Hi,

I'm trying to use threads to perform some intensive processing in the background of a program, dependant on the position of a slider gadget, while the slider gadget is being dragged around. Here's a program that demonstrates the kind of problems I'm having:

Code: Select all

Global CS.CRITICAL_SECTION
Global CSInitialized.l
Global value, ThreadRunning

Procedure Thread(Parameter)
  
  If CSInitialized = #False
    InitializeCriticalSection_(CS)
    CSInitialized = #True
  EndIf
  
  Debug "Thread started"
  
  Repeat
    
    EnterCriticalSection_(CS)
    tempvalue = value
    LeaveCriticalSection_(CS)
    change = #False
    
    For a = 1 To 50
      
      ; Processor-intensive delay loop: (causes crashes!)
      ;For b=1 To 50000
      ;  b+1
      ;  b-1
      ;Next
      
      ; Delay statement:
      Delay(50)
      
      ; If slider has moved, exit and restart the loop
      EnterCriticalSection_(CS)
      If value <> tempvalue
        LeaveCriticalSection_(CS)
        Debug "Value changed: " + Str(value)
        change = #True
        Break
      Else
        LeaveCriticalSection_(CS)
      EndIf
      
      Debug a
      
    Next
    
  Until change = #False
  
  Debug "Thread Finished"
  ThreadRunning = #False
  
EndProcedure


OpenWindow(0, 300, 400, 300, 120, "Threads demo", #PB_Window_SystemMenu)

TrackBarGadget(0,20, 40, 256, 40, 0, 255)

CreateImage(0, 300, 300)

Repeat
  EventID = WaitWindowEvent()
  
  If EventID = #PB_Event_Gadget And EventGadget() = 0
    value = GetGadgetState(0)
    If ThreadRunning = #False
      ThreadRunning = #True
      Debug "Creating thread"
      CreateThread(@Thread(), 0)
    EndIf
  EndIf
  
Until EventID = #PB_Event_CloseWindow

When the gadget moves, 'value' changes, and the thread is supposed to notice, and restart itself. It works fine with the processor-intensive bit commented out, as it is above, but when you put it back in, it gets very unstable. It gets even worse if you take out the Delay command.

I tried to implement the Critical Section commands as described in this post: viewtopic.php?t=11975.
I've figured out the crashes seem to be caused by accessing global variables, so I've tried protecting the commands involving 'value' with the Enter and Leave critical section commands. I've also tried putting the commands in different places but they don't appear to have any effect.

Can anyone help?

Thanks,
Paul

El_Choni · Post by **El_Choni** » Mon Jan 17, 2005 8:51 pm

I think this works a bit better:

Code: Select all

Global CS.CRITICAL_SECTION
Global CSInitialized.l
Global value, ThreadRunning

Procedure Thread(Parameter)
  EnterCriticalSection_(CS)
  Debug "Thread started"
  LeaveCriticalSection_(CS)
  Repeat
    EnterCriticalSection_(CS)
    tempvalue = value
    LeaveCriticalSection_(CS)
    change = #False
    For a=1 To 50
      ; Processor-intensive delay loop: (causes crashes!)
      For b=1 To 50000
        b+1
        b-1
      Next
      ; Delay statement:
;      Delay(50)
      ; If slider has moved, exit and restart the loop
      EnterCriticalSection_(CS)
      If value<>tempvalue
        Debug "Value changed: "+Str(value)
        LeaveCriticalSection_(CS)
        change = #True
        Break
      EndIf
      Debug a
      LeaveCriticalSection_(CS)
    Next
  Until change=#False
  EnterCriticalSection_(CS)
  Debug "Thread Finished"
  ThreadRunning = #False
  LeaveCriticalSection_(CS)
EndProcedure

If CSInitialized = #False
  InitializeCriticalSection_(CS)
  CSInitialized = #True
EndIf

OpenWindow(0, 300, 400, 300, 120, #PB_Window_SystemMenu, "Threads demo")
CreateGadgetList(WindowID())
TrackBarGadget(0,20, 40, 256, 40, 0, 255)

CreateImage(0, 300, 300)

Repeat
  EventID = WaitWindowEvent()
  If EventID=#PB_Event_Gadget And EventGadgetID()=0
    value = GetGadgetState(0)
    If ThreadRunning=#False
      ThreadRunning = #True
      Debug "Creating thread"
      CreateThread(@Thread(), 0)
    EndIf
  EndIf
Until EventID=#PB_EventCloseWindow
DeleteCriticalSection_(CS)

PolyVector · Post by **PolyVector** » Mon Jan 17, 2005 9:57 pm

this is not a very good way of achieving thread-safety... if you need a thread to be notified of some kind of change (like the slider) you should use the "event" commands rather than looping with a criticalsection... The benefit is that you don't waste CPU time and have very quick response...

Here's a quick rewrite of your code (I know it's rather sloppy, but you should get the idea of Events...)

Code: Select all

Global ChangeEvent.l
Global CS.CRITICAL_SECTION
Global value
Global ThreadID.l
#QuitValue=-1;/Signal that we should quit...

;/Some functions to clean up things:
Procedure SetValue(NewValue)
  EnterCriticalSection_(CS)
  If NewValue<>value
    value=NewValue
    PulseEvent_(ChangeEvent);/signal any waiting threads...
  EndIf
  LeaveCriticalSection_(CS)
EndProcedure

Procedure GetValue()
  Protected Result.l
  EnterCriticalSection_(CS)
  Result=value
  LeaveCriticalSection_(CS)
  ProcedureReturn Result
EndProcedure

Procedure TsDebug(String$);//Thread safe debug :)
  EnterCriticalSection_(CS)
  Debug String$
  LeaveCriticalSection_(CS)
EndProcedure

Procedure Thread(Parameter)
  TsDebug("Thread started")
  Repeat
    WaitForSingleObject_(ChangeEvent,#INFINITE);/wait for a signal that there's been a change...
    NewValue=GetValue()
    If NewValue=#QuitValue
      Break
    EndIf
    TsDebug("Value changed: "+Str(NewValue))
  ForEver
  TsDebug("Thread Finished")
EndProcedure


InitializeCriticalSection_(CS)
ChangeEvent=CreateEvent_(#Null,#True,#False,"MyChangeEvent")
ThreadID=CreateThread(@Thread(), 0)

OpenWindow(0, 300, 400, 300, 120, #PB_Window_SystemMenu, "Threads demo")
CreateGadgetList(WindowID())
TrackBarGadget(0,20, 40, 256, 40, 0, 255)

CreateImage(0, 300, 300)

Repeat
  EventID = WaitWindowEvent()
  If EventID=#PB_Event_Gadget And EventGadgetID()=0
    SetValue(GetGadgetState(0))
  EndIf
Until EventID=#PB_EventCloseWindow
SetValue(#QuitValue)
WaitForSingleObject_(ThreadID,1000);/give the thread 1 second to close... I wouldn't recomend using #INFINITE...

DeleteCriticalSection_(CS) 
CloseHandle_(ChangeEvent)

If you must be alerted of EVERY event (even if the thread is busy processing things), then use SetEvent_() and ResetEvent_() instead of PulseEvent_()...

If you want a more generic function to safely set/get long values... you could impliment it like this:

Code: Select all

Procedure SetValue(*Variable.LONG,NewValue)
  EnterCriticalSection_(CS)
  If NewValue<>*Variable\l
    *Variable\l=NewValue
    PulseEvent_(ChangeEvent);/signal any waiting threads...
  EndIf
  LeaveCriticalSection_(CS)
EndProcedure
Procedure GetValue(*Variable.LONG)
  Protected Result.l
  EnterCriticalSection_(CS)
  Result=*Variable\l
  LeaveCriticalSection_(CS)
  ProcedureReturn Result
EndProcedure

paulr · Post by **paulr** » Wed Jan 19, 2005 10:59 am

Thanks for the input guys!

PolyVector wrote:this is not a very good way of achieving thread-safety... if you need a thread to be notified of some kind of change (like the slider) you should use the "event" commands rather than looping with a criticalsection... The benefit is that you don't waste CPU time and have very quick response...

PolyVector, I actually need the thread to keep looping - it's not wasting CPU time, it's going to be performing calculations in the background... It's no good if the thread stops every cycle and waits for the slider to move - I just want the it to notice when it does, so it can restart its calculations using a different input value. Like for example when you do a blur operation in a graphics program, and the preview instantly responds while you drag the blur amount setting up and down.

Can you see why my program is failing to do this? Or tell me a better way to do it?

Thanks!

PolyVector · Post by **PolyVector** » Wed Jan 19, 2005 8:12 pm

well events can be used that way with slight modification... They're still a good idea...

Code: Select all

Global ChangeEvent.l
Global CS.CRITICAL_SECTION
Global value
Global ThreadID.l
#QuitValue=-1;/Signal that we should quit...

;/Some functions to clean up things:
Procedure SetValue(NewValue)
  EnterCriticalSection_(CS)
  If NewValue<>value
    value=NewValue
    SetEvent_(ChangeEvent);/signal any waiting threads...
  EndIf
  LeaveCriticalSection_(CS)
EndProcedure

Procedure GetValue()
  Protected result.l
  EnterCriticalSection_(CS)
  result=value
  LeaveCriticalSection_(CS)
  ProcedureReturn result
EndProcedure

Procedure TsDebug(String$);//Thread safe debug :)
  EnterCriticalSection_(CS)
  Debug String$
  LeaveCriticalSection_(CS)
EndProcedure

Procedure Thread(Parameter)
  Protected d
  TsDebug("Thread started")
  Repeat
    If WaitForSingleObject_(ChangeEvent,0)=0;/Check if we've been signaled...
      ResetEvent_(ChangeEvent)
      NewValue=GetValue()
      If NewValue=#QuitValue
        Break
      EndIf
      TsDebug("Value changed: "+Str(NewValue))
    Else
      Delay(100)
      ;For b=1 To 50000;/It still doesn't like your cpu intensive loop... hrmmm....
      ;  b+1
      ;  b-1
      ;Next
      TsDebug("...")
    EndIf
  ForEver
  TsDebug("Thread Finished")
EndProcedure


InitializeCriticalSection_(CS)
ChangeEvent=CreateEvent_(#Null,#True,#False,"MyChangeEvent")
ThreadID=CreateThread(@Thread(), 0)

OpenWindow(0, 300, 400, 300, 120, #PB_Window_SystemMenu, "Threads demo")
CreateGadgetList(WindowID())
TrackBarGadget(0,20, 40, 256, 40, 0, 255)

CreateImage(0, 300, 300)

Repeat
  EventID = WaitWindowEvent()
  If EventID=#PB_Event_Gadget And EventGadgetID()=0
    SetValue(GetGadgetState(0))
  EndIf
Until EventID=#PB_EventCloseWindow
SetValue(#QuitValue)
WaitForSingleObject_(ThreadID,1000);/give the thread 1 second to close... I wouldn't recomend using #INFINITE...

DeleteCriticalSection_(CS)
CloseHandle_(ChangeEvent)

paulr · Post by **paulr** » Wed Jan 19, 2005 8:33 pm

Code: Select all

      ;For b=1 To 50000;/It still doesn't like your cpu intensive loop... hrmmm.... 
      ;  b+1 
      ;  b-1 
      ;Next

You got it, that's the problem I'm trying to solve! Uncomment the cpu-intensive bit and it all goes wrong!

Basically our programs do much the same thing now, and both crash for the same reason. Is this a bug in PB? Should I report it? Can anyone help sort this out?

Paul

PolyVector · Post by **PolyVector** » Wed Jan 19, 2005 9:01 pm

I can't quite figure it out... the version i posted should be 100% thread safe... anybody else got an idea before paulr posts it as a bug?

Pupil · Post by **Pupil** » Wed Jan 19, 2005 9:21 pm

PolyVector wrote:I can't quite figure it out... the version i posted should be 100% thread safe... anybody else got an idea before paulr posts it as a bug?

Actually that is not quite true, because your debug procedure isn't thread safe and calling it is definately not thread safe, i.e. this will mess with the string buffer:

Code: Select all

TsDebug("Value changed: "+Str(NewValue))

Also the stub of the procedure does some string handling before anything else inside the procedure is executed.

PolyVector · Post by **PolyVector** » Thu Jan 20, 2005 12:45 am

oh yeah, the pb string thread stuff... i completely forgot about that

... I've been working with c++ lately and have been a bit rusty w/ pb...

paulr · Post by **paulr** » Mon Jan 24, 2005 3:10 pm

Code updated For 5.20+

Okay, so if strings in threads can't be trusted, how about this...

Code: Select all

Global ChangeEvent.l
Global CS.CRITICAL_SECTION
Global value
Global ThreadID.l
#QuitValue=-1;/Signal that we should quit...

;/Some functions to clean up things:
Procedure SetValue(NewValue)
  EnterCriticalSection_(CS)
  If NewValue<>value
    value=NewValue
    SetEvent_(ChangeEvent);/signal any waiting threads...
  EndIf
  LeaveCriticalSection_(CS)
EndProcedure

Procedure GetValue()
  Protected result.l
  EnterCriticalSection_(CS)
  result=value
  LeaveCriticalSection_(CS)
  ProcedureReturn result
EndProcedure

Procedure Thread(Parameter)
  Protected d
  Repeat
    If WaitForSingleObject_(ChangeEvent,0)=0;/Check if we've been signaled...
      ResetEvent_(ChangeEvent)
      NewValue=GetValue()
      If NewValue=#QuitValue
        Break
      EndIf
      EnterCriticalSection_(CS)
      Debug 1
      LeaveCriticalSection_(CS)
    Else
      Delay(100)
      For b=1 To 50000;/cpu intensive bit - causes crashes
        b+1
        b-1
      Next
      EnterCriticalSection_(CS)
      Debug 0
      LeaveCriticalSection_(CS)
    EndIf
  ForEver
EndProcedure


InitializeCriticalSection_(CS)
ChangeEvent=CreateEvent_(#Null,#True,#False,"MyChangeEvent")
ThreadID=CreateThread(@Thread(), 0)

OpenWindow(0, 300, 400, 300, 120, "Threads demo", #PB_Window_SystemMenu)

TrackBarGadget(0,20, 40, 256, 40, 0, 255)

Repeat
  EventID = WaitWindowEvent()
  If EventID=#PB_Event_Gadget And EventGadget()=0
    SetValue(GetGadgetState(0))
  EndIf
Until EventID=#PB_Event_CloseWindow
SetValue(#QuitValue)
WaitForSingleObject_(ThreadID,1000);/give the thread 1 second to close... I wouldn't recomend using #INFINITE...

DeleteCriticalSection_(CS)
CloseHandle_(ChangeEvent)

It's PolyVector's code, modified so that now it doesn't use strings at all. And it still crashes if you move the slider gadget back and forth for a while. It definitely seems to be the intensive processing bit that's causing the problems, but I don't see why - it's just a harmless delay loop, and I thought one of the usual uses for threads was to perform complex number-crunching in the background without slowing the main program down!

Can anyone help solve this or is it one for the bug reports forum?

eriansa · Post by **eriansa** » Thu Jan 27, 2005 11:57 pm

Until change = #False

???

paulr · Post by **paulr** » Fri Jan 28, 2005 12:04 am

In my original post, this keeps the thread repeating until it completes the 'For a = 1 To 50' loop without being interrupted by a change in the value of 'value' and the consequent 'break' command. Clear?