Re: Clean-up in OnDestroy

From:

"K?rsat" <xx@yy.com>

Newsgroups:

microsoft.public.vc.mfc

Date:

Tue, 20 May 2008 22:40:16 +0300

Message-ID:

<#v69TFruIHA.5244@TK2MSFTNGP02.phx.gbl>

Thanks a lot Joseph,

The sample code is impromptu, not an excerpt from any serious project. I
wanted to introduce an idea, so it may has some sync issues as you warn.

Well, I shoud go reading your interesting essays now.

"Joseph M. Newcomer" <newcomer@flounder.com> wrote in message
news:iva534tgr484v29b3pqktql7ms7ncrp6vf@4ax.com...

See below...
On Tue, 20 May 2008 12:36:15 +0300, "K?rsat" <xx@yy.com> wrote:

Thank you for your very useful advices.

I use the semaphore to process queued items. When I enqueue a new item, I
call ReleaseSemaphore () by 1. I can simply replace WaitForSingleObject
with
GetQueuedCompletionStatus and ReleaseSemaphore with
PostQueuedCompletionStatus to implement IOCP based queue processing. I
will
do this but I don't know why you use an extra object (an event) while you
simply terminate a thread using already-exist object, in this case the
semaphore. What about the implementation like this :

****
If a semaphore controls a queue, it should be limited to controlling just
the queue. If
there is a shutdown event, it should be limited to just shutting down.
Using one object
to serve two purposes is confusing, and requires an interaction (such as
checking a
boolean) that seems inappropriate.
*****

A queue class which is a wrapper around the STL queue. In enqueue method,
I
first checked the bContinue variable. If it is false then I return false
and
do not enqueue the item. This means queue is closed and terminated as soon
as it's all items processed.

bool MyQueue::enqueue (Item item)
{
   if (!bContinue)
   {
       return false;
   }

   lock ();
   _internal.enqueue (item);
   unlock ();

   return true;
}

My closeQueue method simply set bContinue to false and release semaphore
by
thread count :

void MyQueue::closeQueue ()
{
   bContinue = false;

   ReleaseSemaphore (hSemaphore, THREAD_COUNT);
}

I have a empty method :

bool empty ()
{
   return _internal.empty ();
}

***
You must provide a lock external to this or the value is not guaranteed to
be correct
****

and my thread proc is like this :

while (true)
{
   if (WAIT_OBJECT_0 == WaitForSingleObject (hSemaphore, INFINITE))
   {
        if (!bContinue)
       {
           if (q.empty)

****
This has potential race conditions, and makes me nervous. I suspect it is
not actually
correct, but when I see tests like this which are not locked, I'm
suspicious.
****

           {
               break;
           }
       }

       // process item...
   }
}

return 0;

Now if I have 10 items in the queue and 5 worker threads, the semaphore
will
be released 15 times when I call closeQueue method (10 times for items and
5
times for worker threads). After worker threads processed 10 items, the
semaphore will be released 5 times more which will cause all 5 threads to
exit. Anyway I will use IOCP but just want to hear your comments on this
design.

***
If I have n threads, I will PostQueuedCompletionStatus the shutdown event
n times
****

In respect ofm dialog closure, I have a tray icon for my dialog based
application. When user right-click that icon I show a menu with a close
command in it. What I try to do is implement that close command. I try to
call EndDialog but then the OnClose event is not occured. I will be check
out your related essay to clarify my mind.

****
Then why call EndDialog? Why not PostMessage(WM_CLOSE)? THen the OnClose
handler is
called!
joe
****

"Joseph M. Newcomer" <newcomer@flounder.com> wrote in message
news:fl9s2419obc7igvtlahs9fjae9rblfc821@4ax.com...

See below...
On Sat, 17 May 2008 01:30:43 +0300, "K?rsat" <xx@yy.com> wrote:

Hi Joseph,

If a thread is blocked on a semaphore what do you offer to do to
terminate
that thread other than releasing the semaphore?

Suppose a thread proc like this :

while (bContinue)
{
       if (WAIT_OBJECT_0 == WaitForSingleObject (hSemaphore, INFINITE))
       {
               // Land the spaceship...
       }
       else
       {
               // I always wonder what other programmers do here...
               // My choice is landing the spaceship :)
       }

****
       HANDLE waiters[2];
       waiters[0] = hShutdown;
       waiters[1] = hSemaphore;
       switch(::WaitForMultipleObjects(2, waiters, FALSE, INFINITE))
                   // check docs to see if I remembered the params inthe
right order...
            { /* waitfor */
             case WAIT_OBJECT_0:
                    ...land the spaceship
                    break;
             case WAIT_OBJECT_0 + 1:
                   bContinue = FALSE;
                   continue;
             case WAIT_TIMEOUT:
                    ASSERT(FALSE); // we don't support timeout
                    continue;
             default:
                    ASSERT(FALSE); // what are we doing here?
                    bContinue = FALSE; // we're in trouble, get out
                    continue;
            } /* waitfor */

Now, presumably the semaphore can take on values > 1, possibly
representing the number of
elements in a queue. If it can't, then it is probably inappropriate
here,
and event would
be a better choice.

You would have initialized hShutdown as:

hShutdown = ::CreateEvent(NULL, FALSE, FALSE, NULL);

Also, if you are using the semaphore to control a queue, this is
probably
an
overly-complex design. Use an I/O Completion Port as an interthread
queue
and
PostQueuedMessage to it, and use GetQueuedCompletionStatus in your loop.
You can use the
completion key to signal events like shutdown; see my essay on I/O
Completion Ports on my
MVP Tips site. The code I showed you is essentially from my essay on
worker threads. If
you are not using semaphores to manage queues, rethink whether or not a
semaphore is the
correct solution to your problem. I prefer IOCPs for queuing to
threads.
*****

}

If this thread blocked on the semaphore then we set bContinue to false
and
relese the semaphore to terminate the thread gracefully :

bContinue = false;
ReleaseSemaphore (hSemaphore, ...);

****
But why use a semaphore, which is a counted exclusion object, if what
you
want is
something else?
****

Am I right now?

In respect of waiting threads to terminate :

I always think that terminating a process before it's all threads
terminated
is a bad practice. So I initiate terminations for threads and wait them
to
terminate. It sometimes gives useful information about idden bugs
related
to
thread termination. Off course, your warning about dead-lock should be
noted
and threads should be leave alone with their terminations if requred to
do
so.

****
Note that destroying windows and terminating processes are separable
events. For example,
I would not allow termination to start until the threads had all sent
asynchronous
termination notifications. To do this, I would use the techniques I
discuss in the
OnClose handler discussion in my essay, but instead of putting them in
OnClose, I would
put them in the OnOK or OnCancel handlers. More to the point, I would
not
actually allow
OnOK or OnCancel to do anything, and if I put a "Close" button on, it
would
PostMessage(WM_CLOSE) to my window. I would tend to override OnOK and
OnCancel (see my
essay on dialog-based apps) and do everything from OnClose, by forcing
all
termination
conditions to go through OnClose. And of course this means I would
never
call EndDialog
EXCEPT in OnClose.
****

And eventually, I should think about ActiveX related cases a bit more
considering your warning.

****
ActiveX is sort of the fly in the ointment here; you don't know what it
is
doing or what
it is expecting, and therefore have to be a bit careful.

Premature cleanup can add complexity as well; if you clean things up
entirely in your
close protocol but prior to destroy, you run the risk that the ActiveX
control will
require some of the state you destroyed, so you have to provide for
that.
joe
*****

"Joseph M. Newcomer" <newcomer@flounder.com> wrote in message
news:0qvr24divnl5v03c6euatpv5764hmjjqlc@4ax.com...

OnClose would defiinitely be the wrong place to do this, and OnDestory
*might* the best
place to do this.

See below...
On Sat, 17 May 2008 00:14:25 +0300, "K?r?at" <xx@yy.com> wrote:

In clean up, I do :

- Call some ActiveX control's cleanup methods which cause some windows
messages generated and some ActiveX events occured

***
OnDestroy would be a good place to do this. WHo is supposed to
receive
the messages
"generated". Note that you have to make sure that all these messages
and
events are
processed synchronously, that is, before the call to the ActiveX
control
returns, or you
can be left with dangling pointers (hence the *might* comment)
****

- Set some events (kernel objects) and release some semaphores to
terminate
worker threads and wait those threads to terminate by using
WaitForSingleObject ()

****
You would not use semaphores to terminate worker threads; this would
be
a
very bad choice
most of the time. You would use Events. If you are using semaphores,
I
suspect you do
not know how to use them. It is generally a Bad Idea to wait for
threads
to terminate.
They should terminate cleanly on their own. Note that if the theads
do
a
PostMessage,
they are safe, but the WFSO will deadlock the threads if they do
SendMessage. Overall,
not a good strategy.
****

- Close some handles,

****
Typical OnDestroy type of thing to do
****

- Deallocate some memory

****
Typical OnDestroy type of thing to do
****

That is all.

"AliR (VC++ MVP)" <AliR@online.nospam> wrote in message
news:XwhXj.2056$r82.572@nlpi069.nbdc.sbc.com...

What are you cleaning up?

If the cleanup does not require access to the controls then you
should
put
the cleanup code in the destructor.

AliR.

"K?r?at" <xx@yy.com> wrote in message
news:OGiL391tIHA.4560@TK2MSFTNGP03.phx.gbl...

Hi,

In my dialog based MFC application I put all clean-up code into
OnClose
(). Recently I noticed that calling EndDialog () closes the
application
but without executing OnClose (). So I move my clean-up code into
OnDestroy (). May this cause any validity problems?

Thanks in advance

Joseph M. Newcomer [MVP]
email: newcomer@flounder.com
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm