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?

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.

public UserRecord()
{
}

//return true if the user name is not taken
{
return false;

return true;
}

synchronized void remove(ClientHandler handler)
{
}

synchronized int size()
{
return container.values().size();
}

{
}

synchronized Iterator getIterator()
{
return container.values().iterator();
}

synchronized void clear()
{
this.container.clear();
}
}


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()
{

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