repeated connecting and disconnecting fails

Started by
10 comments, last by Crispy 21 years, 4 months ago
I finally got my client-server app working. Yay. It works fine - data is sent back and forth and everything works just peachy. When the client disconnects from the server, the listener on the client end is terminated and re-created when the client reconnects. After several times of connecting and reconnecting, however, listen() gives an WSAEINVAL error (on the client side which) . Reading the docs tells me that in that case bind() probably failed, but bind() doesn''t produce any errors. Here''s the code - this is the first time I''m messing around with network stuff and I''m trying to get by without anybody else''s code, so I''d appreciate some input regarding the correctness of it:
  
bool TTCPListener::Initialize(int pPort, TTCPSocket * pTCPSocket)
{
  TCPSocket = pTCPSocket;
  Port = pPort;

  //closing a closed socket doesn''t hurt - just in case it was open

  TCPSocket->Close();
  TCPSocket->Open(Port);   
  TCPThreadHandle = CreateThread(NULL, 0, (LPTHREAD_START_ROUTINE)TTCPListener::ControllingFunction, (LPVOID)this, 0, &TCPThreadId);

  return !TCPThreadHandle ? false : true;
}

//static

void TTCPListener::ControllingFunction(TTCPListener * pThis)
{
  pThis->TCPSocket->Receive();
}

bool TTCPListener::Terminate()
{
  DWORD ThreadExitCode;

  if(!GetExitCodeThread(TCPThreadHandle, &ThreadExitCode))
    ShowError("TTCPListener::Terminate()->GetExitThreadCode()");

  TerminateThread(TCPThreadHandle, ThreadExitCode);
  CloseHandle(TCPThreadHandle);
   
  TCPSocket->Close();   
}







void TTCPSocket::Receive()
{
 char m[255];
 memset(m, 0, 255);

 while(1)
  {
  if((SocketToAccept = accept(SocketToListen, (sockaddr*)&RemoteAddress, &AddressSize)) == INVALID_SOCKET)
    {
    VERIFY_WSA("TTCPSocket::Receive()->accept()");
    Sleep(1000);
    break;
    }

  if(recv(SocketToAccept, m, 256, 0) == SOCKET_ERROR)
    VERIFY_WSA("TTCPSocket::Receive()->recv()");
  else
    {
    //send the message to the application

    SocketCallback(m, RemoteAddress);
    memset(&RemoteAddress, 0, sizeof(RemoteAddress));
    }
  }
}

void TTCPSocket::Close()
{
  closesocket(SocketToListen);
  closesocket(SocketToAccept);
}

HRESULT TTCPSocket::Open(int pPort)
{
  memset(&RemoteAddress, 0, sizeof(RemoteAddress));
  memset(&LocalAddress, 0, sizeof(LocalAddress));
  LocalAddress.sin_family = AF_INET;
  LocalAddress.sin_addr.s_addr = htonl(INADDR_ANY);
  LocalAddress.sin_port = htons((unsigned short)pPort);

  if((SocketToListen = socket(AF_INET, SOCK_STREAM, 0)) == INVALID_SOCKET)
    VERIFY_WSA("TTCPSocket::Open()->socket()");

  if(bind(SocketToListen, (sockaddr*)&LocalAddress, sizeof(LocalAddress)) == SOCKET_ERROR)
    VERIFY_WSA("TTCPSocket::Open()->bind()");

  if(listen(SocketToListen, SOMAXCONN) == SOCKET_ERROR)
    VERIFY_WSA("TTCPSocket::Open()->listen()");
  //the WSAEINVAL error occurs here


  AddressSize = sizeof(RemoteAddress);
}
  
Any ideas why that is so? Is there some latency related to sockets so I can''t go about opening ang closing them as often as I want? Crispy
"Literally, it means that Bob is everything you can think of, but not dead; i.e., Bob is a purple-spotted, yellow-striped bumblebee/dragon/pterodactyl hybrid with a voracious addiction to Twix candy bars, but not dead."- kSquared
Advertisement
quote:Original post by Crispy
When the client disconnects from the server, the listener on the client end is terminated and re-created when the client reconnects.


Why are you creating a listener on the client-side? The listen() function should only be called by the server.

What does VERIFY_WSA look like in your code?





[edited by - Digitalfiend on December 4, 2002 10:51:40 AM]
[email=direwolf@digitalfiends.com]Dire Wolf[/email]
www.digitalfiends.com
The client also receives info from other clients. The server is more like a central server - everything is done through it, and since clients can''t connect to each other directly, they are, in fact, called "clients".


  void VERIFY_WSA(const char * error_code, UINT icon){  icon = icon | MB_OK;  char error_msg[255];  switch(WSAGetLastError())   {   case(WSANOTINITIALISED):     strcpy(error_msg, "WSA not initialized (WSANOTINITIALISED)");     if(strlen(error_code) > 0)        AppendWSAString(error_msg, error_code);     MessageBox(NULL, error_msg, "Error", icon);     break;     ...     //cases for all WSA errors     ...   }}  


Crispy
"Literally, it means that Bob is everything you can think of, but not dead; i.e., Bob is a purple-spotted, yellow-striped bumblebee/dragon/pterodactyl hybrid with a voracious addiction to Twix candy bars, but not dead."- kSquared
OK So on the client you have two sockets: one for connecting to the server and one for receiving connections from the server?

So the server can connect to the client even if the client isn't connected to the server?

----------------

First of all, never call TerminateThread. Threads should *always* exit gracefully from their threadproc. Calling TerminateThread can causes memory loss, handles to remain open, and in general is just bad-programming.

You are also not calling shutdown() on your accepted socket (the "client" socket.) While it isn't always necessary, it is a good habit to get into.

You should consider making the following changes to your implementation:


      include <process.h>  // for _beginthreadex   // also make sure you are binding to the *multithreaded* runtime library (either static or dynamic.)  // If you aren't, the compiler will complain that it can't// find _beginthreadex.   // add a private member variable to TTCPSocketprivate:    bool canShutdown;   bool TTCPListener::Initialize(int pPort, TTCPSocket * pTCPSocket){      TCPSocket = pTCPSocket;    Port = pPort;        //closing a closed socket doesn't hurt - just in case it was open      TCPSocket->Close();    TCPSocket->Open(Port);        // change CreateThread to _begindthreadex    TCPThreadHandle = (HANDLE)_beginthreadex(0,                                             0,                                              ControllingFunction,                                              (PVOID)this,                                             0,                                             &TCPThreadId);   return !TCPThreadHandle ? false : true;}  // static - change return typeunsigned int TTCPListener::ControllingFunction(TTCPListener * pThis){      return pThis->TCPSocket->Receive();}  void TTCPSocket::Terminate(unsigned long waitTimeout){    canShutdown = true;            }  // change return type to unsigned longunsigned int TTCPSocket::Receive(){     char m[255];     memset(m, 0, 255);       while(!canShutdown)    {        // your receive code goes here    }    // remember to cleanup your memory (sockets, buffers) here    Close();        // return some exit or status code    return exitCode;}  void TTCPSocket::Close(){      closesocket(SocketToListen);        // shutdown the socket connection    shutdown(SocketToAccept, 2);    closesocket(SocketToAccept);   }          


A few other things to investigate:

- Also, are you attempting to reuse socket handles?
Unless you are using Windows XP specific Winsock calls, make sure you always create a new socket everytime using socket/WSASocket.

- Are you accidently overwriting a buffer (buffer overflow) and wiping out the variable that holds your SOCKET handle? I've done this before and believe me it is a bitch of an error to find.

- If you are reusing TTCPSocket objects (like an object pool), look into thread synchronization issues. Make sure you aren't initializing the object while another thread is closing/cleaning up the object.

Hope this helps,

If it doesn't, feel free to send me the code and I'll see if I can find the problem (or just post more of it here.)


[edited by - Digitalfiend on December 4, 2002 11:45:19 AM]
[email=direwolf@digitalfiends.com]Dire Wolf[/email]
www.digitalfiends.com
Thanks! I just have a few questions for clarification if you don''t mind.

quote:Original post by Digitalfiend
So the server can connect to the client even if the client isn''t connected to the server?


No, technically no because the user on the client end specifies his/her port just before he connects to the server and the listener isn''t run before that at all - besides the server doesn''t know any clients'' IP''s before the clients connect to it.

quote:
First of all, never call TerminateThread. Threads should *always* exit gracefully from their threadproc. Calling TerminateThread can causes memory loss, handles to remain open, and in general is just bad-programming.


Understood and that''s what I figured, but EndThread used to crash the program - seems to be working fine now...

quote:
You are also not calling shutdown() on your accepted socket (the "client" socket.) While it isn''t always necessary, it is a good habit to get into.


Why are you only suggesting calling shutdown() for the listening socket, but not the accepting socket?

quote:
You should consider making the following changes to your implementation: ...


Why _beginthreadex() rather than CreateThread()? Furthermore - if I have multiple threads running simultaneously - how can I be sure which one _endthread() will shut down given it has no parameters?

quote:
- Also, are you attempting to reuse socket handles?
Unless you are using Windows XP specific Winsock calls, make sure you always create a new socket everytime using socket/WSASocket.


Nope. I''m creating one each time I create a new TTCPSocket or send a TCP message (by calling: socket()->connect()->send()->closesocket()->shutdown()).

quote:
- Are you accidently overwriting a buffer (buffer overflow) and wiping out the variable that holds your SOCKET handle? I''ve done this before and believe me it is a bitch of an error to find.


I''m afraid I don''t quite follow. Could you elaborate?

quote:
- If you are reusing TTCPSocket objects (like an object pool), look into thread synchronization issues. Make sure you aren''t initializing the object while another thread is closing/cleaning up the object.


Fortunately, I only have one TTCPSocket on either end - for listening only. Every time I need to send a messge I just create the socket, send the goods and kill it.

quote:
Hope this helps,


Yes, it does

Thanks for the input,
Crispy

"Literally, it means that Bob is everything you can think of, but not dead; i.e., Bob is a purple-spotted, yellow-striped bumblebee/dragon/pterodactyl hybrid with a voracious addiction to Twix candy bars, but not dead."- kSquared
quote:Original post by Crispy
No, technically no because the user on the client end specifies his/her port just before he connects to the server and the listener isn't run before that at all - besides the server doesn't know any clients' IP's before the clients connect to it.


So why does the server need to connect to the client? Maybe I'm just missing your design goal here

Here is how I envision your application:

Client-side:
------------

Socket #1 (TTCPSocket)
--- connects to server

Socket #2 (TTCPListener)
--- listens for connections from server

Server-Side:
------------

Socket #1 (TTCPListener)
--- listens for clients to connect

Socket #2...n
--- the "accepted" client sockets

Is this correct? If so, why does the client need a listening socket? Couldn't the client just keep the connection to the server open? The server could then send updates to the client.

quote:
Understood and that's what I figured, but EndThread used to crash the program - seems to be working fine now...


ExitThread() shouldn't cause your code to crash. There was probably some thread synchronization issues/bugs that were occurring.

quote:
Why are you only suggesting calling shutdown() for the listening socket, but not the accepting socket?


Other way around. You should call shutdown() on every client socket, i.e. the "accepted socket". An accepted socket represents the clients connection to the server. Client connections should always be closed with shutdown() and then closesocket().

quote:
Why _beginthreadex() rather than CreateThread()? Furthermore - if I have multiple threads running simultaneously - how can I be sure which one _endthread() will shut down given it has no parameters?


_beginthreadex should be used because it allows the runtime library to initialise thread-allocated storage and prepare itself for multithreaded use. You don't HAVE to use _beginthreadex but you should. There is no discernable performance difference. If you use CreateThread and call any of the C/C++ runtime library functions that use a global variable (like strtok) you are just asking for trouble...

_endthread()/ExitThread() will terminate the currently executing thread. So whatever thread hits/executes the _endthread/ExitThread function will be deallocated and terminated. Just know that the _endthread/ExitThread functions know what thread they are shutting down (they use various Win32 API functions.) You should never really need to call _endthread/ExitThread directly. When you "return" (i.e. exit) your thread procedure, Windows internally sets the thread's exit code based on your thread proc's return value:

DWORD MyClass::MyThreadProc(void* params){    return 1;}      


Say thread #2 executes MyThreadProc. As soon as MyThreadProc returns, Windows will cleanup the thread and assign MyThreadProc's return value (in this case 1) as the thread's exit code.

quote:
Nope. I'm creating one each time I create a new TTCPSocket or send a TCP message (by calling: socket()->connect()->send()->closesocket()->shutdown() ).


socket(), connect(), send(), closesocket(), shutdown()? You should reverse the closesocket() and shutdown() calls. The call to shutdown() should come first. This properly prepares the TCP/IP channel/connection for closure.

quote:
I'm afraid I don't quite follow. Could you elaborate?


Sure. Say you have a char buffer, 20 bytes long (each x represents a byte, the brackets represent the boundary):
(memory representation)

[xxxxxxxxxxxxxxxxxxxx]

Now lets say that you have a variable named, MySocketHandle. This variables address is stored after (well, nearly after) your buffer. This variable is 4 bytes long:

                               v MySocketHandle variable
[xxxxxxxxxxxxxxxxxxxx][ssss]

Now say you do a memcpy and pass in 23 bytes of data (lets not worry about memory alignment right now.)

[xxxxxxxxxxxxxxxxxxxx][xxxs]

You've now overwritten what is in MySocketHandle variable's memory address. This isn't good because MySocketHandle is likely a HANDLE variable, which in turn points to your process' handle table. Now your HANDLE variable, MySocketHandle, likely points to an invalid socket handle. So now, when you call send()/recv(), bind() and what not Winsock will report that the socket handle you are passing in is invalid.

Whew. Follow?

quote:
Fortunately, I only have one TTCPSocket on either end - for listening only. Every time I need to send a messge I just create the socket, send the goods and kill it.


Can you explain a little bit more of what you are trying to accomplish? Can you explain the architecture a bit more. It seems a little strange (no offense.)

Take care.


[edited by - Digitalfiend on December 4, 2002 6:13:31 PM]
[email=direwolf@digitalfiends.com]Dire Wolf[/email]
www.digitalfiends.com
Here''s a rundown of what the application(s) should do:

originally this was a simple school assignment - we had to create a netplay Tic Tac Toe game. Since the explanatory notes on the assignment card were way too scarce to actually make out if it was supposed to be p2p or through some dedicated server, I decided to escalate it to a higher level - add a GUI and create a central server which can hold any number of players playing any number of games: when a client connects to the server, it is added to a connections list, when the next one connects, a game is created and the two can play. For each pair of "unassigned" players, another game would be created and the players moved to a different list. The GUI provides a simple game field and a chatbox through which one player can chat with the other. AFAIU, I need to create listeners for each of the clients as well as the server since I need to both send and receive data on either end.

If it didn''t seem a bit strange I''d be surprised - I mean I''ve read no literature and the C course this thing is for is at the level of pointers and OO-ness at best...

quote:Original post by Digitalfiend
Client-side:
------------

Socket #1 (TTCPSocket)
--- connects to server

Socket #2 (TTCPListener)
--- listens for connections from server

Server-Side:
------------

Socket #1 (TTCPListener)
--- listens for clients to connect

Socket #2...n
--- the "accepted" client sockets


Is this correct? If so, why does the client need a listening socket? Couldn''t the client just keep the connection to the server open? The server could then send updates to the client.


Hmm - your "diagram" gives off an impression that you''re establishing a pipe-like connection between the two. I see TCP more like a "hit-and-run" kind of protocol - you open a socket, send the message, close the socket. On the other end you poll constantly and when you get something, parse it. So the division of sockets #1 and #2 doesn''t seem quite necessary because the socket through which data is transmitted is chosen randomly by socket() every time a message is sent. Right?


quote:
ExitThread() shouldn''t cause your code to crash. There was probably some thread synchronization issues/bugs that were occurring.


It killed the window, but left the thread running. ExitProcess() fixed this. As for _beginthreadex() - tried it, but haven''t had time to properly look it through.

quote:
Other way around. You should call shutdown() on every client socket, i.e. the "accepted socket". An accepted socket represents the clients connection to the server. Client connections should always be closed with shutdown() and then closesocket().


My bad - it was in your previous post

quote:
_beginthreadex should be used because it allows the runtime library to initialise thread-allocated storage and prepare itself for multithreaded use. You don''t HAVE to use _beginthreadex but you should. There is no discernable performance difference. If you use CreateThread and call any of the C/C++ runtime library functions that use a global variable (like strtok) you are just asking for trouble...


Mmmmm - conflicts - I love those. Haven''t had any yet - guess
I''ll migrate when the need comes. PS - on my BC5.02 simply including process.h was sufficient for _beginthreadex().

quote:
DWORD MyClass::MyThreadProc(void* params){    return 1;} 



Say thread #2 executes MyThreadProc. As soon as MyThreadProc returns, Windows will cleanup the thread and assign MyThreadProc''s return value (in this case 1) as the thread''s exit code.


Aha - got it now.

quote:
socket(), connect(), send(), closesocket(), shutdown()? You should reverse the closesocket() and shutdown() calls. The call to shutdown() should come first. This properly prepares the TCP/IP channel/connection for closure.


Again my bad - it was in your previous post


quote:
Sure. Say you have a char buffer, 20 bytes long (each x represents a byte, the brackets represent the boundary):
(memory representation)

[xxxxxxxxxxxxxxxxxxxx]

Now lets say that you have a variable named, MySocketHandle. This variables address is stored after (well, nearly after) your buffer. This variable is 4 bytes long:

                               v MySocketHandle variable
[xxxxxxxxxxxxxxxxxxxx][ssss]

Now say you do a memcpy and pass in 23 bytes of data (lets not worry about memory alignment right now.)

[xxxxxxxxxxxxxxxxxxxx][xxxs]

You''ve now overwritten what is in MySocketHandle variable''s memory address. This isn''t good because MySocketHandle is likely a HANDLE variable, which in turn points to your process'' handle table. Now your HANDLE variable, MySocketHandle, likely points to an invalid socket handle. So now, when you call send()/recv(), bind() and what not Winsock will report that the socket handle you are passing in is invalid.

Whew. Follow?


Okay - suppose this happens although I think it''s the compiler''s job to avoid things like these - what do I do to void it?

Crispy
"Literally, it means that Bob is everything you can think of, but not dead; i.e., Bob is a purple-spotted, yellow-striped bumblebee/dragon/pterodactyl hybrid with a voracious addiction to Twix candy bars, but not dead."- kSquared
quote:Original post by Crispy
So the division of sockets #1 and #2 doesn''t seem quite necessary because the socket through which data is transmitted is chosen randomly by socket() every time a message is sent. Right?


Could you rephrase your question?

Overall it seems you have a slightly skewed notion of the workings of sockets. Your way (the hit-and-run technique) certainly works, but seems unnecessarily complicated. I''m not criticizing your method, but simply maintaining a single connection between server and client might be a better solution. All sockets are two-way streets; machines on both ends may send and receive at will.

RapscallionGL - arriving soon.
________________________________________________"Optimal decisions, once made, do not need to be changed." - Robert Sedgewick, Algorithms in C
quote:Original post by johnnie2
Could you rephrase your question?

Overall it seems you have a slightly skewed notion of the workings of sockets. Your way (the hit-and-run technique) certainly works, but seems unnecessarily complicated. I''m not criticizing your method, but simply maintaining a single connection between server and client might be a better solution. All sockets are two-way streets; machines on both ends may send and receive at will.

RapscallionGL - arriving soon.


This is what I was trying to get at. Crispy, I think your problem is stemming from the overly complicated architecture of your implementation.

Here is how it should work, in my opinion:

- The client creates connects to the server.
- The server accepts the client.
- The server notifies the client that there are no other "players" available.
- The client keeps it connection open and waits for another player to connect.
- Another client connects to the server.
- The server accepts the next client.
- The server notifies the original waiting clients that there is a new client available.
- The first client chooses to play with the 2nd client.
- The server receives the "play with client 2" message and creates a "game"
- The server determines which client is X and which client is O and notes it in their "connection"

The clients can now interact with each other and playing the game by sending messages to the server.

- Client A sends message MarkGrid(0, 0)
- Server receives message and notes that MarkGrid(0,0) came from client 1, so the server marks the upper-left grid with an X.
- The server updates both clients on the change in the grid.
- The server notifies client 2 that it is his/her turn.
- etc

There is absolutely no need to constantly open and close TCP/IP connections. You are thinking HTTP. HTTP works off a connect/request/response/close architecture and does so for efficiency and scaling reasons (and is also partly why HTTP is stateless by nature.) Most games do not need this type of architecture because it is highly inefficient for games.

quote:Original post by Crispy
Hmm - your "diagram" gives off an impression that you''re establishing a pipe-like connection between the two. I see TCP more like a "hit-and-run" kind of protocol - you open a socket, send the message, close the socket. On the other end you poll constantly and when you get something, parse it. So the division of sockets #1 and #2 doesn''t seem quite necessary because the socket through which data is transmitted is chosen randomly by socket() every time a message is sent. Right?


No, the diagram I "drew" a few posts back was what I thought your architecture looked like; I wasn''t proposing you build your system like that.

Hopefully my comments above made sense.
[email=direwolf@digitalfiends.com]Dire Wolf[/email]
www.digitalfiends.com
Mmkay - so would this be the correct scheme (?):

1) Server has a listener and a pool of sockets
2) Client only has one socket with which it connects to the server
3) Whenever there''s an incoming request for a connection on the server side, a new socket is opened for conversation both ways.

A couple of questions, though:

1) If there is no listener on the client''s side, how does the client know when there''s an incoming message? There still has to be a separate thread to poll the listening socket (which in turn constitutes a listener), right?
2) Is the socket automatically closed on the other end if, for exaple, the server shuts down for no too apparent reason (Windows crashes)?

Thanks,
Crispy

"Literally, it means that Bob is everything you can think of, but not dead; i.e., Bob is a purple-spotted, yellow-striped bumblebee/dragon/pterodactyl hybrid with a voracious addiction to Twix candy bars, but not dead."- kSquared

This topic is closed to new replies.

Advertisement