Java ConcurrentHashMap not thread safe.. wth?

Question

Hello

I was using HashMap before like

   public Map<SocketChannel, UserProfile> clients = new HashMap<SocketChannel, UserProfile>();

now I've switched to ConcurrentHashMap to avoid synchronized blocks and now i'm experiencing problems my server is heavily loaded with 200-400 concurrent clients every second which is expected to grow over time.

which now looks like this

public ConcurrentHashMap<SocketChannel, UserProfile> clients = new ConcurrentHashMap<SocketChannel, UserProfile>();

My server design works like this. I have a worker thread(s) for processing huge amounts of packets. Each packet is checked with a packetHandler sub-routine (not part of thread) pretty much any client can call it at anytime it's almost like static but it isn't.

My whole server is mostly single threaded except for the packet processing portion.

Anyways so when someone uses a command like count up all clients online and get some information from them.

It is also possible that clients can get disconnected and removed from ConcurrentHashMap while the counting is in progress (which causes my problems).

Also I'd like to add some code here.

                int txtGirls=0;
                int vidGirls=0;
                int txtBoys=0;
                int vidBoys=0;
                Iterator i = clients.values().iterator();
                while (i.hasNext()) {
                    UserProfile person = (UserProfile)i.next();
                    if(person != null) {
                        if(person.getChatType()) {
                            if(person.getGender().equals("m"))
                                vidBoys++;
                            else //<-- crash occurs here.
                                vidGirls++;
                        } else if(!person.getChatType()) {
                            if(person.getGender().equals("m"))
                                txtBoys++;
                            else
                                txtGirls++;
                        }
                    }
                }

I mean of course i'm going to fix it by adding a try-catch Exception inside the Iterator to skip these null clients.

But what I don't understand if it checks above if(person != null) shouldn't the code nested automatically works..

if it doesn't means it got removed while it was iterating which should be impossible since it's thread safe wtf?

What should I do? or is try-catch Exception the best way?

Here is the exception

java.lang.NullPointerException
    at Server.processPackets(Server.java:398)
    at PacketWorker.run(PacketWorker.java:43)
    at java.lang.Thread.run(Thread.java:636)

The processPackets contains the code above. and the comment indicates the line count #

Thanks for enlightening me.

Answer 1

You may find that you can't have the map get modified while you are iterating through it. If that is the case you may want to get the values and keys in a separate collection and iterate through that, as it will be immutable.

It won't be perfect, but the other option is to extend ConcurrentHashMap and when something is added or removed you update these four variables, so you don't have to iterate through the entire list each time, as that seems to be a waste of cpu cycles.

Here are a couple of links that may be useful:

This one talks a bit about the fact that the improved concurrency is because of relaxing of some promises. http://www.ibm.com/developerworks/java/library/j-jtp07233.html

memory consistency properties explained: http://download-llnw.oracle.com/javase/6/docs/api/java/util/concurrent/package-summary.html#MemoryVisibility

Answer 2

You need to read the javadocs for the ConcurrentHashMap.values() method, paying special attention to this description of how the iterator for the values() collection works:

"The view's iterator is a "weakly consistent" iterator that will never throw ConcurrentModificationException, and guarantees to traverse elements as they existed upon construction of the iterator, and may (but is not guaranteed to) reflect any modifications subsequent to construction."

The iterator does not give you a consistent snapshot of the state of the values collection, but it is thread-safe, and the expected range of behaviours is clearly specified.

If you want a Map implementation that gives you a consistent snapshot of the values (or keys, or entries) in the map AND allows you to iterate concurrently with modifications, you will probably need to create a custom Map wrapper class (that copies the collections atomically) ... or a full-blown custom Map implementation. Both are likely to be a lot slower than a ConcurrentHashMap for your use-case.

Answer 3

I don't see anything wrong with your code. Because it's unlikely that the crash actually occurs at the else, it's likely that the getGender() method is returning null.

Answer 4

java.util.concurrent.ConcurrentHashMap does not allow null value. So, null check(person != null) in your code is unnecessary.

If you want to deny modification of Map while iteration, you must use synchronization block in above code and all modification operation codes.