Is this the use of multi-threaded design? (FROM#)

Question

I am creating a small chat program consisting of a server and a client. The server stores a list of clients with which it interacts.

I have two worker threads on the server. One handles incoming client connections. Another handles incoming client messages.

Now, since both threads are interacting with a list called "clients", I did 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 using locks correctly to prevent two different threads from modifying my list?

I have not had any problems with this yet, but I just want to make sure that this is correct.

Answer 1

This is correct, but make sure that the ClientHandler does not hold the lock for too long. It should never hold the lock while locking (for example, caused by an I / O operation on a socket). If you break this rule, you will find that your bandwidth is destroyed (while still maintaining correctness).

Answer 2

Do you have one writer and several readers? Check out ReaderWriterLock and these collections .

Answer 3

Looks like OK. Chat servers are very complex for multithreaded tasks. Exceptions can occur inside locks, for example, when an object-socket-server-client receives a break, but before its stream can remove the object from the list, another thread blocks the list and tries to write to the disconnected socket.

Answer 4

Note (above) - since you are not initializing the field (i.e. I don’t see how you do it, when you can destroy and reinitialize, etc.) - make sure that you lock the same instance , for example see Locking an object that may change during code execution

Answer 5

It looks good, but I would make this correction:

 private readonly List<TcpClient> clients = new List<TcpClient>(); 

You can also create a list in the constructor, but save it as readonly . This is the key to make sure that you are blocking the same object. Otherwise, if you re-create the clients list, your code will no longer be thread safe.