Ending Worker Threads hangs the app
CodeGuru Home VC++ / MFC / C++ .NET / C# Visual Basic VB Forums Developer.com
Page 1 of 2 12 LastLast
Results 1 to 15 of 23

Thread: Ending Worker Threads hangs the app

  1. #1
    Join Date
    Nov 2003
    Location
    Portland, OR
    Posts
    827

    Ending Worker Threads hangs the app

    Hi everyone:


    This is a very annoying bug. Let me explain. I have several worker threads in my app. Each one of them is started something like this:
    Code:
    //Starting of thread
    __try
    {
    	//ENTER CRITICAL SECTION
    	/////////////////////////////////////////////
    	EnterCriticalSection(&csThread1);
    
    	GlobalVars.bStopThread1 = FALSE;
    
    	//Start thread
    	CWinThread* pThread = AfxBeginThread(Thread1, NULL);
    	ASSERT(pThread);
    	ASSERT(pThread->m_hThread);
    	if(pThread)
    		GlobalVars.hThread1 = pThread->m_hThread;
    }
    __finally
    {
    	//LEAVE CRITICAL SECTION
    	/////////////////////////////////////////////
    	LeaveCriticalSection(&csThread1);
    }
    And then thread body would be something of this nature:
    Code:
    //Thread itself
    UINT CMainApp::Thread1(LPVOID pParam)
    {
    	//Runner thread
    
    	...
    	
    	while(!GlobalVars.bStopThread1)
    	{
    		//Do thread work
    	}
    
    	return 0;
    }
    The problem occurs very irregularly when I attempt to stop all threads, say from OnClose() when a user hits Close button and my app has to close. I call the following code:
    Code:
    //End thread
    __try
    {
    	//ENTER CRITICAL SECTION
    	/////////////////////////////////////////////
    	EnterCriticalSection(&csThread1);
    
    	if(GlobalVars.hThread1)
    	{
    		//Signal flag to stop immediately
    		GlobalVars.bStopThread1 = TRUE;
    
    		//Wait for thread to end
    		::WaitForSingleObject(GlobalVars.hThread1, INFINITE);
    
    		GlobalVars.hThread1 = NULL;
    	}
    }
    __finally
    {
    	//LEAVE CRITICAL SECTION
    	/////////////////////////////////////////////
    	LeaveCriticalSection(&csThread1);
    }
    The code above woks in about 95% of times, but in 5% it simply hangs the app. I looked all over the MS documentation and I can't see what am I doing wrong. Do you see it?


    Thanks in advance!

  2. #2
    Join Date
    Sep 2002
    Location
    Singapore
    Posts
    673

    Re: Ending Worker Threads hangs the app

    I believe it doesn't hanged, it is just stopped at WaitForSingleObject because the wait is infinite. You can debug and step though the code to see if it is the case.

    How come there is no EnterCriticalSection()/LeaveCriticalSection() whenever you access GlobalVars.bStopThread1 in the thread?

    Usually I use event to stop my threads.

    Alternately, you can try to make bStopThread1 a memory barrier to see if the problem persists. Note: works in Visual Studio 2005 and above only.

    Code:
    volatile BOOL bStopThread1;

  3. #3
    Ejaz's Avatar
    Ejaz is offline Elite Member Power Poster
    Join Date
    Jul 2002
    Location
    Lahore, Pakistan
    Posts
    4,211

    Re: Ending Worker Threads hangs the app

    I agree with CBasicNet, its not very uncommon to have a deadlock in a multi threaded environment. For terminating a thread, take a look at Threads: How to end a thread?.

    Furthermore, for synchronization I would suggest to use some automatic resource handling mechanism, like

    Code:
    class CCSWrapper 
    {
    public:
    	CCSWrapper( ) { ::InitializeCriticalSection( &m_cs ); }
    	~CCSWrapper() { ::DeleteCriticalSection( &m_cs ); }
    
    	
    	void Enter() { ::EnterCriticalSection( &m_cs ); }
    	void Leave() { ::LeaveCriticalSection( &m_cs ); }
    
    private:
    	CRITICAL_SECTION	m_cs;
    };
    Code:
    template <class T>
    class CLockMgr  
    {
    public:
    	CLockMgr( T& lockObject, bool bEnabled = true ) 
    		: m_rLockObject( lockObject ) , m_bEnabled( bEnabled )
    	{
    		if ( m_bEnabled )
    			m_rLockObject.Enter();
    	}
    
    	virtual ~CLockMgr()
    	{
    		if ( m_bEnabled )
    			m_rLockObject.Leave();
    	}
    private:
    	T&   m_rLockObject;
    	bool m_bEnabled;
    };

    Code:
    CLockMgr< CCSWrapper > guard( m_Lock );
    It can save you from a lot of tedious debugging exercise.

  4. #4
    Join Date
    Nov 2002
    Location
    California
    Posts
    4,553

    Re: Ending Worker Threads hangs the app

    Usually, deadlock like you see is caused by interaction that is needed between the worker thread(s) and the main thread, but which is blocked because of the WFSO in the main thread.

    And again, the usual interaction is that the worker thread is using SendMessage() somewhere. The scenario is this: the worker has just issued a SendMessage() to a window created by the main thread. The main thread can't process it, however, because of the WFSO. So both threads just sit there, waiting for each other to respond.

    If this is the cause of your deadlock, then you must show us the "//Do thread work" part of the threads. The problem is inside that.

    Mike

  5. #5
    Join Date
    Nov 2003
    Location
    Portland, OR
    Posts
    827

    Re: Ending Worker Threads hangs the app

    First of all, thank you guys for your input.

    CBasicNet, I'll look into inserting Critical section where you pointed out.

    Ejaz, I did look at the link you gave me and I changed my code to use events, but still the problem stayed.

    MikeAThon, you seem to have hit the nail on the head. The deadlock probably happens because of the internal SendMessage communication between worker threads and main window. (I need SendMessage to update the main window upon the progress of the thread.) Since I cannot predict in the worker thread when the following block is called in the main thread
    Code:
    ::WaitForSingleObject(GlobalVars.hThread1, INFINITE);
    Is there a way to wait while still processing messages in the message loop for the main thread?

  6. #6
    Join Date
    Aug 1999
    Location
    <Classified>
    Posts
    6,882

    Re: Ending Worker Threads hangs the app

    You can call WaitForSingle.. for a short duration of time(instead of Infinite) and if the thread is not done yet then Post yourself a Message (on which you can wait again) and exit that function, that way main window will still keep processing messages.
    Regards,
    Ramkrishna Pawar

  7. #7
    Join Date
    Sep 2002
    Location
    Singapore
    Posts
    673

    Re: Ending Worker Threads hangs the app

    In that case, use PostMessage() instead of SendMessage().

    Is there a way to wait while still processing messages in the message loop for the main thread?
    Call ::WaitForSingleObject(GlobalVars.hThread1, 50); with 50 millisecond timeout in a timer handler, KillTimer() when GlobalVars.hThread1 is signalled.

  8. #8
    Join Date
    Nov 2003
    Location
    Portland, OR
    Posts
    827

    Re: Ending Worker Threads hangs the app

    Quote Originally Posted by Krishnaa
    You can call WaitForSingle.. for a short duration of time(instead of Infinite) and if the thread is not done yet then Post yourself a Message (on which you can wait again) and exit that function, that way main window will still keep processing messages.
    I'm not sure that I understand that, Krishnaa. You see I call this WaitForSingle.. API from a destructor or when the main window is not available. How do I post a message to myself then?

    Quote Originally Posted by CBasicNet
    In that case, use PostMessage() instead of SendMessage().
    I can't, it'd be too complicated to pass strings, data arrays, etc. - will involve much more synchronization.

    I was looking to something like this:
    Code:
    for(;;)
    {
        if(::WaitForSingleObject(GlobalVars.hWaitEvent, 50) == WAIT_OBJECT_0)
            break;
    
        //Do MFC message processing, but how?
    }
    I can implement a simple PeekMessage loop here, but will it be OK with MFC code?

  9. #9
    Join Date
    Aug 1999
    Location
    <Classified>
    Posts
    6,882

    Re: Ending Worker Threads hangs the app

    Well, there you need a change in logic of ending the program, instead of waiting in destructor you should wait in Message handler (the one which is responsible for waiting for short time), let the destructor continue but still hold the application exit on the flag of threads closing.
    Regards,
    Ramkrishna Pawar

  10. #10
    Join Date
    Nov 2003
    Location
    Portland, OR
    Posts
    827

    Re: Ending Worker Threads hangs the app

    Quote Originally Posted by Krishnaa
    Well, there you need a change in logic of ending the program...
    Well, thanks, I understand that. It'd be nice to find a simple solution that doesn't involve such drastic measures as "changing logic of my program".

    After some searching I assembled the following function instead of WaitForSingleObject(). This seems to be working and so far I haven't had a single hang-up. What annoys me the most is that I don't know that for sure, since this deadlock doesn't happen all the time. Can someone tell me if the code below makes sense?
    Code:
    void WaitForEventAndProcessMsgs(HANDLE hEvent)
    {
    	//Do not return until the event in 'hEvent' is signaled
    	//Process messages while waiting
    	ASSERT(hEvent);
    	if(hEvent)
    		for(;;)
    		{
    			//Wait for input or our event to be signaled
    			MsgWaitForMultipleObjectsEx(1, &hEvent, INFINITE, QS_ALLINPUT, 0);
    
    			//Check our event
    			if(::WaitForSingleObject(hEvent, 0) == WAIT_OBJECT_0)
    				break;
    			else
    			{
    				//Process messages
    				MSG msg;
    				while(PeekMessage(&msg, NULL, 0, 0, PM_REMOVE))
    				{
    					if(msg.message == WM_QUIT)
    					{
    						PostQuitMessage((int)msg.wParam);
    						return;
    					}
    
    					#define MSGF_SLEEPMSG 0x5300
    					if(!CallMsgFilter(&msg, MSGF_SLEEPMSG))
    					{
    						TranslateMessage(&msg);
    						DispatchMessage(&msg);
    					}
    				}
    			}
    		}
    }

  11. #11
    Join Date
    Sep 2002
    Location
    Singapore
    Posts
    673

    Re: Ending Worker Threads hangs the app

    Quote Originally Posted by dc_2000
    I can't, it'd be too complicated to pass strings, data arrays, etc. - will involve much more synchronization.
    Nope. Look at the example below.

    Code:
    std::wstring* pStr = new std::wstring();
    *pStr = L"Hello";
    
    PostMessage(WM_MYMSG, (WPARAM)pStr, (LPARAM)0 );
    
    /////
    LRESULT MyDlg::MyMsg(WPARAM wParam, LPARAM, lParam )
    {
    	std::wstring* pStr = (std::wstring*)wParam;
    	if( pStr == NULL )	return 0;
    	
    	// use pStr;
    	
    	delete pStr; // delete before return;
    	return 0;
    }
    See no synchronization!

  12. #12
    Join Date
    Nov 2002
    Location
    California
    Posts
    4,553

    Re: Ending Worker Threads hangs the app

    I agree with CBasicNet. This is standard practice. "new" the string (or whatever) from the heap, and pass a pointer to it in one of the PostMessage() parameters. Then, in the main thread's handler for the message, cast the parameter back to a pointer to the string (or whatever), and after you're done with it, "delete" the pointer.

    All the synchronization issues go away, and you threads will probably execute more quickly too. And then there's no deadlock.

    Mike

  13. #13
    Arjay's Avatar
    Arjay is offline Moderator / MS MVP Power Poster
    Join Date
    Aug 2004
    Posts
    11,219

    Re: Ending Worker Threads hangs the app

    As others have said, synchronization of data doesn't have to be difficult.

    My preference is to encapsulate the shared data within a class, create threadsafe accessors to the data and pass the class pointer to the threads.

    Using this approach, any allocated memory is cleaned up within the same class and user defined messages are used only to signal the UI that data has changed.

    I've modified my Start/Stop sample to add a std::queue that is shared between threads. The sample has a UI thread and two worker threads which sit in a loop. Each time through the loop they push an item onto the shared queue and post a message to the UI. The UI receives the message, updates the corresponding progress control (one for each thread), retrieves the item from the queue and inserts it into a listview control.

    You can start, pause, resume, and stop the operation at any point.

    The following is the OnProgressInc msg handler that gets called when one of the threads needs to increment the progress control. This msg also indicates a item is available in the shared queue and calls the method that inserts the item into the list control.

    Code:
    //+------------------------------------------------------
    // OnIncProgress handler. Receives user defined message 
    // posted from the secondary thread. When we receive this
    // message we increment the progress bar and add the item
    // to the list control.
    //+------------------------------------------------------
    LRESULT CStartStopDlg::OnIncProgress( WPARAM wParam, LPARAM lParam )
    {
    	UINT uID = (UINT) lParam;
    
    	if( TS_START != m_ThreadState ) return 0;
    
    	// Increment the progress controls
    	switch( uID )
    	{
    	case 1:
    		m_ctrlProgress1.StepIt( );
    		break;
    	case 2:
    		m_ctrlProgress2.StepIt( );
    		break;
    	}
    
    	// Get the item out of the queue and put it in the list control
    	InsertItemIntoListView( );
    
    	return 1;
    }
    This method retrieves a queue item and inserts it into the list control. You'll notice that although the this operates in a thread safe manner, any actual synchronization chores are hidden from the caller.

    In this case there are two different threads 'feeding' the queue, but the UI doesn't need to know this, it could be one thread or a 100, with encapsulation, the UI thread need not be concerned.

    Code:
    //+------------------------------------------------------
    // Called when a notification message has been received from
    // one of the secondary threads. Removes one or more items
    // from the shared queue in a thread safe manner and inserts
    // the item into the list control.
    //+------------------------------------------------------
    void CStartStopDlg::InsertItemIntoListView( )
    {
    	CItem* pItem = NULL;
    
    	CString sCount;
    	CString sThreadID;
    	CString sThreadCount;
    
    	// Retrieve the next queue item (thread-safe method)
    	while( NULL != ( pItem = m_ProgressMgr.GetNextItem( ) ) )
    	{
    		// Format the listview column strings
    		sCount.Format( _T( "%d" ), m_uListItemCount );
    		sThreadID.Format( _T( "This is an item from thread: %d" ), pItem->GetID( ) );
    		sThreadCount.Format( _T( "%d" ), pItem->GetCount( ) );
    
    		// Insert the item into the list view and ensure the item is visible
    		m_ctrlList.InsertItem( m_uListItemCount, sCount );
    		m_ctrlList.SetItemText( m_uListItemCount, 1, sThreadID );
    		m_ctrlList.SetItemText( m_uListItemCount, 2, sThreadCount );
    		m_ctrlList.EnsureVisible( m_uListItemCount, TRUE );
    
    		// Remove this item from the queue (thread-safe method)
    		m_ProgressMgr.PopItem( );
    
    		m_uListItemCount++;
        }
    }
    NOTE: The above technique will work only for a single thread servicing the the queue. Thanks to MikeAThon for pointing this out. To work for multiple queue service threads, you can modify the GetNextItem to use front and pop and rather than call m_ProgressMgr.PopItem( ), call delete on the pointer.
    Attached Images Attached Images  
    Attached Files Attached Files
    Last edited by Arjay; May 29th, 2008 at 11:22 AM.

  14. #14
    Join Date
    Nov 2003
    Location
    Portland, OR
    Posts
    827

    Re: Ending Worker Threads hangs the app

    Quote Originally Posted by MikeAThon
    I agree with CBasicNet. This is standard practice. "new" the string (or whatever) from the heap, and pass a pointer to it in one of the PostMessage() parameters. Then, in the main thread's handler for the message, cast the parameter back to a pointer to the string (or whatever), and after you're done with it, "delete" the pointer.
    Thanks, Mike, for this solution. I agree that would work in a simple case, like string or data array.

    Quote Originally Posted by Arjay
    I've modified my Start/Stop sample to add a std::queue that is shared between threads.
    Arjay, thanks for the very good sample. I really like your approach of using the std::queue container to pass data between threads using PostMessage technique. Also the way how you put critical section handling into a class to minimize "headache" of missing the closing API.

    The bottom line, this thread should be included in that "Threads: How to end a thread?" sample someone gave me above. Are there any moderators here who can do that? If I found it earlier it would have saved so much time and re-coding I have to do now.

    Thank you all again!

  15. #15
    Arjay's Avatar
    Arjay is offline Moderator / MS MVP Power Poster
    Join Date
    Aug 2004
    Posts
    11,219

    Re: Ending Worker Threads hangs the app

    A couple of final comments with regard to the sample code posted above.

    The code below is the encapsulated thread-safe queue access code.

    Code:
    // Gets the next item from the ItemQueue
    CItem* GetNextItem( )
    {
      CAutoLockT< CItemQueue > lock(&m_ItemQueue);
        
      if( TRUE != m_ItemQueue.empty( ) )
      {
        return m_ItemQueue.front( );
      }
    
      return NULL;
    }
    
    // Removes the front item from the ItemQueue
    void PopItem( )
    {
      CAutoLockT< CItemQueue > lock( &m_ItemQueue );
    	
      if( TRUE != m_ItemQueue.empty( ) )
      {
        delete m_ItemQueue.front( );
        m_ItemQueue.pop( );
      }
    }
    You'll notice that the code above doesn't use EnterCriticalSection and LeaveCriticalSection. This is because the critical section synchronization code is wrapped in a couple of lightweight classes (using the same technique that Ejaz posted in the beginning of this thread). These type of RAII classes lock during construction and unlock in destruction - the CAutoLockT class is one such class and locks/unlocks the CItemQueue class.

    If you want as trouble free multithreading code, I would encourage anyone to look at taking this RAII approach to synchronization and employing encapsulation as demonstrated earlier. These two things make debugging multithreaded code a whole lot easier.

    Finally I wanted to mention a bit about the thread-safe queue. When I need a threadsafe queue, I never will use the queue as a base class and derive further from the queue. Therefore, I can do the following without any issues.

    Code:
    // CItemQueue -- STL queue with thread lockable attributes
    class CItemQueue : public std::queue< CItem* >, public CLockableCS  
    {
    public:
    	CItemQueue() {};
    	~CItemQueue() {};
    };
    If you are not comfortable with this (multiple inheritence, IS-A, don't derive from stl containers, etc.), you can simply define a CLockableCS object in the same class where your std::queue is defined and lock the CLockableCS object. Both techniques work fine. I've never had any trouble with my approach (of course I never use the CItemQueue as a base class).

    The last thing I wanted to mention is that when sharing a collection between threads, consider the type of collection and what the collection contains.

    For the type of collection, follow the usual rules for choosing between a queue, list, array, etc. As far as what it contains, I find that more often than not, the containers that I share between threads will contain pointers to items rather than the items themselves.

    Why is this? One of the fundamental tenants of good multi-threaded coding is to lock only the necessary code, lock at the last possible point, and release as soon as possible. Having the queue contain the pointer to the object helps with this.

    In this case, we want to lock the queue at the last moment, perform the queue operation (insert, access or remove), and then unlock the queue. If we just stored the item directly, inserting an item into the queue would invoke the item's copy constructor so we would need to be locked during the object's construction phase and for large object's that might be considerable. Furthermore, stl containers move the contained objects around in memory (they can reallocate memory copy objects, etc.), so when this occurs, we want the container to be storing the smallest objects possible - that's why I use pointers.

    Lastly by using a pointer, we can split the use of the object and removal of the object into two operations. Remember, we need to lock the queue when we insert and remove items in the queue, when don't necessarily need to have the queue locked while working with the object.

    We lock the queue a total of 3 times: once to insert, once to access, and once to remove the item. That may seem excessive, but it ensures that the queue is locked and released quickly.

    Remember, synchronization dictates that only one thread can access a shared resource at a time and by using the 3-lock approach, one thread can be inserting an item while another thread is using the item it has already accessed. The more threads that access the shared resource, the more important it is to keep lock times to a minimum.

Page 1 of 2 12 LastLast

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •  


Azure Activities Information Page

Windows Mobile Development Center


Click Here to Expand Forum to Full Width

This is a CodeGuru survey question.


Featured


HTML5 Development Center