-1

I'm having trouble understand an exception thrown (read access violation, this is nullptr) when closing my application. The exception occurs on GetDlgItem(IDC_Button1)->EnableWindow(true); when the CDialogEx::OnCancel(); is called. It appears as though the thread is correctly shut down but the error still persists.

When c_DialogFunctionsThreadRunning = false; is called before the MessageBox then the issue does not occur but this also means the thread is terminated before the prompt is accepted or cancelled.

void CFRP_3D_PrinterDlg::OnBnClickedShutdown()
{
    if (MessageBox(_T("Close program?"), _T("Program"), MB_ICONQUESTION | MB_OKCANCEL | MB_TOPMOST) == IDOK)
    {
        c_DialogFunctionsThreadRunning = false;
        StateMachine.StateEnter(ShutDown);
        CDialogEx::OnCancel();
    }
}

void CFRP_3D_PrinterDlg::DialogFunctionsThread()
{
    c_DialogFunctionsThreadRunning = true;
    CWinThread *CDialogFunctionsThread = AfxBeginThread(DoDialogFunctions, this, THREAD_PRIORITY_NORMAL, 0, 0, nullptr);
    CDialogFunctionsThread->m_bAutoDelete = true;
}

UINT CFRP_3D_PrinterDlg::DoDialogFunctions(LPVOID t)
{
    CFRP_3D_PrinterDlg *DialogFunctions = static_cast<CFRP_3D_PrinterDlg *>(t);
    DialogFunctions->DoDialogFunctions();
    return NULL;
}

void CFRP_3D_PrinterDlg::DoDialogFunctions()
{
    while (c_DialogFunctionsThreadRunning && c_DialogCreated)
    {
        GetDlgItem(IDC_Button1)->EnableWindow(true);
        Sleep(20);
    }
}
YouKnowNothingJohn
  • 23
  • 5
  • 2
    On an unrelated note, the symbol `NULL` is the old C-compatibility symbol for null *pointers*. Your function `DoDialogFunctions` is returning an *integer* value not a pointer. If you want to return zero then return zero (i.e. `0`). – Some programmer dude Oct 23 '18 at 12:02
  • 5
    You should never access UI from a different thread than the one that created it. That way leads to madness. – rodrigo Oct 23 '18 at 12:03
  • 1
    Calling CDialogEx::OnCancel() after the while within DoDialogFunctions might help. Still the message box is created from another thread - as rodrigo denoted, GUI and multi-threading is problematic... – Aconcagua Oct 23 '18 at 12:23
  • You could check if `GetDlgItem(IDC_Button1)` returns `NULL` and call `EnableWindow` only if not. But as mentioned before, calling UI functions from different threads is fishy in first place. – Jabberwocky Oct 23 '18 at 12:37
  • 1
    @jab: That doesn't solve the core issue (accessing UI elements across threads). – IInspectable Oct 23 '18 at 12:40
  • @Someprogrammerdude Thank you for the remark, I added the suggested change. – YouKnowNothingJohn Oct 23 '18 at 13:01
  • @rodrigo Understood, I will see how I can set/change the UI buttons from the same thread. Any clue as to why the error is not present when `c_DialogFunctionsThreadRunning = false;` is called before the `MessageBox`? – YouKnowNothingJohn Oct 23 '18 at 13:03
  • @rodrigo I have looked at the code and cannot see a a way to continuously change the UI without a thread as the UI is not created in a looping thread. Do you have a MFC example somewhere that could help me in the right direction? – YouKnowNothingJohn Oct 23 '18 at 13:38
  • @Aconcagua Same question to you as above. Any input is appreciated! – YouKnowNothingJohn Oct 23 '18 at 13:39
  • @IInspectable Any input from your side how a loop to modify UI elements can be implemented in the same thread? – YouKnowNothingJohn Oct 23 '18 at 13:40
  • You can use a timer easily, that `Sleep(20)` suggests that you do not need real-time updates. You can also use `OnIdle` actions (I don't recall the exact name in MFC). – rodrigo Oct 23 '18 at 13:59
  • 1
    Yes, [using a timer](https://stackoverflow.com/q/7157079/898348) here is probably exactly what you need here. A thread is probably not needed.. – Jabberwocky Oct 23 '18 at 14:15
  • 1
    Another possibility is using a thread that posts `WM_APP` messages to the dialog, this might be cleaner than the solution using a timer, but it's far more complex and probably not worth it. – Jabberwocky Oct 23 '18 at 14:21
  • I modified the code to use a timer instead of a thread and it works quite nicely I think! The error is gone. At the moment I am just considering if there is any benefit to using multiple timers as they have to be handled in the same function anyway, at least in the current implementation, – YouKnowNothingJohn Oct 23 '18 at 15:19
  • @YouKnowNothingJohn just une one timer, using multiple timers only adds needless complexity. – Jabberwocky Oct 23 '18 at 15:34
  • 2
    It is not immediately clear, what issue you are trying to solve by spawning a thread. Without that information it is difficult to provide any recommendations. – IInspectable Oct 24 '18 at 04:55
  • @IInspectable I want to update the GUI in (almost) real time when changes occur. For example, a change of state in the statemachine, a click of a button, movement of a motor, etc. – YouKnowNothingJohn Oct 29 '18 at 10:49
  • Spawning an additional thread will not make things any more responsive. If anything, it will slow down your implementation. Context switches are fairly costly operations, and you still have to synchronize between your threads, so any savings will be lost. Threads are most useful when the tasks they are assigned to is compute-bound, and there is no requirement for synchronization. Neither one is the case for you, as both of your threads spend the majority of their time waiting for events. – IInspectable Oct 29 '18 at 12:23
  • @IInspectable I already rewrote the code to make use of a timer to do the GUI changes, would you agree that is the sensible solution for this goal? – YouKnowNothingJohn Oct 29 '18 at 12:31

1 Answers1

0

This issue is happening because the call CDialogEx::OnCancel() destroy all the UI elements ,then the Thread is trying to access a deleted item, the way to solve this is as follow:

when you create the thread, store the return value in a member variable of the class:

class CFRP_3D_PrinterDlg{
    CWinThread *CDialogFunctionsThread;
    //.....
};
    void CFRP_3D_PrinterDlg::DialogFunctionsThread()
    {
        c_DialogFunctionsThreadRunning = true;
        CDialogFunctionsThread = AfxBeginThread(DoDialogFunctions, this, THREAD_PRIORITY_NORMAL, 0, 0, nullptr);
        CDialogFunctionsThread->m_bAutoDelete = true;
    }

then in the prompt to cancel it wait for the thread to finish before to call OnCancel, in this example is waiting forever but you use your own time.

void CFRP_3D_PrinterDlg::OnBnClickedShutdown()
{
    if (MessageBox(_T("Close program?"), _T("Program"), MB_ICONQUESTION | MB_OKCANCEL | MB_TOPMOST) == IDOK)
    {
        c_DialogFunctionsThreadRunning = false;
        StateMachine.StateEnter(ShutDown);
        ::WaitForSingleObject(CDialogFunctionsThread->m_hThread, INFINITE);
        CDialogEx::OnCancel();
    }
}
Ricardo FuMa
  • 46
  • 1
  • Thank you for your answer! This helps me understand what I did wrong. Could you comment on the difference between `CWinThread *CDialogFunctionsThread = ...` and `CDialogFunctionsThread = ...` in our examples? Also, do you agree that for changing GUI elements, a thread is not a good solution but instead a timer should be used? **EDIT:** ah I already see, you declared the variable in the class so that it is available to use in the `OnBnClickedShutDown()` function? – YouKnowNothingJohn Oct 29 '18 at 10:55
  • Actually I don't think it works as suggested as the auto delete will delete the thread and invalidate the handle? I get an exception thrown when I try `WaitForSingleObject`. – YouKnowNothingJohn Oct 29 '18 at 16:16
  • i just tested and it works, are you sure you are assigning the thread to the correct variable? try this: change the CDialogFunctionsThread->m_bAutoDelete = false; and also after the call to ::WaitForSingleObject(CDialogFunctionsThread->m_hThread, INFINITE); use delete CDialogFunctionsThread;, with this you are releasing the memory manually. if still is not working, maybe you call to create the thread is failing and returning NULL, otherwise, verify your variable assigments. – Ricardo FuMa Oct 30 '18 at 16:15