------------------------------------------------------------------------------------
------------------------------------------------------------------------------------
before the StartPooler function. Sorry for that it is because I minimized
"Joseph M. Newcomer" wrote:
See below...
On Fri, 1 May 2009 16:31:01 -0700, Erakis <Erakis@discussions.microsoft.com> wrote:
Hi,
Here is a simple pooling class implemented into a MFC DLL project.
------------------------------------------------------------------------
BOOL CPooler::StartPooling()
{
// Create startup event
m_hStartupEvent = CreateEvent(NULL, TRUE, FALSE, NULL);
// Create main thread
m_hMainThread = CreateThread( NULL, 0, MainThreadProc,
static_cast<LPVOID>( this ), 0, &g_dwThreadID);
****
This is VERY, VERY, VERY BAD!!!! You must NOT call CreateThread! This is MFC; the ONLY
correct way to create a thread is AfxBeginThread!
It is also completely tasteless to keep the thread ID in a global variable. What is going
to happen if you need two threads? You shouldn't even need the thread ID, let alone keep
it in a global variable! In a sensible universe, you would have a struct/class which had
a thread handle member and a thread ID member, and...whoops, I just reinvented CWinThread,
which is what you should be using!
****
// Check for startup event
switch( WaitForSingleObject( m_hStartupEvent, 1000) )
****
If you use AfxBeginThread, you must create it suspended, set the m_bAutoDelete flag to
FALSE, then call CWinThread::ResumeThread. Otherwise, you will not know if the handle is
valid at the point where you do the WFSO.
****
{
case WAIT_TIMEOUT :
bRet = FALSE; // Thread time-out
****
The timeout is a silly value, and is apparently a random number. There is no reason to
expect that 1000ms is going to be a sensible value, and consequently there are conditions
under which the thread can start but fail to set the event within 1s, and you are screwed.
****
break;
case WAIT_OBJECT_0 :
bRet = TRUE; // Thread sucessfully started
break;
}
// Close startup handle
CloseHandle(m_hStartupEvent);
*****
What about the thread handle? Just because you timed out doesn't mean the thread is
invalid. The thread handle could be valid, but what are you going to do with it if you
return FALSE?
*****
m_hStartupEvent = NULL;
return bRet;
}
BOOL CPooler::StopPooling()
{
// Singal to main thread to stop
SetEvent(m_hEventKill);
// Check thread state
if (WaitForSingleObject( m_hMainThread, 10000) == WAIT_TIMEOUT)
*****
Your choice of name is exceptionally poor; this is not the "main thread", and did you
notice that WFSO can return SEVERAL values, and you test for only ONE of them? Why do you
think this code could be remotely considered viable, let alone correct?
*****
{
// If thread is still running then stop it brutally
if (m_hMainThread != NULL)
{
TerminateThread( m_hMainThread, 0 );
*****
Tasteless in the extreme. If the thread is still running, your program is screwed up and
this will only make the situation much worse.
*****
}
}
// Thread is null
CloseHandle( m_hMainThread );
m_hMainThread = NULL;
return TRUE;
}
DWORD WINAPI CPooler::MainThreadProc(LPVOID lpParameter)
****
Since this is a static method, it is considered tasteful to put
/* static */ in front of it. Also, you can immediately put yourself back into "C++ space"
so you don't need to keep talking about pThis; see my essay on worker threads on my MVP
Tips site.
****
{
// Get pointer on CModBusTcpPooler class object
CPooler *pThis = reinterpret_cast< CPooler *>( lpParameter ) ;
// Cretae kill event (for main thread)
pThis->m_hEventKill = CreateEvent(NULL, FALSE, FALSE, NULL);
****
Who can call StopPooling and why do you believe that they will not call it before this
code is executed? This is truly screwed up code. You are depending upon the thread to
create its own shutdown event, but you have the potential to use it before it is created.
Bad design. Rethink it. Why is it autoreset?
****
// Signal startup event
SetEvent( pThis->m_hStartupEvent );
// Loop until kill event has not been signaled
UINT32 un32Index = 0;
while(WaitForSingleObject( pThis->m_hEventKill, 5) != WAIT_OBJECT_0)
****
Why is it that you want this thread running continuously, polling for the kill event? Or
did you know that choosing a 5ms wait means this thread will consume as much of the CPU as
possible?
{
// Do pooler job
// Give time to CPU
Sleep(5);
****
There is one very simple rule you can always, ALWAYS be sure of: if your threading system
doesn't work correctly unless you have sprinkled one or more Sleep() calls in it, YOUR
DESIGN IS WRONG. Note that the comment is also nonsense, since you can't "give time" to
the CPU. You can only consume time. Which this thread does, as much as it can possibly
get. Timeouts should be on the order of several hundred milliseconds if your goal is to
minimize CPU waste.
The whole design is deeply and irrecoverably flawed. Rewrite it. From scratch.
****
}
// No need this handle anymore
CloseHandle(pThis->m_hEventKill);
pThis->m_hEventKill = NULL;
return FALSE;
****
FALSE is a BOOL. The return type is DWORD. return 0 makes sense; return FALSE does not.
*****
}
CPooler::~CPooler(void)
{
// Stop main thread
StopPooling();
}
****
Why is it you think this destructor is called before ExitInstance is called?
*****
------------------------------------------------------------------------
Now, always in this DLL, I have implemented a wrapper containing an
array of 100 available CPooler that can be created, started, stopped
and removed. Here is the more important part.
****
Note that you CANNOT call any thread-creation function in the InitInstance of a DLL. This
is documented quite carefully; InitInstance in a DLL is called from DllMain and
consequently anything forbidden in DllMain is forbidden in a DLL's InitInstance handler.
THis means that you CANNOT create or destroy a thread in either InitInstance or
ExitInstance of a DLL. Ever.
****
------------------------------------------------------------------------
CPooler* g_Pooler[100];
extern "C" POOLER_LIB_API INT32 __cdecl PoolerCreate(...)
{
// Look for empty place in the array and create the Pooler
int i = 0;
while( i < MAX_POOLER && g_Pooler[i] != NULL ) ++i;
// If empty space was found
CPooler* pTcpPooler = NULL;
if (i < MAX_POOLER)
{
// Create new pooler
pPooler = new CPooler(...);
// Start it !
if ( pPooler->StartPooling() )
{
g_Pooler[in32Result = i] = pPooler;
*****
Perhaps you should learn how to program. You have an assignment embedded in the left-hand
side operand of an assignment. This is so mind-bogglingly tasteless that I'm amazed that
a compiler should be allowed to accept it. There is NO REASON to now write this as two
assignment statements! Just because something is syntactically permissible does not mean
it ever makes sense to use it!
****
}
else
{
// Cannot start so delete instance and return error
delete pPooler;
in32Result = -2;
****
Have you heard of the enum data type? Of #define? Why these seemingly random numbers
that have no mnemonic value? What could anyone possibly make of them? I do not even see
some useful comment that precedes the function that is the documentation of the function,
so the only way to figure out what these values do is to read the code. This is VERY BAD
style.
typedef enum { OK, POOL_START_FAILED, POOL_LIMIT_EXCEEDED } PoolResult;
and return one of these values!
****
}
}
else
{
// No more space to create a pooler
in32Result = -1;
}
return in32Result;
}
****
I find it amazing so many people go through so much work for so little reward, trying to
reinvent something that already exists, and is done better. It's called an I/O Completion
Port, and it is the simplest thread-pooling mechanism you will find. I have yet to see
anyone who has succeeded in coding their own thread-pooling class even remotely correctly,
and this is just another instance of a bad design with deep and fundamental flaws.
Read my essay on the use of I/O Completion Ports on my MVP Tips site.
*****
------------------------------------------------------------------------
Now simply to test the class, inside the InitInstance function, I created
a new Pooler and then I delete it just after. Like this
------------------------------------------------------------------------
BOOL CPoolerLibraryApp::InitInstance()
{
CWinApp::InitInstance();
CPooler* p = new CPooler();
p->StartPooling()
delete p; // PROBLEM HERE !
****
There probably is. You have not described it, so there is no way to tell
****
return TRUE;
}
------------------------------------------------------------------------
But I got some very strange behavior while the delete operation and after !
Like the heap was corrupted or I don't know.
*****
If you don't know, how do you think we will be able to determine what is happening? Note
that pool corruption can be caused by ANYTHING, ANYWHERE, and the fact that delete
*detected* it does not suggest there is a problem with delete!
If there is a problem, we can only help you if you accurately describe what has happened.
****
Anyway the application is
freezing
on this line :
if (WaitForSingleObject( m_hMainThread, 10000) == WAIT_TIMEOUT) inside the
function CPooler::StopPooling().
*****
Probably. There are so many things wrong with this code that I would not even suggest
*trying* to debug it; nothing short of a total rewrite is going to salvage this mess.
*****
I put a brake point on the "return FALSE;" line of the function
CPooler::MainThreadProc. While the destructor is called the thread is well
stopped
but the WaitForSingleObject still wait for thread ending. WHY ?
*****
One of the first causes I would look for is the fact that you simplistically assume that
the ONLY value that could POSSIBLY be returned is WAIT_TIMEOUT. Did you look to see WHAT
value is actually returned? For example, WAIT_ERROR? (That means "invalid handle", and
given the number of deep errors I see here, that is what I would suspect). It is
erroneous coding to assume that WFSO can return only one value, and that you will look for
only one value. If your code does not ALWAYS say
switch(WFSO(...))
{
case ...
...
case ....
...
default:
ASSERT(FALSE); // things are messed up
...
}