A Bug in the MSDN TCP Server Sample for .NET 3.5 (& 2 SP1)

Particularly, the bug is to be found in:

    private void CloseClientSocket(SocketAsyncEventArgs e)
    {
      ...
    }

There is a condition that will crash the server if a client connection comes in between the time the Semaphore is released and when the SocketAsyncEventArgs object is 'released' back to the pool. More precisely, if the server has m_numConnections clients connected and one of the clients disconnects just at the same time as another client attempts to connect, the CloseClientSocket(..) function may be preempted just after the Semaphore has been released and before the SocketAsyncEventArgs object has been put back in the pool. Meanwhile, in the ProcessAccept(..) function the Server will expect that there should be at least one SocketAsyncEventArgs object in the pool, ready to be given to the new connection and the m_readWritePool.Pop() call will fail causing an exception which rightfully will crash the server.

In order to better illustrate this case, one could run the Server with a relatively low number set for m_numConnections (in my case I had 3) and configure a client application that will create multiple client connections at once (3-4) and then disconnect each connection as soon as it connects. This type of application will cause the server to crash within a few seconds (Note: I could not get the server to crash if I ran the client and server applications both on the same machine, which could also be because my computer only has one processor/core and so there is no true parallelism)

Here is the corrected version of the CloseClientSocket(..) function which fixes the glitch..

    private void CloseClientSocket(SocketAsyncEventArgs e)
    {
        AsyncUserToken token = e.UserToken as AsyncUserToken;

        // close the socket associated with the client
        try
        {
            token.Socket.Shutdown(SocketShutdown.Send);
        }
        // throws if client process has already closed
        catch (Exception) { }
        token.Socket.Close();

        // Before releasing the Semaphore..
          // Free the SocketAsyncEventArg so they can be reused by another client
          m_readWritePool.Push(e);

          // decrement the counter keeping track of the total number of clients connected to the server
          Interlocked.Decrement(ref m_numConnectedSockets);
        
        // Finally, release the m_maxNumberAcceptedClients semaphore
        m_maxNumberAcceptedClients.Release();
        Console.WriteLine("A client has been disconnected from the server. There are {0} clients connected to the server", m_numConnectedSockets);
    }

Also, the author of the code sample uses instances of a class called AsyncUserToken which is not provided but which, after a quick examination could be deduced as being a simple container for a System.Net.Socket object, poorly named: Socket. The code for this class could like like this:

    class AsyncUserToken 
    {
        private System.Net.Socket _socket;
        
        public AsyncUserToken()
        {
        }
    
        // public property to get and set the internal private _socket member
        public System.Net.Socket Socket
        {
            get { return _socket; }
            set { _socket = value; }
        }
            
    }

Or, one could substitute the whole process of creating and AsyncUserToken class just to wrap the Socket object with simply passing a reference to the Socket itself.

Visit the MSDN Library page at: http://msdn.microsoft.com/en-us/library/system.net.sockets.socketasynceventargs.aspx