Terminating Threads! [modified]
-
I have an MFC MDI app in which I have included a docking control bar (Cristi Posea - CSizingControlBar - a resizable control bar) The controlbar has a dialog associated with it which, amongst other things, continuously grabs data from a PCI A/D card using a worker thread. I have the following code, first initialise the control bar and associated dialog:- in MainFrm.h:-
class CMainFrame : public CMDIFrameWnd
{
DECLARE_DYNAMIC(CMainFrame)
public:
CMainFrame();CMainControlBar m\_MCB; //the control bar CDataControls m\_GrabDataDialog; //the dialog attached to the control bar//other stuff...
.
.
.
}then in MainFrm.cpp:-
int CMainFrame::EnableControlBar()
{if (!m\_MCB.Create(\_T("Data Window"), this, 50)) { TRACE0("Failed to create Data Window\\n"); return -1; } m\_MCB.SetBarStyle(m\_MCB.GetBarStyle() | CBRS\_TOOLTIPS | CBRS\_FLYBY | CBRS\_SIZE\_DYNAMIC ); m\_MCB.EnableDocking(CBRS\_ALIGN\_ANY);#ifdef _SCB_REPLACE_MINIFRAME
m_pFloatingFrameClass = RUNTIME_CLASS(CSCBMiniDockFrameWnd);
#endifDockControlBar(&m\_MCB, AFX\_IDW\_DOCKBAR\_LEFT); //~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ //~~~~ Attach Controls Dialog //~~~~ to Window if possible //~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ if (!m\_DataDialog.Create(IDD\_MAINCONTROLS,&m\_MCB)) { TRACE0("Failed to create instant bar child\\n"); return -1; // fail to create } //show the bar! m\_DataDialog.ShowWindow(SW\_SHOW); return 0;}
Next, set up the thread:-
class CDataControls : public CDialog
{
// Construction
public:
virtual ~CDataControls();
CDataControls(CWnd* pParent = NULL);CGraphDraw m\_Graph; //the custom control void DoDataDraw(); //passes data to the custom control void DoDataGrab(); //grabs data from an A/D converter BOOL m\_TerminateThreads; //flag to terminate the thread CWinThread \*pDataGrabThread; //thread pointer double Sig\[1024\];//other stuff...
.
.
.
};and:-
BOOL CDataControls::OnInitDialog()
{
CDialog::OnInitDialog();//Initialise Custom Control m\_Graph.Initialise(); //Setup Grab Thread pDataGrabThread = AfxBeginThread(DataGrabThread,this,THREAD\_PRIORITY\_NORMAL, 0, CREATE\_SUSPENDED); if(pDataGrabThread==NULL) AfxMessageBox(L"Crap Thread"); pDataGrabThread->m\_bAutoDelete=FALSE; pDataGrabThread->ResumeThread(); return TRUE;}
Now the actual thread:-
i think its not a safe practice to access the m_TerminateThread variable in main UI thread from another thread, without using any locking mechanisms. Better to use an Event object, which is initially non-signalled, set it to signaled in CDataControls::OnClose(), and wait on thread handle. And in Thread function,
//m_hEvent is the Event handle in CDataControls
while(WAIT_OBJECT_0 != WaitForSingleObject(self->m_hEvent,0))
{
self->DoDataGrab(); //go and grab the data
Sleep(100); //don't hog the CPU
}or,
while(WAIT_OBJECT_0 != WaitForSingleObjectself->m_hEvent,100))
{
self->DoDataGrab(); //go and grab the data
} -
i think its not a safe practice to access the m_TerminateThread variable in main UI thread from another thread, without using any locking mechanisms. Better to use an Event object, which is initially non-signalled, set it to signaled in CDataControls::OnClose(), and wait on thread handle. And in Thread function,
//m_hEvent is the Event handle in CDataControls
while(WAIT_OBJECT_0 != WaitForSingleObject(self->m_hEvent,0))
{
self->DoDataGrab(); //go and grab the data
Sleep(100); //don't hog the CPU
}or,
while(WAIT_OBJECT_0 != WaitForSingleObjectself->m_hEvent,100))
{
self->DoDataGrab(); //go and grab the data
}Actually it is safe in this particular case. As the worker thread only ever reads the state of the variable, but he has to make sure it is reading the changes on each loop, i.e. it must be declared [code]mutable[/code] elase a compiler optimisation step may remove the follow on reads. As long as multiple threads are not read/writing it should be fine.
If you vote me down, my score will only get lower
-
I have an MFC MDI app in which I have included a docking control bar (Cristi Posea - CSizingControlBar - a resizable control bar) The controlbar has a dialog associated with it which, amongst other things, continuously grabs data from a PCI A/D card using a worker thread. I have the following code, first initialise the control bar and associated dialog:- in MainFrm.h:-
class CMainFrame : public CMDIFrameWnd
{
DECLARE_DYNAMIC(CMainFrame)
public:
CMainFrame();CMainControlBar m\_MCB; //the control bar CDataControls m\_GrabDataDialog; //the dialog attached to the control bar//other stuff...
.
.
.
}then in MainFrm.cpp:-
int CMainFrame::EnableControlBar()
{if (!m\_MCB.Create(\_T("Data Window"), this, 50)) { TRACE0("Failed to create Data Window\\n"); return -1; } m\_MCB.SetBarStyle(m\_MCB.GetBarStyle() | CBRS\_TOOLTIPS | CBRS\_FLYBY | CBRS\_SIZE\_DYNAMIC ); m\_MCB.EnableDocking(CBRS\_ALIGN\_ANY);#ifdef _SCB_REPLACE_MINIFRAME
m_pFloatingFrameClass = RUNTIME_CLASS(CSCBMiniDockFrameWnd);
#endifDockControlBar(&m\_MCB, AFX\_IDW\_DOCKBAR\_LEFT); //~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ //~~~~ Attach Controls Dialog //~~~~ to Window if possible //~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ if (!m\_DataDialog.Create(IDD\_MAINCONTROLS,&m\_MCB)) { TRACE0("Failed to create instant bar child\\n"); return -1; // fail to create } //show the bar! m\_DataDialog.ShowWindow(SW\_SHOW); return 0;}
Next, set up the thread:-
class CDataControls : public CDialog
{
// Construction
public:
virtual ~CDataControls();
CDataControls(CWnd* pParent = NULL);CGraphDraw m\_Graph; //the custom control void DoDataDraw(); //passes data to the custom control void DoDataGrab(); //grabs data from an A/D converter BOOL m\_TerminateThreads; //flag to terminate the thread CWinThread \*pDataGrabThread; //thread pointer double Sig\[1024\];//other stuff...
.
.
.
};and:-
BOOL CDataControls::OnInitDialog()
{
CDialog::OnInitDialog();//Initialise Custom Control m\_Graph.Initialise(); //Setup Grab Thread pDataGrabThread = AfxBeginThread(DataGrabThread,this,THREAD\_PRIORITY\_NORMAL, 0, CREATE\_SUSPENDED); if(pDataGrabThread==NULL) AfxMessageBox(L"Crap Thread"); pDataGrabThread->m\_bAutoDelete=FALSE; pDataGrabThread->ResumeThread(); return TRUE;}
Now the actual thread:-
Caslen wrote:
::SendMessage(m_ControlsDialog,WM_CLOSE,0,0); //send a message to close the dialog and thread
You've not shown how
m_ControlsDialogwas declared. I assume it's safe to send a message to. If it's a modeless dialog, why not just callDestroyWindow()?Caslen wrote:
while(self->m_TerminateThreads!=0)
This loop is suspect if
m_TerminateThreadsis not declared asvolatile. Otherwise, it may or may not execute, or it may fail to end. Becausem_TerminateThreadsis not being changed within the loop, the compiler would likely optimize it out. A better way of having the secondary thread end is to have the primary thread signal an event."One man's wage rise is another man's price increase." - Harold Wilson
"Fireproof doesn't mean the fire will never come. It means when the fire comes that you will be able to withstand it." - Michael Simmons
"Man who follows car will be exhausted." - Confucius
-
I have an MFC MDI app in which I have included a docking control bar (Cristi Posea - CSizingControlBar - a resizable control bar) The controlbar has a dialog associated with it which, amongst other things, continuously grabs data from a PCI A/D card using a worker thread. I have the following code, first initialise the control bar and associated dialog:- in MainFrm.h:-
class CMainFrame : public CMDIFrameWnd
{
DECLARE_DYNAMIC(CMainFrame)
public:
CMainFrame();CMainControlBar m\_MCB; //the control bar CDataControls m\_GrabDataDialog; //the dialog attached to the control bar//other stuff...
.
.
.
}then in MainFrm.cpp:-
int CMainFrame::EnableControlBar()
{if (!m\_MCB.Create(\_T("Data Window"), this, 50)) { TRACE0("Failed to create Data Window\\n"); return -1; } m\_MCB.SetBarStyle(m\_MCB.GetBarStyle() | CBRS\_TOOLTIPS | CBRS\_FLYBY | CBRS\_SIZE\_DYNAMIC ); m\_MCB.EnableDocking(CBRS\_ALIGN\_ANY);#ifdef _SCB_REPLACE_MINIFRAME
m_pFloatingFrameClass = RUNTIME_CLASS(CSCBMiniDockFrameWnd);
#endifDockControlBar(&m\_MCB, AFX\_IDW\_DOCKBAR\_LEFT); //~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ //~~~~ Attach Controls Dialog //~~~~ to Window if possible //~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ if (!m\_DataDialog.Create(IDD\_MAINCONTROLS,&m\_MCB)) { TRACE0("Failed to create instant bar child\\n"); return -1; // fail to create } //show the bar! m\_DataDialog.ShowWindow(SW\_SHOW); return 0;}
Next, set up the thread:-
class CDataControls : public CDialog
{
// Construction
public:
virtual ~CDataControls();
CDataControls(CWnd* pParent = NULL);CGraphDraw m\_Graph; //the custom control void DoDataDraw(); //passes data to the custom control void DoDataGrab(); //grabs data from an A/D converter BOOL m\_TerminateThreads; //flag to terminate the thread CWinThread \*pDataGrabThread; //thread pointer double Sig\[1024\];//other stuff...
.
.
.
};and:-
BOOL CDataControls::OnInitDialog()
{
CDialog::OnInitDialog();//Initialise Custom Control m\_Graph.Initialise(); //Setup Grab Thread pDataGrabThread = AfxBeginThread(DataGrabThread,this,THREAD\_PRIORITY\_NORMAL, 0, CREATE\_SUSPENDED); if(pDataGrabThread==NULL) AfxMessageBox(L"Crap Thread"); pDataGrabThread->m\_bAutoDelete=FALSE; pDataGrabThread->ResumeThread(); return TRUE;}
Now the actual thread:-
Note that you should declare m_TerminateThreads as mutable volatile as a compiler optimisation could break your code. These lines
while(self->m_TerminateThreads!=0)
{
self->DoDataGrab(); //go and grab the datacould be interpreted as the compiler as:
if (self->m_TerminateThreads!=0)
{
for (;;)
{
self->DoDataGrab();If you vote me down, my score will only get lower
-
i think its not a safe practice to access the m_TerminateThread variable in main UI thread from another thread, without using any locking mechanisms. Better to use an Event object, which is initially non-signalled, set it to signaled in CDataControls::OnClose(), and wait on thread handle. And in Thread function,
//m_hEvent is the Event handle in CDataControls
while(WAIT_OBJECT_0 != WaitForSingleObject(self->m_hEvent,0))
{
self->DoDataGrab(); //go and grab the data
Sleep(100); //don't hog the CPU
}or,
while(WAIT_OBJECT_0 != WaitForSingleObjectself->m_hEvent,100))
{
self->DoDataGrab(); //go and grab the data
}Cool_Dev wrote:
Better to use an Event object,
Thanks, I tried this - it works ok but I still need the WaitForSingleObject() in the CDataControls::OnClose() (see below) Why is this?
void CDataControls::OnClose()
{
SetEvent(g_Event);
WaitForSingleObject(pDataGrabThread>m_hThread,1000); //give it chance to stop delete pDataGrabThread; //then delete itCDialog::OnClose();}
-
Cool_Dev wrote:
Better to use an Event object,
Thanks, I tried this - it works ok but I still need the WaitForSingleObject() in the CDataControls::OnClose() (see below) Why is this?
void CDataControls::OnClose()
{
SetEvent(g_Event);
WaitForSingleObject(pDataGrabThread>m_hThread,1000); //give it chance to stop delete pDataGrabThread; //then delete itCDialog::OnClose();}
because you need to give the thread a chance to see the event and react to it.
-
Caslen wrote:
::SendMessage(m_ControlsDialog,WM_CLOSE,0,0); //send a message to close the dialog and thread
You've not shown how
m_ControlsDialogwas declared. I assume it's safe to send a message to. If it's a modeless dialog, why not just callDestroyWindow()?Caslen wrote:
while(self->m_TerminateThreads!=0)
This loop is suspect if
m_TerminateThreadsis not declared asvolatile. Otherwise, it may or may not execute, or it may fail to end. Becausem_TerminateThreadsis not being changed within the loop, the compiler would likely optimize it out. A better way of having the secondary thread end is to have the primary thread signal an event."One man's wage rise is another man's price increase." - Harold Wilson
"Fireproof doesn't mean the fire will never come. It means when the fire comes that you will be able to withstand it." - Michael Simmons
"Man who follows car will be exhausted." - Confucius
-
because you need to give the thread a chance to see the event and react to it.
-
Actually it is safe in this particular case. As the worker thread only ever reads the state of the variable, but he has to make sure it is reading the changes on each loop, i.e. it must be declared [code]mutable[/code] elase a compiler optimisation step may remove the follow on reads. As long as multiple threads are not read/writing it should be fine.
If you vote me down, my score will only get lower
IMO it needs to be no wider than what can be handled atomically (hence 32-bit) AND declared volatile. Then it does not matter how many readers there are, as long as there is only one writer all is fine. With several writers, it most often needs a lock of some kind. :)
Luc Pattyn [Forum Guidelines] [Why QA sucks] [My Articles] Nil Volentibus Arduum
Please use <PRE> tags for code snippets, they preserve indentation, and improve readability.
-
I already figured out the 'volatile' thing - I was more concerned about why I had to 'SendMessage' from CMainFrame::OnClose() - why wasn't CDataControls::OnClose() automatically closed when the App is closed?
Caslen wrote:
...why wasn't CDataControls::OnClose() automatically closed when the App is closed?
Because it was in the secondary thread, which was not terminating, perhaps? To test this, change your code to:
WaitForSingleObject(pDataGrabThread->m_hThread, INFINITE);
"One man's wage rise is another man's price increase." - Harold Wilson
"Fireproof doesn't mean the fire will never come. It means when the fire comes that you will be able to withstand it." - Michael Simmons
"Man who follows car will be exhausted." - Confucius
-
Caslen wrote:
...why wasn't CDataControls::OnClose() automatically closed when the App is closed?
Because it was in the secondary thread, which was not terminating, perhaps? To test this, change your code to:
WaitForSingleObject(pDataGrabThread->m_hThread, INFINITE);
"One man's wage rise is another man's price increase." - Harold Wilson
"Fireproof doesn't mean the fire will never come. It means when the fire comes that you will be able to withstand it." - Michael Simmons
"Man who follows car will be exhausted." - Confucius
-
Ok, that is probably it - but it's correct to use SendMessage() from CMainframe to close it though?
Caslen wrote:
...but it's correct to use SendMessage() from CMainframe to close it though?
Is the dialog modeless?
"One man's wage rise is another man's price increase." - Harold Wilson
"Fireproof doesn't mean the fire will never come. It means when the fire comes that you will be able to withstand it." - Michael Simmons
"Man who follows car will be exhausted." - Confucius
-
Caslen wrote:
...but it's correct to use SendMessage() from CMainframe to close it though?
Is the dialog modeless?
"One man's wage rise is another man's price increase." - Harold Wilson
"Fireproof doesn't mean the fire will never come. It means when the fire comes that you will be able to withstand it." - Michael Simmons
"Man who follows car will be exhausted." - Confucius
-
Then just call
DestroyWindow()."One man's wage rise is another man's price increase." - Harold Wilson
"Fireproof doesn't mean the fire will never come. It means when the fire comes that you will be able to withstand it." - Michael Simmons
"Man who follows car will be exhausted." - Confucius