Re: A problem of UDP
- From: Joseph M. Newcomer <newcomer@xxxxxxxxxxxx>
- Date: Fri, 30 May 2008 10:45:35 -0400
See below...
On Thu, 29 May 2008 20:32:37 -0700 (PDT), Bruce Hsu <ccbruce@xxxxxxxxx> wrote:
I am writing a program for chating with another user in LAN. This****
program uses UDP to send messages.
Probably an exceptionally poor decision. UDP is ill-suited for this purpose. A very
superficial anlysis suggests it makes sense, but in fact it is almost always the wrong
choice, because it is fundamentally not reliable.
****
****
When I sent a message to another peer who has already closed his
program, the last message of the peer will be sent to me. I am sure
the message is sent by peer's computer. (Because unplug peer's line
will stop this problem and will reproduce this problem after re-plug
peer's line.)
Don't use UDP.
****
****
Can anyone tell me why this problem occurs and how to prevent this
message to be sent?
Use TCP/IP. It makes much more sense
****
****
----------------------------------
Here is my code to start a socket (At begin of program):
CChating::CChating() : m_socket(NULL), m_pooling(NULL), m_stop(false),
m_buf(NULL), m_wnd(NULL), m_lenbuf(0)
{
DECLARE_EXCEPTION_SCOPE(CChating::CChating, true)
WSADATA wd={0};
int i=0;
i=WSAStartup(MAKEWORD(2, 2), &wd);
Why are you doing low-level WinSock programming? This is a bad choice. Use CAsyncSocket
****
ON_FAIL(i==0,****
MMSG("Winsock can not be initialized. (%d)", i),
Using 8-bit characters is quaint, but not good practice in 2008
****
CChating_CChating_Fail);****
m_socket=socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP);
ON_WSA_FAIL(m_socket!=INVALID_SOCKET,
socket, "Fail to create socket.",
CChating_CChating_Fail);
i=-1;
ON_WSA_FAIL(setsockopt(m_socket, SOL_SOCKET, SO_BROADCAST, (char
*)&i, sizeof(int))!=SOCKET_ERROR,
setsockopt, "Fail to enable broadcasting.",
CChating_CChating_Fail);
i=sizeof(m_lenbuf);
ON_WSA_FAIL(getsockopt(m_socket, SOL_SOCKET, SO_RCVBUF, (char
*)&m_lenbuf, &i)!=SOCKET_ERROR,
ioctlsocket, "Fail to determine max buffer size.",
CChating_CChating_Fail);
m_stop=false;
m_pooling=CreateThread(NULL, 0, pooling_func, this, 0, NULL);
This is incorrect code. If you are using MFC, you MUST use AfxBeginThread; if you are
using the C runtimes, you MUST use _beginthread/_beginthreadex
****
ON_WIN32_FAIL(m_pooling,****
CreateThread, "Fail to start pooling.",
CChating_CChating_Fail);
ON_FAIL(WaitForSingleObject(m_pooling, 1000)==WAIT_TIMEOUT,
"The pooling thread has abnormally terminated.",
CChating_CChating_Fail);
This is very poor for several reasons. For one thing, it is blocking the main program
thread, so you should not see any correct behavior. Use asynchronous sockets. You won't
need a thread.
I have no idea what your ON_FAIL macro does, but WFSO can return codes other than
WAIT_TIMEOUT that have nothing to do with the thread being abnormally terminated
*****
****
m_buf=(PBYTE)malloc(m_lenbuf);
ON_CRT_FAIL(m_buf,
malloc, "Fail to alloc receving buffer.",
CChating_CChating_Fail);
CoCreateGuid((GUID *)m_buf);
gethostname((char *)&m_buf[sizeof(GUID)], 256);
m_me=peer_t(*(GUID *)m_buf, M2W((char *)&m_buf[sizeof(GUID)]),
L"0.0.0.0");
return;
CChating_CChating_Fail:
This suggests a goto. This is bad style. If you want to open and close the socket no
matter what, why are you not using two functions: one to open, then close the socket, and
which calls the function that returns values. This is not a good structure that you have
here. It also means that you are doing synchronous socket communication, which is a
really poor idea
****
closesocket(m_socket);****
WSACleanup();
EXCEPTION_HANDLER_END()
}
----------------------------------
Here is my code to end a socket (At end of program):
CChating::~CChating()
{
DECLARE_DBG_SCOPE(CChating::~CChating, true)
DWORD i=0;
DBG(_T("Pre-exit.\n"));
m_stop=true;
if(WaitForSingleObject(m_pooling, 2000)==WAIT_TIMEOUT)
TerminateThread(m_pooling, -1);
This is wrong because you shouldn't be stoppping threads in a destructor. This happens
very, very late in the process of shutdown, and the thread shutdown should happen during
the OnClose handler.
****
****
DBG(_T("Post-exit.\n"));
free(m_buf);
shutdown(m_socket, SD_BOTH);
closesocket(m_socket);
WSACleanup();
}
----------------------------------
Here is my code to wait messages (At a pooling thread):
DWORD __stdcall CChating::pooling_func(LPVOID data)
{
DECLARE_EXCEPTION_SCOPE(CChating::pooling_func, true)
CChating *me=(CChating *)data;
SOCKET socket=me->m_socket;
bool &stop=me->m_stop;
PBYTE &buf=me->m_buf;
HWND &wnd=me->m_wnd;
int &lenbuf=me->m_lenbuf;
peer_t &_me=me->m_me;
DWORD i=0, j;
packet_t *packet=NULL;
fd_set fs={0};
timeval tv={1, 0};
sockaddr_in sa={0};
me->init_sockaddr_in(sa, htonl(0));
ON_WSA_FAIL(bind(socket, (sockaddr *)&sa, sizeof(sockaddr_in))!
=SOCKET_ERROR,
bind, "Fail to bind address.",
CChating_pooling_func_Fail);
while(!stop)
{
FD_ZERO(&fs);
FD_SET(socket, &fs);
i=(DWORD)select(0, &fs, NULL, NULL, &tv);
DBG(L"The result of select: %x\n", i);
ON_WSA_FAIL((int)i!=SOCKET_ERROR,
select, "Fail to determine the status of socket.",
CChating_pooling_func_Fail);
if(!stop && (int)i)
{
*(int *)&i=sizeof(sa);
j=recvfrom(socket, (char *)buf, lenbuf, 0, (sockaddr *)&sa, (int
*)&i);
packet=(packet_t *)buf;
wcscpy(packet->m_peer.m_addr, M2W(inet_ntoa(sa.sin_addr)));
switch(packet->m_mark)
{
case GETPEER_MARK:
if(!(_me==packet->m_peer))
{
DBG(_T("GETPEER_MARK, %s, %s\n"), packet->m_peer.m_name, packet-
m_peer.m_addr);
if((bool)SendMessage(wnd, WM_GOTPEER, 0, (LPARAM)&packet->m_id))
me->peer_op(sa.sin_addr.S_un.S_addr, packet->m_mark-1, packet-
m_id);
SendMessage is a poor choice for interthread communication. Use PostMessage. You are
trying to impose sequential behavior on an asynchronous structure, always a bad idea. The
result will be what will quickly become an unmaintainable mess.
*NEVER* use WM_ for a user-defined message. This leads to confusion. Consider WM_ a
reserved prefix to Microsoft and do not use it yourself.
****
}****
break;
case GETPEER_MARK-1:
DBG(_T("[ACK] GETPEER_MARK, %s, %s\n"), packet->m_peer.m_name,
packet->m_peer.m_addr);
SendMessage(wnd, WM_GOTPEER, 0, (LPARAM)&packet->m_id);
Use PostMessage
****
break;****
case REMOVE_MARK:
DBG(_T("REMOVE_MARK, %s, %s\n"), packet->m_peer.m_name, packet-
m_peer.m_addr);
SendMessage(wnd, WM_RMPEER, 0, (LPARAM)&packet->m_id);
break;
case INVITE_MARK:
DBG(_T("INVITE_MARK, %s, %s\n"), packet->m_peer.m_name, packet-
m_peer.m_addr);
if((bool)SendMessage(wnd, WM_INVITED, 0, (LPARAM)&packet->m_id))
me->peer_op(htonl(-1), GETPEER_MARK, packet->m_id);
break;
default:
DBG(_T("MESSAGE, %s, %s, %s\n"), packet->m_peer.m_name, packet-
m_peer.m_addr, packet->m_msg);SendMessage(wnd, WM_GOTMSG, 0, (LPARAM)&packet->m_id);
}
}
}
me->peer_op(htonl(-1), REMOVE_MARK, GUID_NULL);
DBG(_T("Pooling thread is exiting.\n"));
EXCEPTION_HANDLER_BEGIN(CChating_pooling_func_Fail, 0)
DBG(_T("Pooling thread will not continue due to the following
problems.\n"));
DBG(_T("%s\n"), M2T((char const *)*___e));
delete ___e;
return -1;
}
This represents among the worst possible ways to program sockets and threads. Rewrite
this code using TCP/IP and CAsyncSocket. As an example of socket programming, I give this
an "F". As an example of threading, I give this an "F". As an example of good
programming style, I give this a D-. I know people have to start somewhere, but it is a
good thing to learn that the somewhere you are is not a somewhere anyone wants to be.
There are chat examples in the MSDN which are much better. Start with one of those.
joe
****
Joseph M. Newcomer [MVP]
email: newcomer@xxxxxxxxxxxx
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm
.
- Prev by Date: Re: How to handle tab notifications in CTabCtrl subclass?
- Next by Date: Re: a question 'bout const (design)
- Previous by thread: a question 'bout const (design)
- Next by thread: What is problem event BEX?
- Index(es):
Relevant Pages
|
