Re: PostThreadMessage just gets last message
On 13 Mai, 20:30, Joseph M. Newcomer <newco...@flounder.com> wrote:
See below...
On Tue, 13 May 2008 07:37:43 -0700 (PDT), ".rhavin grobert" <cl...@yahoo.de> wrote:
Hi there.. i have a strange problem with PostThreadMessage (a
feature? ;-) and perhaps someone can jump in....
I wrote a threading-helper that *should* give an object of a derived
class a worker-thread. That thread is started in the ctor (works) and
killed in the dtor (works, too). The delete in method ThreadPost()
marked with "* MARK *" is not executed, so it seems that
PostThreadMessage works. Unfortunately, when i consecutively call
ThreadPost(), only the last message arrives in _ThreadRun().
Perhaps someone can tell me whats going wrong here?
TIA, .rhavin;)
________________________________________________________
// header file
struct SQTrdMsg {
QUAD qCommand;
QUAD qParam;
QUAD qAux;
void* pBody;
BYTE bFlags;
};
class CQThreader {
public:
CQThreader();
virtual ~CQThreader();
bool ThreadPost(QUAD qCmd, QUAD qParam = 0, QUAD qAux = 0,
PCVOID pBody = NULL, BYTE bFlags = QTMF_DELETEBODY);
bool ThreadPost(SQTrdMsg const* pTrdMsg);
virtual inline bool ThreadOnReceived(SQTrdMsg* pMsg) {return false;};
inline bool ThreadIsRunning() const {return m_fTreadRunning;};
inline void _ThreadRunning(bool fRunning) {m_fTreadRunning =
fRunning;};
static CWinThread* _ThreadStart(void* pThis);
static bool _ThreadStop(CWinThread* pThread);
static UINT _ThreadRun(void* pThis);
private:
CWinThread* m_pThread;
bool m_fTreadRunning;
};
________________________________________________________
// implementation file
//-----------------------------------------------------------------------------
CQThreader::CQThreader() {
_ThreadRunning(false);
m_pThread = _ThreadStart(this);
while (!ThreadIsRunning()) {
Sleep(0);
****
This code is deeply wrong and must be removed. What it does is cause the constructor to
enter an infinite loop until the thread is running, which is just wrong. If the thread
fails, this locks up the caller. You are clearly trying to work in the failed paradigm
that concurrent actions should be executed sequentially. If you care about when the
thread starts, you will have the thread PostMessage a notification to you that it has
started. Code like this should never exist.
i changed it to
___________________________
CQThreader::CQThreader() {
_ThreadRunning(false);
m_pThread = _ThreadStart(this);
}
___________________________
//-----------------------------------------------------------------------------
CQThreader::~CQThreader() {
if (_ThreadStop(m_pThread)) {
while (ThreadIsRunning()) {
Sleep(0);
****
See previous note about Sleep(0). This is completely wrong, and must be treated as
completely wrong. NEVER sprinkle Sleep(0) calls (or Sleep calls with ANY value) around
like pixie dust in the hopes that your failed design will magically begin working. This
is fundamentally flawed design and must be fixed
****> }
}
}
ACK, ill look for a better way to do it...
//-----------------------------------------------------------------------------
bool CQThreader::_ThreadStop(CWinThread* pThread) {
if (pThread == NULL)
return false;
pThread->PostThreadMessage(WM_QUIT, 0, 0);
return true;
****
You are not creating a UI thread, so this code is nonsense. There is no message pump to
handle this request. Delete this code. You would not terminate a worker thread this way.
As to what way you *would* terminate it, that will depend on what the thread is doing and
how it knows to ask if it should stop, but you can safely assume this code is erroneous.
****>}
this class doesnt know if it is a GUI-Thread. However, a WM_QUIT does
also get thru a non-gui-thread's message loop, so i dont see the point
in not using it...?
//-----------------------------------------------------------------------------
bool CQThreader::ThreadPost(QUAD qCmd, QUAD qParam, QUAD qAux, PCVOID
pBody,
BYTE bFlags)
{
SQTrdMsg QTMsg;
QTMsg.qCommand = qCmd;
QTMsg.qParam = qParam;
QTMsg.qAux = qAux;
QTMsg.pBody = const_cast<void*>(pBody);
QTMsg.bFlags = bFlags;
bool bRet = ThreadPost(&QTMsg);
****
Well, this code is completely and irremediably incorrect. It never worked right, and as
written can never be made to work right.
NO, joseph, this code IS correct;-) after returning from
ThreadPost(&QTMsg), the object QTMsg can be safely deleted because its
content is already copied. (see below)
//-----------------------------------------------------------------------------
bool CQThreader::ThreadPost(SQTrdMsg const* pTrdMsg) {
if (pTrdMsg == NULL)
return false;
SQTrdMsg* pM = new SQTrdMsg;
if (pM == NULL)
return false;
*pM = *pTrdMsg;
ThreadOnReceived(pM);
****
This would be called by your worker thread. Why are you calling it from the main thread?
this _IS_ to be called from the main thread. The ThreadPost() function
with 5 Parameters is just an alias for the single-parameter-fn. thats
why it can safely delete its object after calling the single-param-fn,
where a new heap-obj is allocated and its content copied from the
"SQTrdMsg const* pTrdMsg"...
****> delete pM;
****
Why do you make a copy here when you should have created the copy at the call site?
This IS the call site.
What is ThreadOnReceived() doing? I don't see how this code could make sense when called
from the posting thread!
****> return true;
if (m_pThread->PostThreadMessage(QT_QTRDMSG, 0, (LPARAM) pM) !=
FALSE)
return true;
****
How can the if-test above ever be executed when it is preceded by a return true?
****> delete pM; // <-- * MARK *
sorry, i already posted that i left some debuglines in the post. the
actual version of this fn is:
____________________________________
bool CQThreader::ThreadPost(SQTrdMsg const* pTrdMsg) {
if (pTrdMsg == NULL)
return false;
SQTrdMsg* pM = new SQTrdMsg;
if (pM == NULL)
return false;
*pM = *pTrdMsg;
if (m_pThread->PostThreadMessage(QT_QTRDMSG, 0, (LPARAM) pM) !=
FALSE)
return true;
delete pM;
return false;
}
____________________________________
//-----------------------------------------------------------------------------
// static
UINT CQThreader::_ThreadRun(void* pThis) {
// note: GetMessage returns -1 on error, 0 on WM_QUIT
// or on no message available, and 1 on all others
MSG msg;
int iRet;
SQTrdMsg* pFzMsg;
if (pThis == NULL)
return -1;
CQThreader* pThreader = (CQThreader*) pThis;
pThreader->_ThreadRunning(true);
while (true) {
iRet = GetMessage(&msg, (HWND) 0, 0, 0);
****
Why are you writing a message pump by hand in a worker thread? Use a UI thread, and this
ancient Windows 1.0 crap will go away! You have been spending too much time reading
Petzold. This is MFC. Code it as MFC, not as Windows 1.0 16-bit C code.
****> switch (iRet) {
case -1:
//error
break;
case 1:
if (msg.message == QT_QTRDMSG) {
if (msg.lParam == NULL)
break;
pFzMsg = (SQTrdMsg*) msg.lParam;
pThreader->ThreadOnReceived(pFzMsg);
delete pFzMsg;
} else {
::TranslateMessage(&msg);
::DispatchMessage(&msg);
***
Dispatch to what window? Why are you doing any of this?
Because it may be a GUI-thread?
***> }
break;
case 0:
if (msg.message == WM_QUIT) {
pThreader->_ThreadRunning(false);
return 0;
}
}
Sleep(0);
****
ALWAYS AND FOREVER, ASSUME THAT TOSSING Sleep(0) CALLS AROUND LIKE THIS MEANS "MY DESIGN
IS TERMINALLY DEFECTIVE!!!!
Yes, i understood;-)