Synchronized LinkedHashed map

Question

I've written the following code ages ago (10+ years) which is part of a simple chat server. I'm going to refactor it a bit in Java and then for fun I'm going to convert it to Scala and use Akka actors instead of threading.

Do you see any thread issues that can arise from the method of synchronization?

If a thread appends a record and another thread performs a read on the thread that reads, is it not going to have the current data?

Upon reading this article, I assume that is not the case?

I could use a ConcurrentHashMap instead but it doesn't preserve the order of insertion.

final class UserRecord
{
 //unsynchronized version of hashtable which maintains the order
 //which the objects where inserted.
 private LinkedHashMap container;
 public UserRecord()
 {
 container = new LinkedHashMap();
 }
 //return true if the user name is not taken
 synchronized boolean add(ClientHandler handler)
 {
 if (container.containsKey(handler.getUserName()) == true)
 return false;
 container.put(handler.getUserName(), handler);
 return true;
 }
 synchronized void remove(ClientHandler handler)
 {
 container.remove(handler.getUserName());
 }
 synchronized int size()
 {
 return container.values().size();
 }
 synchronized ClientHandler getHandler(String userName)
 {
 return (ClientHandler) container.get(userName);
 }
 synchronized Iterator getIterator()
 {
 return container.values().iterator();
 }
 synchronized void clear()
 {
 this.container.clear();
 }
}

Answer 1

Yes there is a thread safety issue. While the getIterator() is synchronized, the Iterator it returns is not. LinkedHashMap supports removal through the values() view, and hence the Iterator can be used to modify the Map without proper synchronization.

 UserRecord userRecord = new UserRecord();
 populate(userRecord);
 Iterator iterator = userRecord.getIterator();
 iterator.next();
 iterator.remove(); // unsynchronized modification of the Map

The easiest solution is to use Collections.synchronizedMap() which basically does what you attempted.

private Map<String, ClientHandler> container;
public UserRecord()
{
 container = Collections.synchronizedMap(new LinkedHashMap<>());
}

If you do this, the methods on UserRecord no longer need to be synchronized.

As an aside, you will want to properly use generics.

• firephil: Thanks, this is old code, pre generics. using Collections.synchronizedMap(new LinkedHashMap<>()) is the simplest solution i believe. I was thinking to replace the method getIterator to getEnumeration to return an enumeration that hasn't got any modifiers
• bowmore: Using ConcurrentHashMap would be an even simpler and better solution as its concurrency performance is much better under contention. I'm not sure why you need to have the order of addition to the user record, but if that can be replaced by some kind of sorting by the client code, then switching to ConcurrentHashMap is definitely the way to go.
• firephil: I needed to maintain the order of insertion that is why i used back then a LinkedHashMap and also the traversal is faster than a HashMap!
• Robin: Also, in your add method. Don't compare the result of a boolean method with true or false. It is already true or false. This resolves to if(true == true) or if (false == true).