22

I have this code...

internal static void Start()
{
    TcpListener listenerSocket = new TcpListener(IPAddress.Any, 32599);
    listenerSocket.Start();
    listenerSocket.BeginAcceptTcpClient(new AsyncCallback(AcceptClient), null);
}

Then my call back function looks like this...

private static void AcceptClient(IAsyncResult asyncResult)
{
    MessageHandler handler = new MessageHandler(listenerSocket.EndAcceptTcpClient(asyncResult));
    ThreadPool.QueueUserWorkItem((object state) => handler.Process());
    listenerSocket.BeginAcceptTcpClient(new AsyncCallback(AcceptClient), null);
}

Now, I call BeginAcceptTcpClient, then some time later I want to stop the server. To do this I have been calling TcpListener.Stop(), or TcpListener.Server.Close(). Both of these however execute my AcceptClient function. This then throws an exception when I call EndAcceptTcpClient. What is the best practice way around this? I could just put a flag in to stop the execution of AcceptClient once I have called stop, but I wonder if I am missing something.

Update 1

Currently I have patched it by changing the code to look like this.

private static void AcceptClient(IAsyncResult asyncResult)
{
     if (!shutdown)
     {
          MessageHandler handler = new MessageHandler(listenerSocket.EndAcceptTcpClient(asyncResult));
          ThreadPool.QueueUserWorkItem((object state) => handler.Process());
          listenerSocket.BeginAcceptTcpClient(new AsyncCallback(AcceptClient), null);
     }
}

private static bool shutdown = false;
internal static void Stop()
{
     shutdown = true;
     listenerSocket.Stop();
}

Update 2

I changed it to impliment the answer from Spencer Ruport.

private static void AcceptClient(IAsyncResult asyncResult)
{
    if (listenerSocket.Server.IsBound)
    {
            MessageHandler handler = new MessageHandler(listenerSocket.EndAcceptTcpClient(asyncResult));
            ThreadPool.QueueUserWorkItem((object state) => handler.Process());
            listenerSocket.BeginAcceptTcpClient(new AsyncCallback(AcceptClient), null);
    }
}
Anthony D
  • 10,877
  • 11
  • 46
  • 67
  • 2
    You don't have to queue another thread to do the handling work; you can just call the EndAcceptTcpClient to tell the listener it is handled, and then call a BeginAcceptTcpClient right after it to schedule another handling. Use the current thread to handle the request you just received. – Ricardo Nolde Nov 26 '10 at 15:42

5 Answers5

17

I just ran into this issue myself, and I believe your current solution is incomplete/incorrect. There is no guarantee of atomicity between the check for IsBound and the subsequent call to EndAcceptTcpClient(). You can still get an exception if the listener is Stop()'d between those two statements. You didn't say what exception you're getting but I assume it's the same one I'm getting, ObjectDisposedException (complaining that the underlying socket has already been disposed).

You should be able to check this by simulating the thread scheduling:

  • Set a breakpoint on the line after the IsBound check in your callback
  • Freeze the thread that hits the breakpoint (Threads window -> right click, "Freeze")
  • Run/trigger the code that calls TcpListener.Stop()
  • Break in and step through the EndAcceptTcpClient() call. You should see the ObjectDisposedException.

IMO the ideal solution would be for Microsoft to throw a different exception from EndAcceptTcpClient in this case, e.g. ListenCanceledException or something like that.

As it is, we have to infer what's happening from the ObjectDisposedException. Just catch the exception and behave accordingly. In my code I silently eat the exception, since I have code elsewhere that's doing the real shutdown work (i.e. the code that called TcpListener.Stop() in the first place). You should already have exception handling in that area anyway, since you can get various SocketExceptions. This is just tacking another catch handler onto that try block.

I admit I'm uncomfortable with this approach since in principle the catch could be a false positive, with a genuine "bad" object access in there. But on the other hand there aren't too many object accesses in the EndAcceptTcpClient() call that could otherwise trigger this exception. I hope.

Here's my code. This is early/prototype stuff, ignore the Console calls.

    private void OnAccept(IAsyncResult iar)
    {
        TcpListener l = (TcpListener) iar.AsyncState;
        TcpClient c;
        try
        {
            c = l.EndAcceptTcpClient(iar);
            // keep listening
            l.BeginAcceptTcpClient(new AsyncCallback(OnAccept), l);
        }
        catch (SocketException ex)
        {
            Console.WriteLine("Error accepting TCP connection: {0}", ex.Message);

            // unrecoverable
            _doneEvent.Set();
            return;
        }
        catch (ObjectDisposedException)
        {
            // The listener was Stop()'d, disposing the underlying socket and
            // triggering the completion of the callback. We're already exiting,
            // so just return.
            Console.WriteLine("Listen canceled.");
            return;
        }

        // meanwhile...
        SslStream s = new SslStream(c.GetStream());
        Console.WriteLine("Authenticating...");
        s.BeginAuthenticateAsServer(_cert, new AsyncCallback(OnAuthenticate), s);
    }
David Pope
  • 6,457
  • 2
  • 35
  • 45
  • Yeah I ended up putting a stop bool that I set just before I call close. Then I check this before calling end accept. – Anthony D Aug 05 '09 at 13:46
  • Even that approach suffers from the threading problem. Basically, unless there's a lock that covers both the boolean and calls to the TcpListener, you can get thread scheduling that causes the exception: 1) callback checks the boolean, we're still listening, cool, 2) that thread is swapped out for the thread that sets the boolean and calls Stop(), 3) the callback thread resumes and calls EndAccept. – David Pope Aug 05 '09 at 18:25
  • Why do you cause the `ObjectDisposedException` by calling `EndAcceptTcpClient` on a closed listener instead of just checking whether it’s still live first, and simply returning from the callback? – Roman Starkov Mar 27 '12 at 18:16
7

No you're not missing anything. You can check the IsBound property of the Socket object. At least for TCP connections, while the socket is listening this will be set to true and after you call close it's value will be false. Though, your own implementation can work just as well.

Spencer Ruport
  • 34,865
  • 12
  • 85
  • 147
  • 1
    This is _the_ answer; just check `listener.Server.IsBound` in the async callback and if it’s false, just return. No need to call `EndAccept*` and then catch the (expected and documented) exception. – Roman Starkov Mar 27 '12 at 18:18
1

This is a simple example how to start listening, how to process requests asynchronously, and how to stop listening.

Full example here.

public class TcpServer
{
    #region Public.     
    // Create new instance of TcpServer.
    public TcpServer(string ip, int port)
    {
        _listener = new TcpListener(IPAddress.Parse(ip), port);
    }

    // Starts receiving incoming requests.      
    public void Start()
    {
        _listener.Start();
        _ct = _cts.Token;
        _listener.BeginAcceptTcpClient(ProcessRequest, _listener);
    }

    // Stops receiving incoming requests.
    public void Stop()
    { 
        // If listening has been cancelled, simply go out from method.
        if(_ct.IsCancellationRequested)
        {
            return;
        }

        // Cancels listening.
        _cts.Cancel();

        // Waits a little, to guarantee 
        // that all operation receive information about cancellation.
        Thread.Sleep(100);
        _listener.Stop();
    }
    #endregion

    #region Private.
    // Process single request.
    private void ProcessRequest(IAsyncResult ar)
    { 
        //Stop if operation was cancelled.
        if(_ct.IsCancellationRequested)
        {
            return;
        }

        var listener = ar.AsyncState as TcpListener;
        if(listener == null)
        {
            return;
        }

        // Check cancellation again. Stop if operation was cancelled.
        if(_ct.IsCancellationRequested)
        {
            return;
        }

        // Starts waiting for the next request.
        listener.BeginAcceptTcpClient(ProcessRequest, listener);

        // Gets client and starts processing received request.
        using(TcpClient client = listener.EndAcceptTcpClient(ar))
        {
            var rp = new RequestProcessor();
            rp.Proccess(client);
        }
    }
    #endregion

    #region Fields.
    private CancellationToken _ct;
    private CancellationTokenSource _cts = new CancellationTokenSource();
    private TcpListener _listener;
    #endregion
}
1

try this one. it works fine for me without catching exceptions.

private void OnAccept(IAsyncResult pAsyncResult)
{
    TcpListener listener = (TcpListener) pAsyncResult.AsyncState;
    if(listener.Server == null)
    {
        //stop method was called
        return;
    }
    ...
}
Ralph Shillington
  • 20,718
  • 23
  • 91
  • 154
  • 1
    The problem with this is that Stop() actually re-creates a new (un-bound) socket for Server. Start() then binds this socket (to the IPEndPoint or IPAddress + port it was created for) and starts it listening. That's why IsBound is slightly more accurate. – J Bryan Price Aug 12 '11 at 19:48
0

i think that all tree things are needed and that the restart of BeginAcceptTcpClient should be placed outside the tryctach of EndAcceptTcpClient.

    private void AcceptTcpClientCallback(IAsyncResult ar)
    {
        var listener = (TcpListener)ar.AsyncState;

        //Sometimes the socket is null and somethimes the socket was set
        if (listener.Server == null || !listener.Server.IsBound)
            return;

        TcpClient client = null;

        try
        {
            client = listener.EndAcceptTcpClient(ar);
        }
        catch (SocketException ex)
        {
            //the client is corrupt
            OnError(ex);
        }
        catch (ObjectDisposedException)
        {
            //Listener canceled
            return;
        }

        //Get the next Client
        listener.BeginAcceptTcpClient(new AsyncCallback(AcceptTcpClientCallback), listener);

        if (client == null)
            return; //Abort if there was an error with the client

        MyConnection connection = null;
        try
        {
            //Client-Protocoll init
            connection = Connect(client.GetStream()); 
        }
        catch (Exception ex)
        {
            //The client is corrupt/invalid
            OnError(ex);

            client.Close();
        }            
    }