Re: Using CWinThread in MFC apps

Tech-Archive recommends: Fix windows errors by optimizing your registry

You cannot put socket handlers in a worker thread. See my essay on multithreaded socket
handlers.

More below...

On Mon, 3 Aug 2009 15:28:56 +0800, "Jack" <jl@xxxxxxxxxx> wrote:

Thanks guys for the help..
I ran into another problem now.
My CSocket::OnReceive is never called.
I searched the net and some website suggested
that I was stucked in a loop instead of the message pump
I looked here and there for a couple of hours but still
couldn't find the bug...
Could you please help?

[code]
// Socket.cpp : implementation file

//

#include "stdafx.h"

#include "CDemo.h"

#include "Socket.h"

#include ".\socket.h"

class CCDemoDoc;

UINT __cdecl Sock_Handler(LPVOID arg);

SOCKET hConnected;
****
This instantly says "SERIOUS DESIGN ERROR!" There should be no global variables used for
this purpose.
****

// cSocket

cSocket::cSocket()

{

OutputDebugString("cSocket::cSocket\n");

}

cSocket::cSocket(CCDemoDoc *pDoc)

{

m_Doc = pDoc;

}

cSocket::~cSocket()

{

OutputDebugString("cSocket::~cSocket\n");

}



// cSocket member functions

void cSocket::OnAccept(int nErrorCode)

{

OutputDebugString("cSocket::OnAccept\n");

//Accept(this);

Attach(hConnected);
****
Why are you doing this? It makes no sense.
****

// TODO: Add your specialized code here and/or call the base class

AfxBeginThread(Sock_Handler, this);
*****
You cannot put sockets in a worker thread. You must use a UI thread.
*****

CSocket::OnAccept(nErrorCode);

}

void cSocket::OnClose(int nErrorCode)

{

OutputDebugString("cSocket::OnClose\n");

// TODO: Add your specialized code here and/or call the base class

CSocket::OnClose(nErrorCode);

}

void cSocket::OnConnect(int nErrorCode)

{

OutputDebugString("cSocket::OnConnect\n");

// TODO: Add your specialized code here and/or call the base class

CSocket::OnConnect(nErrorCode);

}

void cSocket::OnOutOfBandData(int nErrorCode)

{

OutputDebugString("cSocket::OnOutOfBandData\n");

// TODO: Add your specialized code here and/or call the base class

CSocket::OnOutOfBandData(nErrorCode);

}

void cSocket::OnReceive(int nErrorCode)

{

OutputDebugString("cSocket::OnReceive\n");


char buff[4096];

int nRead;

nRead = Receive(buff, 4096);

if (nRead != SOCKET_ERROR)

{

if (nRead)
****
I presume you mean
if(nRead != 0)
****

{

buff[nRead] = 0;
****
If you read 4096 bytes, this puts the 0 in the 4097th position of the 4096-long array.
Note that you must receive sizeof(buff)-1 (do NOT repeat the constant in two places; use
sizeof or use a #define or const UINT to declare the size)
****

CString szTemp(buff);
****
Note that if you are a Unicode app, this will create a Unicode string. Note that this
will not work properly if the string is encoded in UTF-8, which is the recommended best
practice.
****

// ((CMsocsDlg*)m_pDlg)->m_strRecv += szTemp;

// ((CMsocsDlg*)m_pDlg)->UpdateData(FALSE);
****
The above lines would ALWAYS be erroneous. You CANNOT call UpdateData from a secondary
thread, worker or UI. This would be a serious mistake, and these lines should simply be
removed from the code lest there be a temptation to uncomment them.
****

// if (szTemp.CompareNoCase("bye") == 0 ) ShutDown();
****
Why would you expect this to EVER work under ANY conditions imaginable? If you are
receiving 4096 bytes, you might well get

This is a test message\nThis is another message\nThis is yet another message\nbye

There is NO CORRELATION between the data packet size sent by a Send and the number of
bytes received by a Receive. The only guarantee you have is that the Receive will
eventually receive all the bytes sent by a successful Send, with no duplicates, in order,
with no missing data. Otherwise, you can send

This
is
a
test\n
This
is
another\n

and your first Receive might return

This is a test\nThis

and the next Receive will receive
is another\n

Or you might send

This is a test\nThis is another\n

and receive

This is
followed by
a test\nThis is another\n

Any delusion that there will in any way whatsoever be a correlation between the length of
packets sent and those received is exactly that. It is your responsibility, if the
packets have some kind of structure, to reassemble them into something that has meaning.
joe
****

}

else Close();

}

else

{

wsprintf (buff, "Error number is %d", GetLastError());
****
You should safely assume that sprintf, wsprintf, etc. are meaningless noise, and should
NEVER be used in practice. There is no excuse for using it here! The correct code would
be

CString msg;
msg.Format(_T("Error number is %d"), GetLastError());
****

AfxMessageBox (buff);

}


CSocket::OnReceive(nErrorCode);

// TODO: Add your specialized code here and/or call the base class


}

void cSocket::OnSend(int nErrorCode)

{

OutputDebugString("cSocket::OnSend\n");

// TODO: Add your specialized code here and/or call the base class

CSocket::OnSend(nErrorCode);

}

int cSocket::Receive(void* lpBuf, int nBufLen, int nFlags)

{

OutputDebugString("cSocket::Receive\n");

// TODO: Add your specialized code here and/or call the base class

return CSocket::Receive(lpBuf, nBufLen, nFlags);

}

int cSocket::Send(const void* lpBuf, int nBufLen, int nFlags)

{

OutputDebugString("cSocket::Send\n");

// TODO: Add your specialized code here and/or call the base class

return CSocket::Send(lpBuf, nBufLen, nFlags);

}

BOOL cSocket::OnMessagePending()

{

OutputDebugString("cSocket::OnMessagePending\n");

// TODO: Add your specialized code here and/or call the base class

return CSocket::OnMessagePending();

}

//////////////////////////////////////

UINT __cdecl Sock_Handler(LPVOID arg)

{

cSocket *pSock = (cSocket*) arg;

while(true)

{

if (!pSock->buff.empty())
****
I have no idea what this is supposed to be doing, but it is safe to assume that this code
is erroneous. I can't imagine what value this has.

Given you have a socket in a UI thread, this is deeply erroneous, since there is no
possible way the buffer can ever be set.

In a UI thread, there would never be a reason to test the buffer size like this; the
OnReceive handler would be responsible for creating the packet and sending it to the
receiving thread, and it would not make sense to put that code anywhere else.
****

{

// pSock->CopyDataToPacket();

pSock->buff.clear();

}


Sleep(500);
****
First rule: any thread that has a Sleep in it is wrong.
Second rule: if you think this is right, consult the first rule
Third rule: The exceptions are amazingly rare and apply only under fairly exotic
conditions, which this is not an instance of.

Therefore, you can safely assume that putting a Sleep in a thread is always a mistake.
Note that a thread must work *correctly* without any Sleep() calls.
joe
****


}

return 0;

}


[/code]

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

Relevant Pages

  • Re: Using CWinThread in MFC apps
    ... void cSocket::OnAccept(int nErrorCode) ... int cSocket::Receive ... int cSocket::Send(const void* lpBuf, int nBufLen, int nFlags) ...
    (microsoft.public.vc.mfc)
  • Re: Using CWinThread in MFC apps
    ... void cSocket::OnAccept(int nErrorCode) ... int cSocket::Receive ... int cSocket::Send(const void* lpBuf, int nBufLen, int nFlags) ...
    (microsoft.public.vc.mfc)
  • Re: Sockets causing deadlock
    ... void CALLBACK ReceiveCallback(IN DWORD dwError, IN DWORD cbTransferred, ... // If we don't have a socket, ... int nError; ...
    (microsoft.public.win32.programmer.networks)
  • Socket.Send is slow when sending within same process
    ... static void Main ... Socket socket = new Socket(AddressFamily.InterNetwork, ... int completed = 0; ... Put this code in a console app called Receive ...
    (microsoft.public.dotnet.languages.csharp)
  • Re: Socket mit Network-Byte-Order (Motorola-Format)- Client und Server
    ... int _port; ... public string SendRequest{ ... void Disconnect{ ... Socket serverSocket = new Socket( ...
    (microsoft.public.de.german.entwickler.dotnet.csharp)