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

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

Code:

---

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:

Code:

---

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:

Code:

---

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?

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

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

Quote:

---

Originally Posted by VBCherry

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?

Quote:

---

Originally Posted by VBCherry

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....

@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!!!

@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?

Quote:

---

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.

Quote:

---

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.

Quote:

---

Originally Posted by 2kaud

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.

Quote:

---

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?

Quote:

---

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.

Quote:

---

Originally Posted by 2kaud

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)?

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()) ?

Quote:

---

Originally Posted by 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()) ?

---

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.

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

Code:

---

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

---

Quote:

---

Originally Posted by 2kaud

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

---

This only guarantees a lower limit. So there is a possibility that the process might sleep for longer and miss the next packet of data. Won't my idea work?

If the data is only being received every 50ms, then its probably easier to just wait for the data comm event to occur using WaitCommEvent(). See https://msdn.microsoft.com/en-us/lib...(v=vs.85).aspx

This will wait until a comm event is signalled. The lpEvtMask parameter indicates what comm event is signalled. For data being received then this
would be EV_RXCHAR when all chars available could then be read. The loop then turns into something like

Code:

---

DWORD        dwCommEvent;

while (true) {
    WaitCommEvent(hcomm, &dwCommEvent, NULL);
    if (dwCommEvent & EV_RXCHAR) {
        //Character(s) waiting to be read
        //Read all available chars from comm port and process
    }
}

---

A code example of reading from a comm port, assembling one line of read data and processing it is

Code:

---

//Thread master function
unsigned __stdcall ProcessData(PVOID args)
{
HANDLE        hComm = *(HANDLE*)args;

DWORD        dwCommEvent,
                dwRead;

char        chRead;

string        line;

        for ( ; ; ) {
                if (WaitCommEvent(hComm, &dwCommEvent, NULL) && (dwCommEvent & EV_RXCHAR))
                        do {
                                if (ReadFile(hComm, &chRead, 1, &dwRead, NULL)) {
                                        if (dwRead == 1) {
                                                if (chRead < ' ') {
                                                        if (chRead == 13) {
                                                                Process(hComm, line);
                                                                line = "";
                                                        }
                                                } else
                                                        line += chRead;
                                        }
                                } else {
                                        cout << "error in the ReadFile call\n";
                                        return 8;
                                }
                        } while (dwRead);
                else {
                        cout << "error in waitcommevent\n";
                        return 9;
                }
        }
}

---

Thank you so much! This is way beyond anything you needed to do!
Could you explain what you are doing with chRead in the two if statements? Also what does Process do? I just want to understand this, rather than just lift it.
A link to any resource would do, I would not want to waste your time.

Another good essay of Joe Newcomer about using serial ports: http://flounder.com/serial.htm

Quote:

---

Could you explain what you are doing with chRead in the two if statements?

---

hComm is the handle to the port obtained from CreateFile().

When a comms event occurs then WaitCommEvent() will return. If the event is EV_RXCHAR then at least one char is ready to be read from the port. The do loop reads one char at a time from the port until no chars are read. For the purpose this code is used, the device being read sends data one line at a time terminated by a CR (ASCII code 13). If a char 13 is read then the end of line has been read and the accumulated string is passed to the process function. Chars with a value of less than a <space> in this application are ignored - hence the test for < ' '. If the read char is >= ' ' then it is appended to the string line and the loops repeat. Whether the processing of the read char in this example code in your case is appropriate or not will depend upon the format that the device from which you are reading sends its data.

Quote:

---

Also what does Process do

---

Process() simply processes the completed assembled line of data read from the device in the string line. This is whatever you want to do with the read data.

Cheers!

Quote:

---

Originally Posted by VBCherry

This only guarantees a lower limit. So there is a possibility that the process might sleep for longer and miss the next packet of data.

---

No Windows user timer can be relied upon to be 100% accurate as the OS is not real-time. I agree that having Sleep() in code is not usually a good idea but I thought that you wanted simply to sample the input every 50ms rather than read the data which is received every 50ms. IMO using any windows timing mechanism in this scenario is not recommended as exactly 50ms cannot be guaranteed - and if slightly longer could miss the next packet as you stated. Also the time taken to process the data packet needs to be taken into consideration (which needs to be less than 50ms). The wait time plus the process time needs to be 50ms which is going to be very difficult - if not impossible - to undertake. Therefore as suggested in my posts #17 and #18 and in the article referenced by Victor in post #20 IMO other ways should be investigated that don't rely upon timing.