Is this a correct use of multithreading design? (C#)

Question

I'm building a small chat program that consists of a server and client. The server keeps a list of clients that it interacts with.

I've got two worker threads on the server. One handles incoming client connections. The other handles incoming client messages.

Now, since both threads interact with a List called 'clients', I've done something like this.

// The clients list looks something like this...
List<TcpClient> clients;

// This is running on one thread.
ConnectionHandler()
{
    while(true)
    {
        // Wait for client to connect, etc. etc.

        // Now, add the client to my clients List.
        lock(clients)clients.Add(myNewClient);
    }
}

// This is running on another thread.
ClientHandler()
{
    while(true)
    {
        lock(clients)
        {
            /*
            This will be handling things like incoming messages
            and clients disconnecting (clients being removed from
            the 'clients' List
            */
        }
    }
}

Is this a correct use of locks to prevent my List from being altered by two different threads at once?

I haven't had any problems doing this so far, but I just want to make sure it's correct.

Answer 1

This is correct, but make sure that ClientHandler does not hold the lock for too long. It should never hold the lock while blocking (e.g. caused by an IO operation on a socket). If you violate this rule you will find your throughput being destroyed (still maintaining correctness).