Java ConcurrentHashMap not thread safe.. wth?

Question

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.

Kalamazoo

Have you tried Collections.synchronizedMap(map) ? – Eichler

"TF" is that ConcurrentHashMap is thread-safe, but you are expecting something else on top of thread safety. – Redraft

it would also help if you explained what "crash occurs here" means. What kind of "crash"? What's the exception? – Redraft

Please answer @Stephen C's questions and rewrite this as a question and less of a story. – Shanitashank

Sorry I'm a newbie to this whole deal, but I edited the question and put the exception error but it's still very hazy stacktrace. What would you guys recommend instead of extending the class something simple. I'd do copying collection idea but i'm not sure if thats even possible to be 100% safe aint objects all linked up by references so it's kinda pointless anyways if the copied object gets deconstructed/destructed somewhere else?. – Kalamazoo

JAVA DOC (docs.oracle.com/javase/6/docs/api/…) says ConcurrentHashMap class is fully interoperable with Hashtable in programs that rely on its thread safety but not on its synchronization details. – Papain

Answer 1

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.

Redraft

Thanks someone had to dumb it down for me as I am a simple man. But I don't understand if the values collection keeps same while iterating and it cannot hold null's why do I get the nullpointerexception? it's a reference problem then huh.. so these references are linked like addresses to same class. Other then expending on CocurrentHashMap what? atomically you mean like native copy method? nevermind i found it Package java.util.concurrent.atomic – Kalamazoo

By "atomic", I mean as a single uninterruptible action; see en.wikipedia.org/wiki/Atomicity_%28programming%29. BTW, in Java there is no such thing as an (atomic) native copy method. The only ways to guarantee atomicity in Java are using primitive or java.util.concurrent.* locking, or using volatile. Both approaches have caveats. – Redraft

@Kalamazoo - if the NPE occurred at exactly the point you indicated, it is probably not because of a null returned by the iterator. More likely, person.getGender() has returned null. – Redraft

Okay it's getGender(). I've overlooked that the gender doesn't get constructed with the class but it comes in a second later from a response packet since could be that delay which caused the null. – Kalamazoo

I'd like to ask the final question I seem to now get the impression that I cannot modify such as using put/remove? while I am iterating the ConcurrentHashMap? at any location? if thats true does it just skip the code? that uses put/remove or sets a block? either are pretty bad but hopefully it blocks.. i'd setup tests but maybe you guys can give me the answer quicker – Kalamazoo

Oh well i guess you abandoned this topic. Seems like you got the most votes i'll just accept your answer. – Kalamazoo

@Kalamazoo - (I didn't abandon the topic, but I do have other things happening in my life.) You can modify the map while iterating. However, you may or may not observe the results of the modifications in the concurrent iteration. That's what the javadoc says. – Redraft

Alright mate thanks for everything I really appreciate it. I ended up using a static counter which adds++ to which ever a tboy/tgirl/vboy/vgirl then at connection clean up removes based on their gender/camType etc after 24 hours of running the counters are totally screwed up some even in the negatives haha but alright i'll figure it out – Kalamazoo

Answer 2

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.

Arbitrary

Yeah thats it.. but thats because person is null.. seems like i will go with the idea of copying the values into the collections? idk if that will solve it? – Kalamazoo

@Kalamazoo it is not possible in your code for person.getChatType() to not dereference null and then person.getGender() to dereference null. I think you are misinterpreting your NullPointerException. – Shanitashank

Answer 3

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.

Santamaria

Thanks for that i'll remove it (hopefully you are not saying disinformation, sorry for being rude). But thanks to you guys I learn a new trick everyday! – Kalamazoo

@Kalamazoo The JavaDoc makes it clear that @Santamaria is not saying disinformation. download.oracle.com/javase/6/docs/api/java/util/concurrent/… – Shanitashank

Answer 4

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

Cioban

true.. i am wasting alot of cpu cycles but overriding the CocurrentHashMap seems too much trouble since this is just one command.. and many more commands to follow which do similar things. – Kalamazoo

Hey James can I be sure on one thing 100%? if the UserProfile class which is assigned to every client is copied to a new collection which i will iterate if the CocurrentHashMap removes that UserProfile isn't the reference (pointer?) the same? meaning both will get removed? can you clear that up for me. – Kalamazoo

Answer 5

ConcurrentHashMap is threadsafe and its functions are thread safe but it doesn't mean your all lines of code in which you are using concurrenthashmap are also thread safe. The block of code you must synchronize. You are iterating and trying to get object from concurrenthashmap but in middle some other thread is getting chance to remove object from same concurrenthashmap due to which your thread is throwing NPE.

Rosemarie