solemnwarning/wxwidgets
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 4

Fix calling wxThread::Delete() after thread exit in wxMSW

Browse source 
Contrary to what an existing unit test claimed to do, it did not test
deleting a joinable thread that had already terminated because it
actually hasn't had the time to terminate yet in the existing test (but
this will be changed soon). And calling Delete() on a thread which
really did already terminate resulted in error messages because it tried
to wait on an already invalid thread handle and ended up by returning
wxTHREAD_KILLED which was wrong (killing a dead thread failed too) and
was different from wxTHREAD_NO_ERROR expected by the test.

Fix this by not closing the thread handle when the thread terminates in
order to allow waiting for it again later, if necessary. This shouldn't
be a big deal because in normal circumstances the thread object will be
destroyed very soon, closing the handle anyhow. But the comment about
"not needing the handle any more" was wrong, as we may need it to get
the thread exit code again in the future.
This commit is contained in:
Vadim Zeitlin 2023-03-13 00:47:23 +01:00
parent 3cbab2d76b
commit 4f8f38a99c
1 changed files with 31 additions and 21 deletions
Download patch file Download diff file Expand all files Collapse all files

52
src/msw/thread.cpp
View file

@ -406,11 +406,6 @@ public:
}
~wxThreadInternal()
{
Free();
}
void Free()
{
if ( m_hThread )
{
@ -684,8 +679,6 @@ wxThreadError wxThreadInternal::Kill()
return wxTHREAD_MISC_ERROR;
}
Free();
return wxTHREAD_NO_ERROR;
}
@ -712,10 +705,11 @@ wxThreadInternal::WaitForTerminate(wxCriticalSection& cs,
// as Delete() (which calls us) is always safe to call we need to consider
// all possible states
{
wxCriticalSectionLocker lock(cs);
wxCriticalSectionLocker lock(cs);
if ( m_state == STATE_NEW )
{
switch ( m_state )
{
case STATE_NEW:
if ( shouldDelete )
{
// WinThreadStart() will see it and terminate immediately, no
@ -732,12 +726,34 @@ wxThreadInternal::WaitForTerminate(wxCriticalSection& cs,
}
//else: shouldResume is correctly set to false here, wait until
// someone else runs the thread and it finishes
}
else // running, paused, cancelled or even exited
{
shouldResume = m_state == STATE_PAUSED;
}
break;
case STATE_RUNNING:
// Nothing special to do, just wait for the thread to exit.
break;
case STATE_PAUSED:
shouldResume = true;
break;
case STATE_CANCELED:
// No need to cancel it again.
shouldDelete = false;
break;
case STATE_EXITED:
// We don't need to wait for the thread to exit if it already did,
// but doing it does no harm neither and it's a rare case not worth
// optimizing for.
//
// Just ensure we don't call OnDelete() again as we may have
// already done it (unfortunately we have no way of knowing if we
// did, but it seems better not to do it at all rather than do it
// twice).
threadToDelete = nullptr;
break;
}
} // release cs
// resume the thread if it is paused
if ( shouldResume )
@ -855,10 +871,6 @@ wxThreadInternal::WaitForTerminate(wxCriticalSection& cs,
if ( pRc )
*pRc = wxUIntToPtr(rc);
// we don't need the thread handle any more in any case
Free();
return rc == THREAD_ERROR_EXIT ? wxTHREAD_MISC_ERROR : wxTHREAD_NO_ERROR;
}
@ -1222,8 +1234,6 @@ void wxThread::Exit(ExitCode status)
{
wxThreadInternal::DoThreadOnExit(this);
m_internal->Free();
if ( IsDetached() )
{
delete this;