Prev: Knit picking
Next: No source on specified on that location
From: Joseph M. Newcomer on 21 Sep 2009 15:10
Because writing your own message pump is very suspect. The chances of making a serious
error are very high, and as you observed, you have an error. The correct approach to this
problem is to use a UI thread, not hand-create a message pump in a worker thread.

The issue that can arise is a synchronization error in the lifetime of the local objects
in the per-thread message map. Writing your own code has several nasty implications,
including the fact that you are not doing any cleanup of temporary MFC objects that might
be created. So you can develop storage leaks.

Bottom line: if you want to handle sockets in a separate thread, use a UI thread, don't
try to simulate what you think the message pump might be doing (in your case, you got it
wrong, and as long as you leave that code there, the message pump is wrong, because it
does not match the expected behavior of an MFC message pump)

It is a negative comment because the code is wrong. You may think it is working, but in
fact it is missing critical pieces which the MFC environment expects.
joe

On Mon, 21 Sep 2009 11:11:12 -0700 (PDT), neilsolent <n(a)solenttechnology.co.uk> wrote:

>
>> I'm wondering if you wanted to run an async socket in a separate thread, and this is your
>> code for doing so. �If so, it is highly suspect code. �The correct way to run a socket in
>> a secondary thread is to create a UI thread to handle it; in general, you can have high
>> confidence that if you are writing a message pump in MFC, you are doing something very
>> wrong.
>> � � � � � � � � � � � � � � � � joe
>
>Yes I am running an async socket in a "separate" thread. Why is it
>highly suspect code?
>This code now works fine. I think it is clear what the problem was and
>how I solved it from my posts.
>My app does not have a user interface.
>I don't mind people making negative remarks - but please give a proper
>explanation of why you think what I am doing won't work.
Joseph M. Newcomer [MVP]
email: newcomer(a)flounder.com
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm
From: Joseph M. Newcomer on 21 Sep 2009 15:11
The point is that at no time did you say "I am using a CAsyncSocket in a separate thread
and I wrote my own message pump", which is the crucial piece of information. Otherwise,
the expectation is that you have hit some error in the MFC runtime.
joe

On Mon, 21 Sep 2009 11:03:55 -0700 (PDT), neilsolent <n(a)solenttechnology.co.uk> wrote:

>On 21 Sep, 15:25, Joseph M. Newcomer <newco...(a)flounder.com> wrote:
>> I know you pointed out that it was line 2688. �But in WHAT FILE?
>
>The name of the file is of no importance. The stack trace refers to a
>line 2688. Hence I pointed to the line 2688 of my code.
>
>> This is clearly not part
>> of MFC, and therefore, since you give the code completely out of context, it is
>> questionable what it is doing or even why it exists. �
>> If it is part of your code, you have said nothing that would suggest where it is or why it
>> exists.
>
>The context is as stated in the first post
Joseph M. Newcomer [MVP]
email: newcomer(a)flounder.com
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm
From: neilsolent on 21 Sep 2009 16:06
On 21 Sep, 19:37, Stephen Myers <""StephenMyers
\"@discussi...(a)microsoft.com"> wrote:
> neilsolent wrote:
> >> I'm wondering if you wanted to run an async socket in a separate thread, and this is your
> >> code for doing so.  If so, it is highly suspect code.  The correct way to run a socket in
> >> a secondary thread is to create a UI thread to handle it; in general, you can have high
> >> confidence that if you are writing a message pump in MFC, you are doing something very
> >> wrong.
> >>                                 joe
>
> > Yes I am running an async socket in a "separate" thread. Why is it
> > highly suspect code?
> > This code now works fine. I think it is clear what the problem was and
> > how I solved it from my posts.
> > My app does not have a user interface.
> > I don't mind people making negative remarks - but please give a proper
> > explanation of why you think what I am doing won't work.
>
> The UI thread design supplies the message pump for you.  The name is
> unfortunate.  If you replace the name UI Thread with Thread with message
> pump it helps.
>
> The biggest problem with the code is that you seem to be doing it the
> hard way.  I would use the OnClose() override to delete the socket.  If
> you need to close the socket at other times, Close() is easy.  Using a
> UI thread leaves you with a small amount of code in InitInstance and the
>   a few message handlers.
>
> Steve

OK. The issue here was with the timing of deleting a CAsyncSocket-
derived object. How does the UI thread model handle that any better?
At what point would you delete the object in the UI thread?
From: Joseph M. Newcomer on 21 Sep 2009 16:59
There will be a call on either OnDisconnect (indicating the remote site had disconnected)
or OnClose (incidating the socket has been closed). Only after the OnClose can you be
sure that the socket is no longer required, and it can be deleted. You did not establish
the basis for how you closed the socket, and therefore your code was clearly dispatching
to the (now nonexistent) window for the socket, hence the failure. Note that the
potential accumulation of garbage temporary objects remains a problem.

You have to show the ways in which you manipulate the socket; you cannot analyze a problem
like this just by showing a message pump. You showed a piece of code completely out of
context, with no explanation, so it was impossible to analyze what was there; had you
stated the problem (which should have been obvious once you looked at the stack backtrace
and saw what was going wrong) with the correct amount of detail, then the correct solution
would have been obvious.

Note that I had to rewrite the horrible Microsoft example for multithreaded sockets, which
got threading wrong, sockets wrong, and synchronization wrong, and I did this without
having to write a message pump.

If you do not understand how an MFC message pump works (not just an arbitrary message
pump; an MFC message pump) then trying to simulate it will produce erroneous results. If
your thread does not closely resemble CWinThread::Run and implement the same chain of
events, then it is incorrect. That OnIdle call is critical.

(Note: I have already proven your code is wrong; you cannot defend it based on the fact
that you like it, or think that it works, or once read about what a message pump in a
non-MFC app looked like in a book or example. It doesn't work. It presents the illusion
that it works. Delivering products based on illusory correctness is not a robust solution
strategy. Using a UI thread creates a correct solution, which actually IS correct.)
joe
On Mon, 21 Sep 2009 13:06:12 -0700 (PDT), neilsolent <n(a)solenttechnology.co.uk> wrote:

>On 21 Sep, 19:37, Stephen Myers <""StephenMyers
>\"@discussi...(a)microsoft.com"> wrote:
>> neilsolent wrote:
>> >> I'm wondering if you wanted to run an async socket in a separate thread, and this is your
>> >> code for doing so. �If so, it is highly suspect code. �The correct way to run a socket in
>> >> a secondary thread is to create a UI thread to handle it; in general, you can have high
>> >> confidence that if you are writing a message pump in MFC, you are doing something very
>> >> wrong.
>> >> � � � � � � � � � � � � � � � � joe
>>
>> > Yes I am running an async socket in a "separate" thread. Why is it
>> > highly suspect code?
>> > This code now works fine. I think it is clear what the problem was and
>> > how I solved it from my posts.
>> > My app does not have a user interface.
>> > I don't mind people making negative remarks - but please give a proper
>> > explanation of why you think what I am doing won't work.
>>
>> The UI thread design supplies the message pump for you. �The name is
>> unfortunate. �If you replace the name UI Thread with Thread with message
>> pump it helps.
>>
>> The biggest problem with the code is that you seem to be doing it the
>> hard way. �I would use the OnClose() override to delete the socket. �If
>> you need to close the socket at other times, Close() is easy. �Using a
>> UI thread leaves you with a small amount of code in InitInstance and the
>> � a few message handlers.
>>
>> Steve
>
>OK. The issue here was with the timing of deleting a CAsyncSocket-
>derived object. How does the UI thread model handle that any better?
>At what point would you delete the object in the UI thread?
Joseph M. Newcomer [MVP]
email: newcomer(a)flounder.com
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm
From: neilsolent on 22 Sep 2009 04:48
> There will be a call on either OnDisconnect (indicating the remote site had disconnected)
> or OnClose (incidating the socket has been closed).  Only after the OnClose can you be
> sure that the socket is no longer required, and it can be deleted.  You did not establish
> the basis for how you closed the socket, and therefore your code was clearly dispatching
> to the (now nonexistent) window for the socket, hence the failure.  Note that the
> potential accumulation of garbage temporary objects remains a problem.

I also need to close sockets on demand, rather than passively (in
response to an OnClose or whatever).
The peer may not want to close or disconnect, so I don't think I can
rely on the message coming in.

> You have to show the ways in which you manipulate the socket; you cannot analyze a problem
> like this just by showing a message pump.  You showed a piece of code completely out of
> context, with no explanation, so it was impossible to analyze what was there; had you
> stated the problem (which should have been obvious once you looked at the stack backtrace
> and saw what was going wrong) with the correct amount of detail, then the correct solution
> would have been obvious.

Well, it's always obvious when you aleady have been given the answer.
Without the answer, I didn't know which part of the code was
important.

> If you do not understand how an MFC message pump works (not just an arbitrary message
> pump; an MFC message pump) then trying to simulate it will produce erroneous results.  If
> your thread does not closely resemble CWinThread::Run and implement the same chain of
> events, then it is incorrect.  That OnIdle call is critical.  
>
> (Note: I have already proven your code is wrong; you cannot defend it based on the fact
> that you like it, or think that it works, or once read about what a message pump in a
> non-MFC app looked like in a book or example.  It doesn't work.  It presents the illusion
> that it works.  Delivering products based on illusory correctness is not a robust solution
> strategy.  Using a UI thread creates a correct solution, which actually IS correct.)
>                                         joe
>

How did you prove the code is wrong?
First  |  Prev  |  Next  |  Last
Pages: 1 2 3 4 5
Prev: Knit picking
Next: No source on specified on that location