Do I need to lock my variables for thread safety?

You should lock variables if they are going to be accessed in multiple threads.

If you don't you will have trouble.

Note:
On re-reading, I use the term list and vector interchangably, as it is a list but is implemented as a vector so the terms have merged in my head :)

I made a little networked chat server/client, which was multithreaded. I did it to learn about multithreading and network stuff.

I have 3 pieces of data that need locking:

- list of clients- global "all theads quit now" flag- list of messages pending to be sent ( which doubles as a list of received messages in my app, in another app they would be separate )

Now, i don't really believe that my quit flag needs to be locked, as there is only ever 1 write to it, and that write doesn't check its value so concurrent changes should work( i.e. i say "done = true;" as opposed to "done = !done;" which could fail if 2 threads tried to request a shutdown at the same time ).

I made the list of clients a vector of pointers-to-clients, so if I have a client reference I don't worry about the vector moving the contents of the array around and invalidating my pointer.

All operations that affect the list of clients are locked, and I can ensure this because the data is hidden in an anonymus namespace and only accessible through functions.

To obtain a list of current clients, you pass a vector into a function, the function locks the client list copies all the elements into that vector, and unlocks the data. I know this isn't 100% proven to work perfectly if clients are deleted, but at the moment i have a GC thread for clients that takes its time since client closure to client cleanup, and it seems to work okay. The only operations allowed on the outgoing vector are read ones. At present this is only enforced by me, and not programmatically. (*notes that in TODO list*).

To obtain the list of messages, you pass in a vector, and the same thing is done except the message list is cleared then, as the only thread that will call this is the one tasked with sending messages to the clients, and I have to clear it sometime.

Adding messages to the queue or clients to the list is also locked, preventing little accidents.

Clients that are no longer active are moved into a zombie list, which is periodically cleared by a reaper thread. This is done inside the same function that copies the list of clients out, so that vector always has a list of clients that were active at the time of the call.

Most of this architecture has come about due to the blocking nature of some of the calls of the API I use, and the fact that I gave each client it's own listener thread.

Actually describing the program has alerted me to some things I hadn't taken into account, and I will work on them. Ensuring thread safety is hard.

Typically, locking just a single variable is not the right thing to do to get thread safety in your design.

class Foo {    int x;  public:    void setX( int v ) {      lock(); // useless lock      x = v;      unlock();    }    int getX() const {      lock(); // useless lock      return x;      unlock();    }};

You could just as well get rid of the locks, and the thread safety would be the same. The problem is that "x" has some semantic meaning at a higher scope, and you have to keep the object locked for modification while you operate on the higher scope.

In the end, the "higher scope" is often "the game world" which means that each object that needs to perform some action needs to lock "the game world" which means that everything ends up serializing on this single lock, and the threading is effectively useless.

A slightly better design involves using locking to safely put requests into a work queue, and a single thread then updating the world based on the work queue requests; this allows threads to receive network data and put items into work queues independently of world update. However, you're very seldom CPU bound on the receive-and-enqueue operation, so again, the threading buys you very little.