MFC DLL - ExitInstance hang on WaitForSingleObject

[Erakis]

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);

// Check for startup event
switch( WaitForSingleObject( m_hStartupEvent, 1000) )
{
case WAIT_TIMEOUT :
bRet = FALSE; // Thread time-out
break;

case WAIT_OBJECT_0 :
bRet = TRUE; // Thread sucessfully started
break;
}

// Close startup handle
CloseHandle(m_hStartupEvent);
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)
{
// If thread is still running then stop it brutally
if (m_hMainThread != NULL)
{
TerminateThread( m_hMainThread, 0 );
}
}

// Thread is null
CloseHandle( m_hMainThread );
m_hMainThread = NULL;

return TRUE;
}

DWORD WINAPI CPooler::MainThreadProc(LPVOID lpParameter)
{
// 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);

// 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)
{
// Do pooler job

// Give time to CPU
Sleep(5);
}

// No need this handle anymore
CloseHandle(pThis->m_hEventKill);
pThis->m_hEventKill = NULL;

return FALSE;
}

CPooler::~CPooler(void)
{
// Stop main thread
StopPooling();
}

------------------------------------------------------------------------

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.

------------------------------------------------------------------------
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;
}
else
{
// Cannot start so delete instance and return error
delete pPooler;
in32Result = -2;
}
}
else
{
// No more space to create a pooler
in32Result = -1;
}

return in32Result;
}
------------------------------------------------------------------------

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 !

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. Anyway the application is
freezing
on this line :

if (WaitForSingleObject( m_hMainThread, 10000) == WAIT_TIMEOUT) inside the
function CPooler::StopPooling().

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 ?

I already try to create a Pooler inside the InitInstance and destroying it
inside
the ExitInstance and I get the same result.

PS : To call the function of this DLL I made a simple MFC dialog project,
use LoadLibray, GetProcAddress, etc...

Any help should be VERY appreciated because I worked on this problem
while 16 hours :(

Best regards,
Martin

[Doug Harrison [MVP]]

On Fri, 1 May 2009 16:31:01 -0700, Erakis
<Era...@discussions.microsoft.com> wrote:

>Hi,

Hi! Jump directly to the end to see what I think the problem is. Start from
the beginning for some more or less unrelated tips.

>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);
>
> // Check for startup event
> switch( WaitForSingleObject( m_hStartupEvent, 1000) )
> {
> case WAIT_TIMEOUT :
> bRet = FALSE; // Thread time-out
> break;
>
> case WAIT_OBJECT_0 :
> bRet = TRUE; // Thread sucessfully started
> break;
> }
>
> // Close startup handle
> CloseHandle(m_hStartupEvent);
> m_hStartupEvent = NULL;
>
> return bRet;
>}

You should use AfxBeginThread and CWinThread in an MFC program. See this
page for info on using CWinThread correctly:

http://members.cox.net/doug_web/threads.htm

The logic in StartPooling is rather creative.

>BOOL CPooler::StopPooling()
>{
> // Singal to main thread to stop
> SetEvent(m_hEventKill);
>
> // Check thread state
> if (WaitForSingleObject( m_hMainThread, 10000) == WAIT_TIMEOUT)
> {
> // If thread is still running then stop it brutally
> if (m_hMainThread != NULL)
> {
> TerminateThread( m_hMainThread, 0 );
> }
> }
>
> // Thread is null
> CloseHandle( m_hMainThread );
> m_hMainThread = NULL;
>
> return TRUE;
>}

That's even more questionable. If the thread is still running after being
asked to exit, it has a bug, and terminating the thread isn't going to help
you find the bug. It may also lead your program to subsequently malfunction
in seemingly unrelated ways.

>DWORD WINAPI CPooler::MainThreadProc(LPVOID lpParameter)
>{
> // 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);
>
> // 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)
> {
> // Do pooler job
>
> // Give time to CPU
> Sleep(5);
> }

You're doing a timed wait at the top of the loop, so you don't need to
sleep here. It probably doesn't make any sense to sleep anyway given the
preemptive scheduler.

> // No need this handle anymore
> CloseHandle(pThis->m_hEventKill);
> pThis->m_hEventKill = NULL;
>
> return FALSE;
>}
>
>CPooler::~CPooler(void)
>{
> // Stop main thread
> StopPooling();
>}
>
>------------------------------------------------------------------------
>
>Now, always in this DLL

That's likely your problem...

That spelling makes sense, but it's really "breakpoint".

>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 ?
>
>I already try to create a Pooler inside the InitInstance and destroying it
>inside
>the ExitInstance and I get the same result.
>
>PS : To call the function of this DLL I made a simple MFC dialog project,
>use LoadLibray, GetProcAddress, etc...
>
>Any help should be VERY appreciated because I worked on this problem
>while 16 hours :(

IIRC, InitInstance and ExitInstance for MFC DLLs are called in DllMain
context, from which it is forbidden to do things like create threads. This
is definitely true for globals, whose ctors and dtors are called from
DllMain. Therefore, you need to initialization and destroy these objects
from EXE context.

--
Doug Harrison
Visual C++ MVP

[Erakis]

Hi Doug,

Sorry for "brake point" :)
First of all I want to thank for your great help, this is really appreciated
:)

If I well understand what you said, I cannot create or stop thread in
InitInstance/ExitInstance ? Why MSDN do not talk about that ? Else, what it
is permit to do and not in those two fuction ?

If I make two function in the DLL ; InitLib() and CloseLib()
So the EXE could load the DLL, call InitLib(), create a new Pooler using the
PoolerCreate() function and at close event of the EXE, the EXE should call
CloseLib()
to stop all the running Pooler and free other resource ?

What if the EXE do not call CloseLib() ? It still will produce strange
behavior ? Leak ?

Because this DLL is a library and anyone could use it so I don't want to
introduce any leak in case of bad use. Do I'm stuck with this limitation ?

Best regards,
Martin

[Joseph M. Newcomer]

See below...

On Fri, 1 May 2009 16:31:01 -0700, Erakis <Era...@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
...
}

to try to sort out what really happened, it is not even worth trying to analyze the code.
It is wrong. Fix it (but in this case, that fixes a superficial flaw; the deep flaws are
so deep that the code can only be scrapped and rewritten)
*****

>
>I already try to create a Pooler inside the InitInstance and destroying it
>inside
>the ExitInstance and I get the same result.
>
>PS : To call the function of this DLL I made a simple MFC dialog project,
>use LoadLibray, GetProcAddress, etc...
>
>Any help should be VERY appreciated because I worked on this problem
>while 16 hours :(

*****
You have not said what the reason for the thread is, or what it does. Or how it knows
what to work on, or how it knows how to report success. You have created a solution
without stating the problem. So the FIRST thing to do is to state the problem. You
should know the interarrival rate of events to be processed by the threads, for example;
if you are processing one event per second it is quite different than if you have to
process thousands of events per second. It changes the whole picture.

Here is the code for the most general thread-pooling mechanism I can think of.

I would create an I/O Completion Port, and a set of threads to work on it, NOT in the
InitInstance of the DLL (which is impossible), but by providing two entry points to your
DLL to initialize and destroy the thread pool.

typedef void (CDECL * POOLER_CALLBACK_FN)(LPVOID);

class Pooler {
public:
Pooler() { count = 0; iocp = NULL; stop = NULL; }
HANDLE Create(UINT n);
void Destroy();
void Enqueue( POOLER_CALLBACK_FN f, LPVOID p);
protected:
typedef enum { CODE_STOP = -1, CODE_EXECUTE } Codes;
void Kill();
LONG count;
static UINT pooler(LPVOID);
void runPool();
HANDLE iocp;
HANDLE stop;
};

HANDLE Pooler::Create(UINT n)
{
ASSERT(iocp == NULL);
if(iocp != NULL)
return NULL; // tried to call Create more than once

iocp = ::CreateIoCompletionPort(INVALID_HANDLE_VALUE, NULL, 0, 0);
for(UINT i = 0; i < n; i++)
{
CWinThread * thread = AfxBeginThread(pooler, this);
}

return iocp;
}

void Pooler::Destroy()
{
stop = ::CreateEvent(NULL, TRUE, FALSE, NULL);

for(UINT i = 0; i < count; i++)
::PostQueuedCompletionStatus(iocp, (DWORD)CODE_STOP, 0, NULL);

switch(::WaitForSingleObject(stop, INFINITE))
{ /* wfso */
case WAIT_OBJECT_0:
break;
default:
ASSERT(FALSE);
break;
} /* wfso */
::CloseHandle(stop);
::CloseHandle(iocp);
}

void Pooler::Enqueue(POOLER_CALLBACK_FN fn, LPVOID p)
{
::PostQueuedCompletionStatus(iocp, (DWORD)CODE_EXECUTE, (ULONG_PTR) p, (LPOVERLAPPED)
fn);
}

/* static */ UINT Pooler::pooler(LPVOID p)
{
Pooler * me = (Pooler * p);
me->runPool();
return 0;
}

void Pooler::runPool()
{
InterlockedIncrement(&count);
BOOL running = TRUE;
while(running)
{ /* thread loop */
Codes code;
LPVOID parm;
POOLER_CALLBACK_FN fn;

BOOL b = ::GetQueuedCompletionStatus(iocp,
(LPDWORD)&code,
(PULONG_PTR)&parm,
(POOLER_CALLBACK_FN) fn,
INFINITE);
if(!b)
{ /* failed */
... deal with error
} /* failed */
else
{ /* process request */
switch(code)
{ /* code */
case CODE_STOP:
running = FALSE;
continue;
case CODE_EXECUTE:
try {
fn(parm);
}
catch(...)
{
}
continue;
default:
ASSERT(FALSE);
running = FALSE;
continue;
} /* code */

if(InterlockedDecrement(&count) == 0)
::SetEvent(stop);
}

This is fully general, it works well, does not consume 100% of the CPU, requires no
Sleep() calls, does not require a fixed-size array to limit the number of threads, and is
about as simple as you can make a thread pooler. If you want to do something, you call
the Enqueue function, which takes a pointer to the function and a pointer to whatever is
passed to the function; eventually, the request is dequeued, the function is called with
the parameter and it does whatever it wants to do. Should it be so tasteless as to throw
an exception, this will not kill the thread, because of the try/catch logic that catches
and ignores all exceptions.

You can eliminate the function passed in and the LPVOID by simply calling a virtual method
of the class, e.g., enhance the class I'm about to show by changing and adding as shown

class Pooler {
public:
void Enqueue(LPVOID p);

protected:
virtual Execute(LPVOID p) PURE;
}

Now, to get a specialized queue, you would subclass Pooler and implement the Execute
method. This avoids having to pass a pointer to a static function. So the dequeue
handler would do
case CODE_EXECUTE:
try {
Execute(parm);
}
catch(...)
{
}

your derived class would be

class MyPooler : public Pooler {
protected:
void Execute(LPVOID p) { ... }
};
(or put the body in the .cpp file; it doesn't matter)

I typed this in off the top of my head, but I think it is reasonably complete and correct.
I wouldn't even try to make sense out of what you have; it has too many deep and
fundamental design errors in it.
joe
****
>
>Best regards,
>Martin
Joseph M. Newcomer [MVP]
email: newc...@flounder.com
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm

[Joseph M. Newcomer]

See below...

On Fri, 1 May 2009 19:17:01 -0700, Erakis <Era...@discussions.microsoft.com> wrote:

>Hi Doug,
>
>Sorry for "brake point" :)
>First of all I want to thank for your great help, this is really appreciated
>:)
>
>If I well understand what you said, I cannot create or stop thread in
>InitInstance/ExitInstance ? Why MSDN do not talk about that ? Else, what it
>is permit to do and not in those two fuction ?

****
Probably because the MSDN documentation was written for, and has not been updated since,
MFC 1.0 on 16-bit Windows. It is confusing, misleading, incomplete, and in some cases
out-and-out erroneous. See my article which I just finished on my MVP Tips site:

http://www.flounder.com/msdn_documentation_errors_and_omissions.htm#CWinApp::InitInstance
****

>
>If I make two function in the DLL ; InitLib() and CloseLib()
>So the EXE could load the DLL, call InitLib(), create a new Pooler using the
>PoolerCreate() function and at close event of the EXE, the EXE should call
>CloseLib()
>to stop all the running Pooler and free other resource ?

****
Yes. It would be erroneous for the caller to fail to call CloseLib
****

>
>What if the EXE do not call CloseLib() ? It still will produce strange
>behavior ? Leak ?

****
Probably. Of course, the program is erroneous, so who cares? If the programmer cannot
follow your documentation (and you *are* writing careful documentation to go with this,
aren't you?) then whatever happens is not your fault. You are not responsible for
protecting the user of your DLL from their failure to program correctly.
****

>
>Because this DLL is a library and anyone could use it so I don't want to
>introduce any leak in case of bad use. Do I'm stuck with this limitation ?

****
You are not "introducing" a leak; the failure of the user of your DLL to call your
CloseLib is the user of your DLL causing a leak because of bad use, and this is not your
responsibility to solve.
joe
*****

[Doug Harrison [MVP]]

On Fri, 1 May 2009 19:17:01 -0700, Erakis
<Era...@discussions.microsoft.com> wrote:

>Hi Doug,
>
>Sorry for "brake point" :)
>First of all I want to thank for your great help, this is really appreciated
>:)
>
>If I well understand what you said, I cannot create or stop thread in
>InitInstance/ExitInstance ? Why MSDN do not talk about that ? Else, what it
>is permit to do and not in those two fuction ?

MSDN doesn't talk about a lot of things, and that's especially true when
you look at individual articles as opposed to MSDN as a whole. As I recall,
the application object for an MFC DLL (only "non-extension" MFC DLLs have
application objects) is a global variable, globals in DLLs are constructed
and destructed in DllMain context, and InitInstance/ExitInstance are called
as part of app object construction/destruction. For more on DllMain
restrictions, see the DllMain documentation and search MSDN for "DllMain"
and "loader lock".

See also my last four posts in this thread, where I explain a scenario that
causes deadlock when a thread is created in DllMain:

http://groups.google.com/group/microsoft.public.vc.mfc/browse_frm/thread/7d0576f42150e80d/63906b040cd5e2ff?hl=en#63906b040cd5e2ff

Finally, there are a lot of MS people who write blogs that contain info you
won't find in MSDN, so that's another source to search.

>If I make two function in the DLL ; InitLib() and CloseLib()
>So the EXE could load the DLL, call InitLib(), create a new Pooler using the
>PoolerCreate() function and at close event of the EXE, the EXE should call
>CloseLib()
>to stop all the running Pooler and free other resource ?
>
>What if the EXE do not call CloseLib() ? It still will produce strange
>behavior ? Leak ?
>
>Because this DLL is a library and anyone could use it so I don't want to
>introduce any leak in case of bad use. Do I'm stuck with this limitation ?

Yep. I don't see any way to make it fully automatic.

[Erakis]

Hi,

First of all I want to thank you for your helpfull advise :)

However I have some question that I did not found the answer during week-end.

Suppose I'm using the m_bAutoDelete as you propose and I still need to wait
for thread to be started ; ** Take a look at the WAIT_TIMEOUT condition **

------------------------------------------------------------------------------------

// Create startup event
m_hStartupEvent = CreateEvent(NULL, TRUE, FALSE, NULL);

// Create main thread
m_PoolerThread = AfxBeginThread( PoolerThreadProc, static_cast<LPVOID>( this
), THREAD_PRIORITY_NORMAL, 0, CREATE_SUSPENDED );
m_PoolerThread.m_bAutoDelete = FALSE;

// Resume thread
m_PoolerThread.ResumeThread();

// Check for startup event
switch( WaitForSingleObject( m_hStartupEvent, 1000) )

{
case WAIT_TIMEOUT :
bRet = FALSE; // Thread time-out

// If I well understood the thread could be still be running here ?
// So what should I do ? I think I'd should stop it ? But how to
stop it correctly ?
// Simply called "delete m_PoolerThread" ?
break;

case WAIT_OBJECT_0 :
bRet = TRUE; // Thread sucessfully started
break;
}

// Close startup handle
CloseHandle(m_hStartupEvent);

m_hStartupEvent = NULL;
------------------------------------------------------------------------------------

About Stop pooling function there was already a mechanism to check if it is
called
before the StartPooler function. Sorry for that it is because I minimized
the code
to make it easier to read in this forum.

Best regards,
Martin

[Scott McPhillips [MVP]]

"Erakis" <Era...@discussions.microsoft.com> wrote in message
news:9491476E-8700-4DD8...@microsoft.com...

If you get this timeout then the thread has not yet started. That makes it
impossible to stop it. Why do you want this timeout? The code is simple
enough that the thread should always start. (But you have no way to know
how long it might take.) Instead of passing a timeout value you can pass
INFINITE.

--
Scott McPhillips [VC++ MVP]

[Erakis]

Hi Scott :)

I'm doing that because "Joseph M. Newcomer" advise me to handle all possible
return value of WaitForSingleObject. WAIT_TIMEOUT is one of the possible
value.

I admit that the code supplied in this exemple is very simple because I
wanted to keep the post clearly to get helped.

In fact this class is an ModBus pooler, there is a lot of code but nothing
is executed between the thread starting event and the
SetEvent(m_hStartupEvent).

So in fact is no reason that thread may stuck using INFINITE. But why not
implement the WAIT_TIMEOUT for good programming as Joseph propose ?

Best regards,

[Scott McPhillips [MVP]]

"Erakis" <Era...@discussions.microsoft.com> wrote in message

news:EEB11EEF-F98A-49AF...@microsoft.com...

> Hi Scott :)
>
> I'm doing that because "Joseph M. Newcomer" advise me to handle all
> possible
> return value of WaitForSingleObject. WAIT_TIMEOUT is one of the possible
> value.
>
> I admit that the code supplied in this exemple is very simple because I
> wanted to keep the post clearly to get helped.
>
> In fact this class is an ModBus pooler, there is a lot of code but nothing
> is executed between the thread starting event and the
> SetEvent(m_hStartupEvent).
>
> So in fact is no reason that thread may stuck using INFINITE. But why not
> implement the WAIT_TIMEOUT for good programming as Joseph propose ?

You can only get WAIT_TIMEOUT if you set a timeout. But if there is no need
for a timeout set INFINITE and the problem goes away.

[David Ching]

"Scott McPhillips [MVP]" <org-dot-mvps-at-scottmcp> wrote in message
news:uMPiRvNz...@TK2MSFTNGP04.phx.gbl...

> You can only get WAIT_TIMEOUT if you set a timeout. But if there is no
> need for a timeout set INFINITE and the problem goes away.
>

I agree, there is no need to handle WAIT_TIMEOUT if that cannot be generated
(which it cannot by specifying INFINITE). It confuses code to handle
WAIT_TIMEOUT and providing a good error handling in a case that cannot
happen (and really there is seldom a really good way of handling a timeout
since something in another thread has gone very wrong, and it's likely not
possible to entirely fix that). So when the reader looks at this code, they
think there is a possible bug in the WAIT_TIMEOUT case, but this is the
wrong impression since it won't ever execute. Whatever gives the code
reader the correct impression is the correct way to code it.

-- David

[Joseph M. Newcomer]

See below...

On Mon, 4 May 2009 06:29:01 -0700, Erakis <Era...@discussions.microsoft.com> wrote:

>Hi,
>

>First of all I want to thank you for your helpfull advise :)
>
>However I have some question that I did not found the answer during week-end.
>
>Suppose I'm using the m_bAutoDelete as you propose and I still need to wait
>for thread to be started ; ** Take a look at the WAIT_TIMEOUT condition **

*****
In general, putting timeouts in is akin to the use of Sleep() statements in the thread. It
almost always waves a big red flag that says "dangerous design decisions here". Or, as I
tell my students, "Never put a timeout on a WaitFor unless you have a plan for exactly
what you are going to do when it times out, and this plan will work under all possible
timing situations under every possible concurrency scenario". Otherwise, your code is
wrong. I think this design is very bad, and I don't see any reason to waste time trying
to figure out how to debug it. Either the thread starts, or it doesn't. If it starts,
you will get a non-NULL return. If it doesn't start, you will get a NULL. Everything
beyond that is irrelevant. If you care about timeouts, don't do it this way.
joe

>
>------------------------------------------------------------------------------------
>// Create startup event
>m_hStartupEvent = CreateEvent(NULL, TRUE, FALSE, NULL);
>
>// Create main thread
>m_PoolerThread = AfxBeginThread( PoolerThreadProc, static_cast<LPVOID>( this
>), THREAD_PRIORITY_NORMAL, 0, CREATE_SUSPENDED );
>m_PoolerThread.m_bAutoDelete = FALSE;
>
>// Resume thread
>m_PoolerThread.ResumeThread();
>
>// Check for startup event
>switch( WaitForSingleObject( m_hStartupEvent, 1000) )
>{
> case WAIT_TIMEOUT :
> bRet = FALSE; // Thread time-out
> // If I well understood the thread could be still be running here ?

****
Of course. It almost certainly could still be running. Or not.
****

> // So what should I do ? I think I'd should stop it ? But how to
>stop it correctly ?

****
By having the timeout, you open yourself to these questions. If you don't bother doing
the wait, the issues do not arise.
****

> // Simply called "delete m_PoolerThread" ?

****
This would probably be a fatal error.

****
> break;
>
> case WAIT_OBJECT_0 :
> bRet = TRUE; // Thread sucessfully started

****
This whole thing is erroneously predicated on the concept that any given wait timeout is
going to make sense. Generally, this should not be a model you use for programming
concurrency. The whole design is wrong.
****

[Joseph M. Newcomer]

Yes, but see my earlier message. You must (a) handle all cases and (b) HANDLE all cases.
That is, you have to account for other errors that can occur, but you must also make sure
that your actual response to the condition is correct. As I pointed out, the presumtion
that the return code can only be WAIT_TIMEOUT is erroneous, but in addition, you must have
a very careful plan as to what happens on a timeout. Your idea that you should kill the
thread is erroneous. You simply signal the thread to stop, should it start up, and go
away. Eventually, the thread will stop, and you will get a notification that it stopped.
But that's an asynchronous event.

One key failure of your design: you have erroneously assumed that you have to represent
sequentiality of execution by sequentiality of syntactic structure. The whole idea that
you can synchronously depend on the thread starting, and you have to actually stop and
wait to see if it started, is the design failure here. I would not waste any effort doing
ANY of this at this point. Call AfxBeginThread, handle the NULL return if it failed, and
otherwise return. At some indefinite point in the future something will happen that will
indicate the thread is running, or it has prematurely stopped. It is the thread's
responsibility to inform about this. You are forcing a sequential model onto a parallel
environment, and this is inherently bad design.

So: you can assume the thread is running if AfxBeginThread returns a non-NULL value. There
is no reason to wait for it, or to time out, or anything like that. If it ever stops
running, it will tell you. How? Because you will have programmed to do so, by any one of
several alternative mechanisms. So the whole idea that you will WaitForSingleObject
already is a failed part of the design. I do not see any reason for it to exist at all.
joe

[Joseph M. Newcomer]

There is nothing wrong with the design that a total redesign cannot fix.
joe