4
\$\begingroup\$

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();
    }
}
\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
4
  • \$\begingroup\$ 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 \$\endgroup\$ – firephil Jul 17 '14 at 16:55
  • \$\begingroup\$ 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. \$\endgroup\$ – bowmore Jul 17 '14 at 17:05
  • \$\begingroup\$ 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! \$\endgroup\$ – firephil Jul 18 '14 at 10:16
  • \$\begingroup\$ 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). \$\endgroup\$ – Robin Jul 21 '14 at 16:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.