Re: Problem with socket
- From: Joseph M. Newcomer <newcomer@xxxxxxxxxxxx>
- Date: Tue, 31 May 2005 12:58:50 -0400
Be aware that those port numbers are part of the IANA-assigned range. Take a look at
http://www.iana.org/assignments/port-numbers
which is the list of assigned port numbers. If you are using any port number in the range
3312-4000, you run the risk of conflict with any of the hundreds of applications which
already have one of those port numbers legitimately assigned. If you need a private port
number range for your own purposes, you must allocate if from the range 49152 (0xC000)
through 65535 (0xFFFF).
Also, you appear to be running with synchronous sockets, always a bad idea. In addition,
you have apparently built what is known as an iterative server, which means it can have no
concurrency. This is not the best of structures.
Given you have synchronous sockets, can you explain how you are going to shut this
application down correctly? Particularly given that hundreds of threads will be blocked in
socket operations on sockets for which there are no handles...
I presume that 8 is a really important number for the listen; if you want maximum, you
would have specified SOMAXCONN as the value, which is what most people choose. Can you
justify why you think "8" is the correct number to use?
Presumably you have some way to wait for all these ports; the only way to do this with
synchronous sockets is to spin off 688 threads all of which block on an accept statement.
This strikes me as a Reall Bad Idea. But what I see is that you are creating 688 sockets
and throwing away the handles to all but the last one, which is the only one you listen
on. I'm sure this is not what you intended. I see no thread creation for listener threads.
Your code for receiving a message is erroneous. It reads 100 bytes, across a TCP/IP
socket. This means that unless every message is *exactly* 100 bytes in length, you will be
reading either part of a longer message, or a shorter message and some or part of
subsequent messages. Note that TCP/IP is a *stream* protocol, which means the quantity
bytes you get in a recv is completely unrelated to the quantity of bytes in corresponding
send operations; if you send ten 10-byte messages, you might get one recv that receives
all ten of them. If you send 64-byte messages, the recv will likely receive the first 64
bytes of the first message and the first 36 bytes of the next message. I see nothing in
this code that is going to sort out the messages.
Finally, having read some, all, or part, or some parts of some, messages on the socket
(you have no idea which of these applies!) you set the socket handle to NULL then call
closesocket passing a NULL handle in. Of course, the socket you have remains open, and if
you are unlucky, the closesocket does not crash your app with an access fault or invalid
handle error. Since you don't check the return value of closesocket, you have no idea if
it worked. It certainly didn't do what you intended.
The indentation is so poor that I can't figure out what the flow of control actually is. I
make it a policy that every { has a comment following it and every } has the matching
comment, making code easy to read.
As a style issue, never, ever use a comma in a declaration list. If you need to declare
three variables, you take three lines to do it. A sane language would not even have a
construct that allowed a comma in a declaration list. The result of using comma lists is
code that is hard to read and hard to maintain. It also appears you are declaring int
variables like "flag" with numeric values like "1" assigned. Are you sure you did not
intend to declare variables of type BOOL with values like "TRUE" attached? Never use an
int to represent a boolean value. It makes your code nearly impossible to decrypt.
Encrypted code is not maintainable. And why are you initializing s to 1 and then
immediately assigning s = 1? This seems pointless.
It is a very, very, very good idea to put a space after each comma in a parameter list. It
makes the code readable.
I see a lot of use of "global" variables, in the sense you are referencing variables which
are in the class (a variable in a class used by lots of functions is still a global
variable from a structural viewpoint), and apparently shared among all 688 threads. There
is no attempt to manage synchronization, or even coherency, among what I see are a lot of
concurrent uses. Most of these should be local variables anyway. Local to the nonexistent
thread...
In addition, you have used the completely meaningless word "crash" to describe your
problem. There is no such thing as "a crash". There is always a very specific message that
comes out that contains a lot of important information describing what went wrong. Without
that critical information, the word "crash" is a meaningless word; you might have well
have said "my application framboozles" for all the information that is conveyed.
I have no idea what you really intended, but this code needs a complete rewrite before it
even begins to make sense. Bottom line: there's nothing wrong with this code that a
complete rewrite will not solve.
joe
On Mon, 30 May 2005 16:44:03 +0530, "Ashutosh" <efextra@xxxxxxxxxxxxxxxxx> wrote:
>hi
>
>i am using SOCKET data type for socket connecttion
>
>i am binding the socket on any port available betwneen 3312 -4000
>
>and start listening successfully but on the accept() command my program get
>crashed.
>
>the code is given below:
>
>here m_pSocket and msgsock both are SOCKET type
>
>UINT Bthread(LPVOID p)
>{
> CBufferSocket *dlg=static_cast<CBufferSocket *>(p);
> TRY
> {
>
> int iPort;
> BOOL bPort=FALSE;
>
> // selecting the port
> for(iPort=3312;iPort<4000;iPort++)
> {
> dlg->serv.sin_addr.s_addr=htonl(INADDR_ANY);
> dlg->serv.sin_family=AF_INET; // standard connect
> dlg->serv.sin_port=htons(iPort);
> dlg->addlen=sizeof(dlg->serv);
> dlg->m_pSocket=socket(AF_INET,SOCK_STREAM,0);
> if(bind(dlg->m_pSocket,(sockaddr*)&dlg->serv,dlg->addlen)!=SOCKET_ERROR)
> {
> bPort=TRUE;
> dlg->m_iPort=iPort;
> break;
> }
> else
> dlg->m_pSocket=NULL;
> }
>
> if(bPort==TRUE)
> {
> listen(dlg->m_pSocket,8);
> char buff[1024];
> int s=1,loop=1,flag=0;
> s=1;
> dlg->msgsock=NULL;
> if(dlg->m_pSocket!=NULL)
>
>dlg->msgsock=accept(dlg->m_pSocket,(sockaddr*)&(dlg->serv),&(dlg->addlen));
>// **************** Point of crash *******************
> else
> AfxMessageBox("Con not accepted");
>
> if (dlg->msgsock==INVALID_SOCKET)
> {
> AfxMessageBox("Invalid accepte");
> }
> else
> {
> AfxMessageBox("before accepte");
> // Begin Connection with new command information
> while(s!=SOCKET_ERROR)
> {
> s=recv(dlg->msgsock,buff,100,0);
> //take appropriate actions on the receiving the command
>
> if (s!=SOCKET_ERROR)
> {
>
> // store the command for the execution
> AfxMessageBox(buff);
> dlg->WriteText(buff);
> }
>
> }
>
> // Set Connection free to accept the other connection
> dlg->msgsock=NULL;
> closesocket(dlg->msgsock);
> }
> dlg->SaveFile();
> // finally destroy the connection
> AfxEndThread(0);
> }
> }
> CATCH(CException, ex)
> {
> CString str;
> str.Format("Error : %d",ex->ReportError());
> }
> END_CATCH
>
>
>
> return 0;
>
>}
>
>
>my program always crash at this point:
>dlg->msgsock=accept(dlg->m_pSocket,(sockaddr*)&(dlg->serv),&(dlg->addlen));
>
>any one can give me any idea why this crash ?
>
>please given any solution ...
>
>please help me..
>
>Ashutosh
>
Joseph M. Newcomer [MVP]
email: newcomer@xxxxxxxxxxxx
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm
.
- References:
- Problem with socket
- From: Ashutosh
- Problem with socket
- Prev by Date: Looking for a charting control
- Next by Date: Re: CListCtrl item text string length limitation
- Previous by thread: RE: Problem with socket
- Next by thread: How to do this?-Dialogs in Dll
- Index(es):
Relevant Pages
|
