Synchronizing collections and aborting tasks

Question

I am writing a small multi-threaded network server. All classical stuff: it listens for incoming connections, accepts them and then serves them in different threads. Also, this server sometimes will have to restart, and to do so it must a) stop listening, b) kick out all connected clients, c) adjust some settings/wait, d) resume listening.

Well, I pretty much don't know a thing about developing multi-threaded programs, so I am looking for help. Here's what I came to (core stuff only):

class Server
{
    class MyClient
    {
        Server server;
        TcpClient client;
        bool hasToFinish = false;

        public MyClient(Server server, TcpClient client)
        {
            this.server = server;
            this.client = client;
        }

        public void Go()
        {
            while (!hasToFinish)
            {
                // do all cool stuff
            }
            CleanUp();
        }

        private void CleanUp()
        {
            // finish all stuff

            client.Close();
            server.myClients.Remove(this);
        }

        public void Finish()
        {
            hasToFinish = true;
        }
    }

    bool running = false;
    TcpListener listener;
    HashSet<MyClient> myClients = new HashSet<MyClient>();

    public void Start()
    {
        if (running)
            return;

        myClients.Clear();
        listener = new TcpListener(IPAddress.Parse("127.0.0.1"), 1234);
        listener.Start();
        listener.BeginAcceptTcpClient(AcceptClient, this);
        running = true;
    }

    public void Stop()
    {
        if (!running)
            return;

        listener.Stop();
        foreach (MyClient client in myClients)
        {
            client.Finish();
        }
        myClients.Clear();
        running = false;
    }

    public void AcceptClient(IAsyncResult ar)
    {
        MyClient client = new MyClient(this, ((TcpListener)ar.AsyncState).EndAcceptTcpClient(ar));
        myClients.Add(client);
        client.Go();
    }
}

It's absolutely unsatisfactory. There is no sychronizing (I just don't know where to put it!), and calling Server.Stop() doesn't make MyClient-s to stop immediately. How do I fix these problems?

Answer 1

The code looks quite clean, we can make it thread-safe with simple modifications.

There are three parts of the problem, the "client", the "server" and the client-server interaction.

Client first, the Go() method is invoked by one thread (let's call it A) and the Finish() method is invoke by another thread (B). When thread B modify hasToFinish field, thread A may not see the modification immediately because the variable may be cached in the CPU cache. We can fix it by making hasToFinish field "volatile", which force thread B to publish the variable change to thread A when update.

Now the server class. I recommend you to synchronise three methods on the "Server" instance like the example below. It makes sure Start and Stop are called sequentially and the variables they changes are published across threads.

The client-server interaction need to be addressed as well. In your code, Client remove its reference from the Server but the server clear all clients references when Finish() any way. It looks redundant to me. If we can remove the part of code in client, we have nothing to worry about. If you choose to keep the logic in the client rather in the server for what ever reason, create a public method call RemoveClient(Client client) in the Server class and synchronise it against the Server instance. Then let the client to invoke this method instead of manipulating the HashSet directly.

I hope this solve your problem.

public void Start()
{
  lock(this) 
  {
    if (running)
        return;

    myClients.Clear();
    listener = new TcpListener(IPAddress.Parse("127.0.0.1"), 1234);
    listener.Start();
    listener.BeginAcceptTcpClient(AcceptClient, this);
    running = true;
  }
}

public void Stop()
{
  lock(this)
  {
    if (!running)
        return;

    listener.Stop();
    foreach (MyClient client in myClients)
    {
        client.Finish();
    }
    myClients.Clear();
    running = false;
  }
}

public void AcceptClient(IAsyncResult ar)
{
  lock(this)
  {
    MyClient client = new MyClient(this, ((TcpListener)ar.AsyncState).EndAcceptTcpClient(ar));
    myClients.Add(client);
    client.Go();
  }
}