2

How do we start securely only one WorkerThread?
Exist it a design pattern for that?

Actually I do this way, It seems me a bit overcomplicated.

void CLogView::OnStartStoplogger()
{
    m_bLogPtRun = !m_bLogPtRun;    // (bool)
    if (m_bLogPtRun)
    {
        // First check, if Thread is still running
        if (!m_pLogPointThread) 
        {
            m_pLogPointThread = AfxBeginThread(AddLogPointThread, this);
        }
        else
           return;   // Thread still running
    }
    else
    {   
      if (m_pLogPointThread )
      {
         if (m_pLogPointThread) {
           DWORD dwRET = WaitForSingleObject(m_pLogPointThread->m_hThread, /*INFINITE*/ 5000);
           if (dwRET != WAIT_OBJECT_0)
             MessageBox(_T("Could not finished LogPointThread"));
      }
}
     
Tom Tom
  • 1,127
  • 11
  • 25
  • 5
    A side note: `std::thread` is easier to use that MFC threads api. It's also c++ standard. It has a `join` method which can replace your `WaitForSingleObject`. – wohlstad Jan 13 '23 at 14:25
  • 1
    Also, your `if (!m_pLogPointThread) m_pLogPointThread = ...` check is not safe. Another thread can start the thread between the `if` condition check and the creation of the thread. – Ted Lyngmo Jan 13 '23 at 14:27
  • 3
    you are writing the code - why worry about starting multiple threads? it seems a bit unlikely. – Neil Butterworth Jan 13 '23 at 14:54
  • Chances are that you don't even want to spin up a thread for this. [APC](https://learn.microsoft.com/en-us/windows/win32/sync/asynchronous-procedure-calls) can implement asynchronous operations without the complexities inherent to multithreading. And since Windows' I/O stack is asynchronous all the way, that's a good fit for a logger. – IInspectable Jan 13 '23 at 14:54
  • 1
    Another option is to use [std::async/std::future](https://en.cppreference.com/w/cpp/thread/async), or C++20 [std::jthread](https://en.cppreference.com/w/cpp/thread/jthread) which have better RAII behaviors meaning the destructors of std::future or std::jthread do the synchronization/join for you. Also when you are using threads, learn about [std::mutex](https://en.cppreference.com/w/cpp/thread/mutex)/[std::scoped_lock](https://en.cppreference.com/w/cpp/thread/scoped_lock) they will help you avoid [race-conditions](https://stackoverflow.com/questions/34510/what-is-a-race-condition) – Pepijn Kramer Jan 13 '23 at 15:47
  • Anyway, the pattern you are looking for is the [Singleton](https://en.wikipedia.org/wiki/Singleton_pattern) pattern. Not that I'd understand why you are hellbent on creating a thread for no apparent reason. You can just send off I/O calls asynchronously. No locking, no races, same (or better) performance. Just make sure you never ever go anywhere near `aync`/`future`. They have **horrible** performance at least in MSVC. Windows has far better primitives for asynchronous I/O. – IInspectable Jan 13 '23 at 16:08
  • 2
    Also, `if (m_bLogPtRun) { ... } else { m_bLogPtRun = false; ... }` is kinda bonkers. – IInspectable Jan 13 '23 at 16:18
  • Your "specs" aren't detailed enough. What do you want to achieve exactly, and under what conditions or assumptions? For example, can this (singleton) thread be started from multiple points in your code? Once started, what should happen to the other requests? Wait until the previous ones are finished? What happens when the thread finishes its work, is it exited/destroyed (and then possibly restarted), or awaits another request etc etc. Also, using a (shared) boolean to check if the thread is started is not OK, better use some sync object like a (manual-reset) event or mutex. – Constantine Georgiou Jan 13 '23 at 17:13
  • I start a serial communication to read 1..n RTD devices, Start cmd, wait and read is relative slow (2sec) for a cycle.. So I outsource this in a Worker thread. Then from there I post a UI message to the Main Thread if the result is ready. – Tom Tom Jan 13 '23 at 17:46
  • 1
    So why not simply start only one worker thread? Its function would be a start-wait-read-notifyUI loop, as you describe it. Maybe i'm missing something... – Constantine Georgiou Jan 13 '23 at 18:18
  • Yes, I want start and use only ONE worker thread. Not by mystake two. Ok then it is better do start the worker thread at the very beginning and never end until the program exit? But this will involve an slightly another design to send start-stop command to the thread – Tom Tom Jan 13 '23 at 19:22
  • Again, Windows' I/O is **asynchronous** by default. It strikes me as odd that someone would willingly want to add multithreading when a single thread is *a lot* easier to manage, and likely gets you more performance. And uses up less power. – IInspectable Jan 13 '23 at 20:03
  • @IInspectable APC is still new to me, can you add a simple code example that explains this technique. I have read the proposed MS documentation, but I still don't know how start now. – Tom Tom Jan 13 '23 at 20:56
  • 1
    So you want your thread to not be requesting data all the time, and this be a user-initiated operation? (the "more detailed specs" I was talking about). Then there are two solutions i can think of: A. Have the worker thread running until the program exits and accept requests via the APC mechanism, and B. Create a worker thread for every user request, it will execute the start-wait-read-notifyUI cycle and exit - concurrent execution can be prevented by using a critical section object. – Constantine Georgiou Jan 13 '23 at 21:42
  • 1
    APC is quite simple to implement, basically you create a thread acting as a "server", ie accepting requests, via `QueueUserAPC()` calls. The thread function is a `while(!TerminationRequested()) { SleepEx(INFINITE, TRUE); }` loop. For each request issued (`QueueUserAPC()` call) the `APCFUNC` is called when the thread enters the `SleepEx()` function. The APC function is called in the context of the worker thread. You can even pass a "parameter" to the APC function. – Constantine Georgiou Jan 13 '23 at 21:58
  • Is the "Start/Stop" item a pushbutton- or a check-box- style menu-item (and possibly toolbar button). Looks like the latter is the case to me, ie the checked state means "Keep retrieving and logging RTD", and the unchecked one "No longer retrieve and log RTD". Otherwise wouldn't it be two separate buttons (Start and Stop)? Start would then issue a single RTD retrieval request and Stop rather mean "Kill the stuck thread", no? – Constantine Georgiou Jan 14 '23 at 07:50
  • Yes, "checkbox style" retrieving loggging on/off. The additional RTD is later added. So we have first to see if it is usefull and don't interference the existing system. So I thought , to be sure, I kill the additional thread if not used. – Tom Tom Jan 15 '23 at 22:59

1 Answers1

1

OK, below is an example, assuming you create a worker thread that will be running till the application exits. The user can pause and resume data retrieval and logging. No need to employ APC for this, as noted in the comments, instead synchronization is used to control the worker thread. The synchronization object I'm proposing is the manual-reset Event. It acts as a "synchronization boolean", set and cleared by the main (UI) thread - the worker thread waits on it if it's non-signaled.

1. Variables and Initialization

BOOL bInitOK = FALSE;  // Worker Thread Successfully Initialized
BOOL bChkRTD = FALSE;  // Retrieval & Logging of RTD Enabled
HANDLE m_hEvt = CreateEvent(NULL, TRUE, FALSE, NULL); // Controlling Retrieval & Logging of RTD
CWinThread *m_pLogPointThread = AfxBeginThread(AddLogPointFN, this); // Worker Thread - Created once

Please note that AddLogPointFN here is a AFX_THREADPROC function, not a class. This creates a thread without a message-queue. Also you don't need to define and use any thread-local data (ie data instantiated per thread), as the thread is instantiated only once, ie your data are essentially "global" (although they may be contained into another single-instance class, like the main window or view). Alternatively you could create your thread using the CreateThread() function and a LPTHREAD_START_ROUTINE - this one is _stdcall. The event controls whether the worker thread looks for data; it kind of "duplicates" the bChkRTD variable, which is set by the user - an event is a waitable object, while a boolean variable is not.

2. Thread Procedure

UINT AddLogPointFN(LPVOID lParam)
{
    // CLogView* variable - For convenience
    CLogView* pView = (CLogView*)lParam; // Assuming lParam is CLogView

    // Add one-time, resource-heavy initialization here, eg connections
    .
    .
    bool bInit = InitConnections();

    if (!bInit)
    {
        // Initialization Failed - Tell UI to display an error-message or exit
        pView->PostMessage(USERMESSAGE_ALPNOTIFY, 1); 
        return 1;
    }
    
    // Initialization Successful - Tell UI to enable RTD request menus or buttons
    pView->PostMessage(USERMESSAGE_ALPNOTIFY, 2);

    // Wait if m_hEvt is non-signaled
    while(WaitForSingleObject(pView->m_hEvt,INFINITE)==WAIT_OBJECT_0)
    {
        // Retrieve RTD
        .
        .
        // Data retrieved, tell UI to update
        pView->PostMessage(USERMESSAGE_ALPNOTIFY, 3, (LPARAM)p_Data);
    }
    return 0;
}

RTD retrieval is controlled by waiting on the event object. The event is manual-reset, ie the wait function won't reset it - it is set/reset by the UI thread only. The thread doesn't modify the (global) bInitOK variable directly (this would require synchronized access) instead it notifies the UI thread to set it.

3. Main Thread Items and Routines

// Start/Stop Command and UI Update
void CLogView::OnStartStoplogger()
{
    bChkRTD = !bChkRTD; // Check-box behavior, see also ON_UPDATE_COMMAND_UI
    if (bChkRTD) SetEvent(m_hEvt);
    else ResetEvent(m_hEvt);
}

void CLogView::OnUpdateStartStoplogger(CCmdUI *pCmdUI)
{
    pCmdUI->Enable(bInitOK);    // Enabled if Initialization Successful
    pCmdUI->SetCheck(bChkRTD);  // Checked if RTD Retrieval Enabled
}

// USERMESSAGE_ALPNOTIFY Handler
afx_msg LRESULT CLogView::OnALPNotify(WPARAM wParam, LPARAM lParam)
{
    switch(wParam)
    {
        case 1: // Initialization Failed - Show some message or exit
            .
            .
            break;
        case 2: // Initialization Successful - Enable Start/Stop UI item
            bInitOK = TRUE;
            break;
        case 3: // New RTD Available - Display the data
            // E.g. copy the date returned from the worker thread to some local structure
            CopyMyData(m_pMyData, (MY_DATA*)lParam);
            GlobalFree((HGLOBAL)lParam)
            UpdateData(FALSE);
            break;
    }
}
Constantine Georgiou
  • 2,412
  • 1
  • 13
  • 17