How can I make this thread safe?

Question

I have a server that receives various xml messages from clients (one thread per client) and routes the messages to different functions depending on the message type. Eg. if the first element in the messages contains the string 'login' it signifies that this is a login message so route the message to the login() function.

Anyway, I want to make this message so things don't get messed up if multiple clients are connected and the dispatcher switches threads in middle of the message routing. So here is how I am routing the messages -

public void processMessagesFromClient(Client client)
{
    Document message;

    while (true)
    {
        try
        {
            message = client.inputStream.readObject();

            /*
             * Determine the message type
             */
            String messageType = getMessageType(message);

            // Route the message depending on its type
            switch (messageType)
            {
                case LOGIN:
                    userModel.handleLogin();
                ...
                ...
                ...
                etc...
             }
        } catch(Exception e) {}
   }

So how can I make this thread safe? I figure I need to put a synchronise statement in somewhere but Im not sure where. Also Ive been reading around on the subject and I found this post which says there is an issue with using synchronise on 'this' - https://stackoverflow.com/a/416198/1088617

And another post here which says singletons aren't suitable for using synchronise on (My class in the code above is a singleton) - https://stackoverflow.com/a/416202/1088617

Answer 1

I would actually have a message handler thread which is responsible for reading incoming messages. This will then hand off processing to a worker thread to do the time consuming processing of the message. You can use the Java ThreadPoolExecutor to manage this.

Answer 2

Your class is already thread safe, because you are only using local variables. Thread safety only comes into play when you access class state (ie fields), which your code doesn't (seem to) do.

What you are talking about is serialization - you want to funnel all message processing through one point to guarantee that message processing is one-at-a-time (starts and finishes atomically). The solution is simple: Employ a static synchronized method:

public void processMessagesFromClient(Client client) {
    Document Message;

    while (true) {
        processMessage(client);
    }
}

private static synchronized processMessage(Client client) {
    try {
        message = client.inputStream.readObject();

        String messageType = getMessageType(message);

        // Route the message depending on its type
        switch (messageType) {
            case LOGIN:
                userModel.handleLogin();
            ...
            etc...
        }
    } catch(Exception e) {}
}

FYI static synchronized methods use the Class object as the lock. This code will make your code behave like a single thread, which your question seems to want.

Answer 3

Provided you have one of these objects per thread, you don't have a problem. You only need to synchronized a shared object which can be modified by one of the threads.

public void processMessagesFromClient(Client client) {    
    while (true) {
        processMessage(client);
    }
}

private void processMessage(Client client) {
    try {
        Document message = client.inputStream.readObject();

        String messageType = getMessageType(message);

        // Route the message depending on its type
        switch (messageType) {
            case LOGIN:
                userModel.handleLogin();
            ...
            etc...
        }
    } catch(Exception e) {}
}

Answer 4

If you already have 1 thread per connection, then the only thing that you have to synchronize are the functions which handle the events (i.e. functions like userModel.handleLogin()).

Answer 5

I guess the best solution should be to use a thread safe queue like the ConcurrentQueue and use a single working thread to pick up this values and run the actions one by one.

Answer 6

You need to know which resource should be only used be one thread at a certain time.

In your case it is likely that reading the next message needs to protected.

 synchronize (lock) {
      message = client.inputStream.readObject();
 }

However, your code sample does not really show what needs to protected against concurrent access

Answer 7

The method itself is thread safe. However, noting that this your class is a singleton, you might want to use double checked locking in your getInstance to ensure thread safety. Also you should make sure your instance is set to static

      class Foo {
            private static volatile Foo instance = null;
            public static Foo getInstance() {
                if (instance == null) 
                {
                    synchronized(this) 
                    {
                        if (instance == null)
                            instance = new Foo ();
                    }
                }
                return instance ;
            }
        }