Concern over handling bad call to accept()

Question

I'm writing a MUD server for personal learning purposes and I've, happily, managed to wrap up the socket stuff up into a couple of classes and everything appears to be working correctly; the server listens for and accepts connections and currently takes text from the client and sends it right back.

The thing is I'm not quite sure what to do with a call to accept() that returns something other than WSAEWOULDBLOCK or a valid socket. Do I just reset the new socket to 0 and return, with maybe an error message saying something bad happened? This is what I'm currently doing now, with the addition of if it happens 20 times I'll shut down the server.

void MUDControlSocket::Poll()
{
   // create a new connection here
   timeval timeout;

   FD_ZERO(&ReadSet);
   FD_ZERO(&WriteSet);
   FD_ZERO(&ExceptionSet);

   TopSocket = GetSocket();
   NewSocket = 0;
   FD_SET( GetSocket(), &ReadSet );

   if( SocketList.size() > 0 )
   {
      for( sockIter iter = SocketList.begin(); iter != SocketList.end(); ++iter )
      {
         FD_SET((*iter)->GetSocket(), &ReadSet);
         FD_SET((*iter)->GetSocket(), &WriteSet);
         FD_SET((*iter)->GetSocket(), &ExceptionSet);
         TopSocket = (*iter)->GetSocket();
      }
   }

   if( select( TopSocket+1, &ReadSet, &WriteSet, &ExceptionSet, &timeout ) == SOCKET_ERROR )
   {
      cout << "Error on select() call: " << SocketErrorType(WSAGetLastError()) << endl;

      delete this;
      exit(EXIT_FAILURE);
   }

   // as long as everything is working correctly, this if block should always be entered UNLESS a new connection is accepted
   if( (NewSocket = accept(GetSocket(), NULL, NULL) ) == INVALID_SOCKET )
   {
      if( WSAGetLastError() == WSAEWOULDBLOCK ) // it's not an actual problem. just nothing to connect to yet
         return;
      NewSocket = 0;
      static int count = 0;
      cout << "Error on accepting new connection: " << SocketErrorType(WSAGetLastError()) << endl;
      if( ++count >= 20 )
         done = true;
      return;
   }

   SocketList.push_back(new MUDSocket(NewSocket)); // only happens if accept DOES NOT return a value of INVALID_SOCKET i.e. a new connection was accepted
   TopSocket = NewSocket;
   NewSocket = 0;
}

TopSocket and NewSocket are of type SOCKET and declared at file scope. SocketList is a std::list of MUDSocket* and MUDControlSocket is derived from MUDSocket as a singleton.

Let me know if you need more info and thanks for any help.

Answer 1

First: don't set the socket to 0: that's a valid fd for sockets on some *NIX systems, and a bad habit to take. Assume the only invalid socket fd is -1. Doing anything else will give you real bugs in real software later on (trust me: I'm speaking from experience debugging code that used 0 as an invalid socket fd).

Other than that, I'd say just raise an exception: accept shouldn't fail unless you run out of resources, which should be both exceptional and an error. C++ has a mechanism for handling such things, and that's exceptions.

BTW: delete this is almost always a very bad idea, exiting in the middle of your code may make it hard to debug (throw an exception in stead) and let the caller do the exiting if need be) and in stead of trying to accept a socket with accept you can use select to tell you whether there is anything to accept - and move the special-case handling out of the function to only select in there. You could go a bit further and implement a specialized observer pattern (like I did on my podcast about a month ago) to practice not only your networking code, but your design patterns as well. That would also help make your code more portable, and re-usable later.

HTH

Answer 2

Return the error and let the calling code deal with it appropriately.

Answer 3

Some of the other errors possible from accept is that memory is low, the number of connections is exhausted, etc., etc.

Perhaps it could be handled by closing unused or forgotten connections, or simply giving up and throwing an exception.