[RESOLVED] Why doesn't this method of closing a worker thread work?

[VBCherry]

I have an MFC application whose GUI has a start and a stop button. The start button calls a function which I want to run in the background, periodically. My solution was to use the SetTimer function to call the OnTimer function at regular intervals. This function then creates a thread using AfxBeginThread and then populates the structure that I pass to the thread as a parameter. All this works just fine.

What I would like to do now is the make the stop button end the thread. I have read about TerminateThread but it seems too risky. Since I want to close the worker thread from another thread, I know I can't use AfxEndThread, but I wanted to know why the following solution doesn't work.

What I have tried:

I created an int variable called count and set it to 0 whenever start is pressed, and set it to 1 whenever stop is pressed.
The structure I am passing into AfxBeginThread looks like this

typedef struct THREADSTRUCT
            {
            MyGUIDlg*    _this;
	       int          flag;
            //you can add here other parameters you might be interested on
           } THREADSTRUCT;

When start is pressed the SetTimer function is called, whose callback function is defined like this:

void MyGUIDlg::OnTimer(UINT_PTR nIDEvent)
{
	THREADSTRUCT* _param = new THREADSTRUCT;
	_param->_this=this;
	_param->flag = count;
	AfxBeginThread(StartThread, _param);	
	CDialog::OnTimer(nIDEvent);
}

In my StartThread function I have added the following:

UINT MyGUIDlg::StartThread(LPVOID param)
{
    THREADSTRUCT *ts = (THREADSTRUCT*)param;
    if((ts->flag)==1)
	{
		//Use AfxEndThread to close this thread
	}
 
	return 0;
}

But this doesn't seem to work, any thoughts why?

[2kaud]

In startThread don't you need a loop that checks flag rather than just 1 test at the beginning?

See http://flounder.com/workerthreads.htm

[VBCherry]

The StartThread function is called periodically by SetTimer. That is why I did not put it in a loop.

[2kaud]

The StartThread function is called periodically by SetTimer. That is why I did not put it in a loop.

with the same param pointer? - every time OnTimer() is called, param is set to point to new allocated memory.

PS Where are you freeing the allocated memory?

[VictorN]

The StartThread function is called periodically by SetTimer. That is why I did not put it in a loop.

No, it is called only once in each thread you start!
And note that you start a new thread each time your OnTimer handler is called...
Note also that you lose access to every new created THREADSTRUCT instance right after your OnTimer returns, so you won't be able to change any value of those THREADSTRUCT instances....

[VBCherry]

@VictorN @2kaud

Thank you for pointing out the mistake I was making. I changed the code so that I declared the THREADSTRUCT instance in the header file, and initialised it and it's members in the OnStartButton function. This was I know that I am passing the same structure every time. I delete the _param pointer in the function associated with the stop button.
I also realised that SetTimer has a KillTimer function to stop it, which I thought would be a easier way of stopping creation of new threads. But this just hangs my GUI for a bit and then resumes whatever the thread was doing. I'm not sure why my KillTimer isn't working.

But thank you for pointing out this mistake!!!

[VBCherry]

@VictorN

I was thinking about why the thread wasn't end when I killed the timer, is it possible that the thread doesn't end after the thread function goes out of scope each time?

[2kaud]

I also realised that SetTimer has a KillTimer function to stop it, which I thought would be a easier way of stopping creation of new threads

Yes. But this has nothing to do with terminating existing threads.

is it possible that the thread doesn't end after the thread function goes out of scope each time?

A thread ends when the code for the thread finishes running - or the thread is abnormally terminated (avoid). The scope of the thread function in this context is irrelevant.

[VBCherry]

Yes. But this has nothing to do with terminating existing threads.

But if it is my timer that creates new threads after the old one have ended (return 0), won't killing that timer stop further threads from being created?
I am terribly sorry if I am making this unnecessarily complicated. It's just that I want what ever the thread does to stop when I press stop, and I assumed killing the timer would do this. But this just causes my GUI to hang.
If it helps, the application is supposed to read from a serial port every 50 milliseconds and display the data on the GUI when the user presses start. This is why I put the read function in the thread (so that the GUI can still be interacted with) and the thread in the timer. I open the port and initialise the' _param' parameter outside the thread. On stop, I kill the timer and close the serial port handle. But this causes the aforementioned hang.

[2kaud]

to read from a serial port every 50 milliseconds and display the data on the GUI when the user presses start

But why start a new thread for every read? Why not have a timer within the thread function itself? Why not start the port thread once when the main program starts - but suspended. When start button clicked, resume the thread and read port every 50 milliseconds. When stop button clicked suspend the thread. When the main program finishes, the thread will automatically be terminated.

[VBCherry]

But why start a new thread for every read? When start button clicked, resume the thread and read port every 50 milliseconds.

Is there a way to automatically resume a suspended thread after a certain amount of time? Also does the suspension and resumption happen from within the thread? This could potentially make my code smaller and more efficient!

EDIT: I found the related notes in the website you linked. Can those functions be called from both outside and inside the thread? And do I use Sleep() between them?
Also AfxBeginThread returns a CWinThread pointer, is this an identification for the thread? So can I store this in a pointer and call SuspendThread(CWinThread* pPointer)?

[2kaud]

But if it is my timer that creates new threads after the old one have ended (return 0), won't killing that timer stop further threads from being created?

How do you know when the old thread has ended? - there's nothing in the posted code that determines this. Killing the timer will stop new threads being created but this doesn't terminate existing threads. Possibly the 'hang' is caused because on stop you close the serial port handle from which existing threads are still trying to read?

[2kaud]

Resuming a suspended thread has to be done outside of the thread - as its suspended! However you can use a timer within a thread to give the approx 50ms port read requirement. As mentioned in post #11, why not create the thread suspended once at the start of the program (and save its return value as this is a pointer to the CWinThread object eg pthrd) and when the user clicks the start button, resume the thread (pthrd->ResumeThread()) and when clicks the stop button suspend the thread (pthrd->SuspendThread()) ?

[VBCherry]

Resuming a suspended thread has to be done outside of the thread - as its suspended! However you can use a timer within a thread to give the approx 50ms port read requirement. As mentioned in post #11, why not create the thread suspended once at the start of the program (and save its return value as this is a pointer to the CWinThread object eg pthrd) and when the user clicks the start button, resume the thread (pthrd->ResumeThread()) and when clicks the stop button suspend the thread (pthrd->SuspendThread()) ?

This is a brilliant idea! I do have one hypothetical to run by you. (I am terribly sorry for taking your time, but it is not often that I have someone experience guiding me).
Can use the timer to callback a function that resumes the thread every 50ms, and have the thread suspend itself each time? I don't know how to use SetTimer inside a thread as I have to define a callback function, and I don't know how to pass this into the thread.

[2kaud]

The easiest way is probably to sleep the thread for 50ms. Consider for the thread function

while (true) {
   //read port
   //process port data
   Sleep(50);
}