Handling messages from a server

I've never really done anything with concurrency, so I'm not sure if this is done correctly.

Basically, I have a class that handles all messages from the server and decides what to do them. It uses a separate thread to keep looping through a list to check for any new messages it has to deal with.

The main server thread needs to be able to add messages to the ArrayList without causing a concurrency error, and so far it seems ok but I don't have that many messages coming in.

public class MessageThread implements Runnable {
    public static ArrayList messages = new ArrayList();

    @Override
    public void run() {
        while (true) {
            synchronized (messages) {

                for (Message m : messages) {

        }
    }
}
    public static void addMessage(Message message) {
        synchronized (messages) {
            messages.add(message);
        }
    }

    public static void removeMessage(Message message) {
        synchronized (messages) {
            messages.remove(message);
        }
    }
}

So as you can see, the server thread would use the MessageThread.addMessage method whenever it needs to add a message.

Your ArrayList is correctly guarded from access by multiple threads but you've got another problem. The busy wait in your run method is going to hog the CPU and might even starve the other threads that would be trying to put a message in the queue. You'll need some way of signaling that the queue is non-empty so that the run function can wait on the signal. In .NET it would be a ManualResetEvent. I don't know the java equivalent. Or better yet, have a look at BlockingQueue.

@Override
public void run() {
    while (true) {
        queueFullEvent.waitOne();
        synchronized (messages) {
            for (Message m : messages) {
                // messages should be removed here
                messages.remove(m);
                if (messages.isEmpty)
                    queueFullEvent.reset();    
            }
        }
    }
}

public static void addMessage(Message message) {
    synchronized (messages) {
        messages.add(message);
        queueFullEvent.set();
    }
}

public static void removeMessage(Message message) {
   // would require additional syncronization objects to avoid a race.
}

}

StackExchange Code Review Q#18578, answer score: 4