Re: Is the following code MT-Safe?

Tech Tip: Click here to run a free scan for Windows Errors and optimize PC performance

From: Joseph M. Newcomer (newcomer_at_flounder.com)
Date: 07/02/04 

Date: Thu, 01 Jul 2004 20:48:33 -0400

As far as I can tell, the critical section serves no useful purpose. There should also be
no need to check the thread pointer for NULL in the start handler, since the program
should never be allowed to invoke start if the thread is already running. Likewise, there
should be no need to check for the pointer in the stop routine, since the stop routine
should not be invokable from the GUI if the thread is not running.

All those 100-ms delays are going to be a problem, since if there is actually work to do,
the work is limited to being done 10 times a second. The rule should be "block if you have
nothing to do, and don't block if you do have something to do". While it is not uncommon
to poll for completion by waking up and testing a boolean flag, it is extremely unusual to
force a wait period if there is actually something to do.

Overall, this code simply needs a redesign. Most issues of trying to maintain state are
best handled by using a distributed finite state machine to handle the state transitions,
rather than all this waiting on events, thread handles, etc. The more I do multithreading,
the more I avoid waits that are timed, and I now tend to always avoid anything that waits
on the thread itself, preferring a message-notification mechanism to manage the state
machine for me. It is so much easier to deal with and reason about.
                                        joe

On Thu, 1 Jul 2004 08:00:18 -0700, "Alexander Grigoriev" <alegr@earthlink.net> wrote:

>You don't need _stopEvent, nor _startedEvent, nor _stoppedEvent. Two of
>those can be simple bool flags, and the thread handle can be used instead of
>_stoppedEvent.
>
>class A
>{
>public:
> A() :
> b_Stop(false),
> m_pThread(NULL)
> {}
>
>// got to have a destructor
> ~A()
> {
> stop();
> }
>
> void start()
> {
> CSingleLock lock( &crit, TRUE );
>
> if(NULL == m_pThread)
> {
> b_Stop = false;
>
> m_pThread = AfxBeginThread(ThreadFunc, this, THREAD_PRIORITY_NORMAL, 0,
>CREATE_SUSPENDED );
>
> if (NULL != m_pThread)
> {
> m_pThread->m_bAutoDelete = false;
> m_pThread->ResumeThread();
> }
> }
> }
>
> void stop()
> {
> CSingleLock lock( &crit, TRUE );
>
> if(NULL != m_pThread)
> {
> b_Stop = true;
>
> ::WaitForSingleObject(m_pThread->m_hThread, INFINITE );
> delete m_pThread;
> m_pThread = NULL;
> }
> }
>
> void test()
> {
> CSingleLock lock( &crit, TRUE );
>
> // Save running state and stop
> bool bWasRunning = (NULL != m_pThread);
>
> if( bWasRunning )
> stop();
>
> assert(NULL == m_pThread);
>
> // do work with the thread disabled temporarily
> DoSomeStuff();
>
> // Restore running state
> if( bWasRunning )
> start();
> }
>
>private:
> static UINT ThreadFunc( LPVOID pParam )
> {
> A * a = (A*) pParam;
>
>
> while( ! a->b_Stop)
> {
> // Do work in this thread
> Sleep(100); // replace with ::WaitForSingleObject, if you process
>work items
> }
>
> }
>
>bool volatile b_Stop;
>CWinThread * m_pThread;
>};
>
>int main()
>{
> A a;
> a.start();
> a.test();
>}
>
>
>"Ken Durden" <creepiecrawlies@hotmail.com> wrote in message
>news:18c6b284.0407010504.74f24903@posting.google.com...
>> "Doug Harrison [MVP]" <dsh@mvps.org> wrote in message
>news:<eam6e09aroht5unu32rjgqd5m2hvoggcb1@4ax.com>...
>> >
>>
>> First off all, here's an updated version that addresses the concerns
>> of you and others:
>>
>> class A
>> {
>> public:
>> A() :
>> _bRunning(false),
>> _startedEvent(FALSE,FALSE),
>> _stopEvent(FALSE,FALSE),
>> _stoppedEvent(FALSE,FALSE)
>> {}
>>
>> void start()
>> {
>> CSingleLock lock( &crit, TRUE );
>>
>> if( !_bRunning )
>> {
>> _startedEvent.ResetEvent();
>>
>> AfxBeginThread(ThreadFunc, (LPVOID) this, ... );
>> ::WaitForSingleObject( _startedEvent, INFINITE );
>> }
>> }
>>
>> void stop()
>> {
>> CSingleLock lock( &crit, TRUE );
>>
>> if( _bRunning )
>> {
>> _stoppedEvent.Reset();
>> _stopEvent.Set();
>>
>> ::WaitForSingleObject( _stoppedEvent, INFINITE );
>> }
>> }
>>
>> void test()
>> {
>> CSingleLock lock( &crit, TRUE );
>>
>> // Save running state and stop
>> bool bWasRunning = _bRunning;
>> if( bWasRunning )
>> stop();
>>
>> assert( !_bRunning );
>>
>> // do work with the thread disabled temporarily
>> DoSomeStuff();
>>
>> // Restore running state
>> if( bWasRunning )
>> start();
>> }
>>
>> private:
>> static UINT ThreadFunc( LPVOID pParam )
>> {
>> A * a = (A*) pParam;
>>
>> a->_bRunning = true;
>> a->_startedEvent.Set();
>>
>> while( WAIT_TIMEOUT == ::WaitForSingleObject( a->_stopEvent, 100 )
>> )
>> {
>> // Do work in this thread
>> }
>>
>> a->_bRunning = false;
>> a->_stoppedEvent.Set();
>> }
>>
>> CCriticalSection crit;
>> CEvent _startedEvent;
>> CEvent _stopEvent;
>> CEvent _stoppedEvent;
>> bool _bRunning;
>> };
>>
>> int main()
>> {
>> A a;
>> a.start();
>> a.test();
>> }
>>
>>
>> > What do you hope to accomplish with this? First, you're not protecting
>any
>> > data from concurrent access that I can see. Second, your CSingleLock
>> > statement is completely wrong. It constructs a temporary object that is
>> > immediately destroyed, so it serves no purpose whatsoever. You need to
>> > declare a lock variable, e.g.
>> >
>> > CSingleLock lock( &crit, TRUE );
>>
>> Good point, this was an oversight in posting a trimmed down version of
>> much more complicated real code. The locks in my modified functions
>> should be serving a more important role now. They serve to protect
>> against multiple threads calling into stop(), start(), and / or test
>> and screwing everything up.
>>
>> >
>> > Assuming you fix the CSingleLock statement as previously described, it
>can
>> > serve to synchronize access to _bRunning, provided all MT access to
>> > _bRunning is so protected. But you don't protect it in ThreadFunc below,
>so
>> > it serves no purpose here.
>>
>> Logically, access to _bRunning is protected inside of ThreadFunc,
>> because ThreadFunc can only modify the variable when being started or
>> being stopped, both functions that have a lock and wait for the action
>> to complete.
>>
>> >
>> > >private:
>> > > static UINT ThreadFunc( LPVOID pParam )
>> > > {
>> > > A * a = (A*) pParam;
>> > >
>> > > a->_bRunning = true;
>> > > ::WaitForSingleObject( a->_stopEvent, INFINITE );
>> > > a->_bRunning = false;
>> > >
>> > > a->_stoppedEvent.Set();
>> > > }
>> >
>> > You can probably get rid of _stoppedEvent and simply wait on the thread
>> > handle. See this message for more on this:
>> >
>> >
>http://groups.google.com/groups?selm=m3ep501falcm3ecgdb1tsof6p42f32fvk7%404ax.com
>>
>> OK, I have no problem with that. For this example I don't want to,
>> however. For production code this would be a more robust
>> implementation to handle the case of the ThreadFunc prematurely
>> terminating.
>>
>> >
>> > If all access to _bRunning is protected by a mutex, then volatile serves
>no
>> > purpose other than to slow down your code and possibly force you to cast
>> > volatile away, the latter a major concern when you're talking about
>objects
>> > of class type. If you choose not to synchronize access to the variable,
>you
>> > can sometimes get away with merely declaring it volatile, depending on
>your
>> > machine architecture and the semantics your compiler vendor chooses to
>> > attach to volatile. See the end of this message for links to messages
>which
>> > go into more detail on this.
>>
>> Okay, in the modified version of the code, there is still no critical
>> section inside of ThreadFunc for when bRunning is modified. Are you
>> claiming that I need a separate mutex for all read/writes of
>> _bRunning?
>>
>> Thanks,
>> -ken
>

Joseph M. Newcomer [MVP]
email: newcomer@flounder.com
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm

Relevant Pages

  • Re: Extracting printf(...) from (void) printf(....)
    ... I want to search if (void) is there at the beggining of the line.. ... To cast the return value to type serves no purpose since only pointers ... And to keep lint etc. from doing so in cases ...
    (comp.lang.perl.misc)
  • Re: Allocation of memory for Array of Pointers
    ... calloc, malloc, or reallocis unnecessart and serves only to hide errors. ... and that void * pointer will seamlessly be converted to a pointer to the appropiate type. ...
    (comp.lang.c)