Removing items from HashMap gives ConcurrentModificationException

Question

I have a server application which has an ArrayList of connections. This program makes use of multiple threads. The main thread runs a process at intervals to detect closed connections and then remove them from the array so they can be garbage collected.

This process is as follows:

private void cullOtherProcessors() throws Exception {
    //log.log(Level.FINE,"XMLMessageReciever:cullOtherProcessors");
    List<ConnectionAppInterface> toDel = new ArrayList<ConnectionAppInterface>();
    for (ConnectionAppInterface cur : m_OtherProcessors.keySet()) {
        if (cur!=null) {
            if (cur.isClosed()) {
                //Connection is closed - we could never send a message over it
                toDel.add(cur);
            }
        }
    }
    for (int c=0;c<toDel.size();c++) {
        log.log(Level.FINE,"**XMLMessageReciever:cullOtherProcessors - removing closed connection");
        m_OtherProcessors.remove(toDel.get(c));
    }
}

My server ran for a couple of months but according to the logs it crashed with the following error:

08/10/16 01:06:39     calling connect
08/10/16 01:06:39   **XMLMessageReciever:cullOtherProcessors - removing closed connection
08/10/16 01:06:39   CloseableThread: End of run for thread Socket.Connect(113726)
08/10/16 01:06:39     Checking connected
08/10/16 01:06:39     Active Threads this group: 5
08/10/16 01:06:39     getting Message Reciever
08/10/16 01:06:39     Active Threads: 8
08/10/16 01:06:39     Setting m_establishingCon
08/10/16 01:06:39     Establishing connection to robertsNode
Server Failed
java.util.ConcurrentModificationException
    at java.util.HashMap$HashIterator.nextNode(HashMap.java:1429)
    at java.util.HashMap$KeyIterator.next(HashMap.java:1453)
    at metcarob.com.common.network.xmlprotocol.XMLMessageReciever.cullOtherProcessors(XMLMessageReciever.java:57)
    at metcarob.com.common.network.xmlprotocol.XMLMessageReciever.LoopIteration(XMLMessageReciever.java:98)
    at metcarob.com.common.network.xmlprotocol.ConnectionManager.LoopIteration(ConnectionManager.java:48)
    at metcarob.com.personalservices.singlenodeserver.Main.run(Main.java:138)
    at metcarob.com.personalservices.singlenodeserver.Main.main(Main.java:398)

Basically something happened to the ArrayList while I was looping through it (possibly another connection being established) causing nextNode to throw the exception.

I am trying to work out the best method of getting around it. I am considering simply catching and ignoring the error. Any threads missed will simply be culled on the next loop. My proposed solution is:

private void cullOtherProcessors() throws Exception {
    //log.log(Level.FINE,"XMLMessageReciever:cullOtherProcessors");
    //m_OtherProcessors
    List<ConnectionAppInterface> toDel = new ArrayList<ConnectionAppInterface>();
    try {
        for (ConnectionAppInterface cur : m_OtherProcessors.keySet()) {
            if (cur!=null) {
                if (cur.isClosed()) {
                    //Connection is closed - we could never send a message over it
                    toDel.add(cur);
                }
            }
        }
    } catch (ConcurrentModificationException e) {
        log.log(Level.FINE,"**XMLMessageReciever:cullOtherProcessors - ConcurrentModificationException being ignored");
    }
    for (int c=0;c<toDel.size();c++) {
        log.log(Level.FINE,"**XMLMessageReciever:cullOtherProcessors - removing closed connection");
        m_OtherProcessors.remove(toDel.get(c));
    }
}

I will now put this code back into the server and run it for more months.

I would like to know if this is a good solution to the problem. Since it took months for this to occur in the first place and the code loops connections every 5 minutes I think there is a low chance it would occur so often the connections will never be culled. (I will watch the logs and see)

It would be good to know what experts thinks.

**It was suggested this question is the same as "Iterating through a Collection, avoiding ConcurrentModificationException when removing in loop"

It is not. In my code I do not remove items while iterating over the HashMap, instead I iterate over the map and store the items I intend to remove in a temporary Array. I then go through the Array and remove the items.

Uhm, ignoring the exception is not a pretty thing to do. Have you tried to extract this unit and to reproduce the issue with threads? I see a race condition there, because you're reading and writing the same data structure m_OtherProcessors without syncronisation — Paolof76, Oct 14 '16 at 08:59
Are you sure that your error is related to multithreading? ConcurrentModificationException is nothing to do with threads. If it occurs because of multithreading, simply catching the error will not save you from the consequences of not using a concurrent data structure. — Tamas Hegedus, Oct 14 '16 at 09:03
m_OtherProcess is a private variable in this class " Map m_OtherProcessors = new HashMap(); " and items are put into it when new Connections are established — Robert3452, Oct 14 '16 at 09:03
Possible duplicate of [Iterating through a Collection, avoiding ConcurrentModificationException when removing in loop](http://stackoverflow.com/questions/223918/iterating-through-a-collection-avoiding-concurrentmodificationexception-when-re) — Jaroslaw Pawlak, Oct 14 '16 at 09:05
@Robert3452 Yeah but then you read the keys and if the connection is closed, you try to remove it. Think about what happen when another thread it going through the keys list when you remove an element... It gives it this exact exception i think. Try to syncronize the two operations, so nobody can come in beetween... — Paolof76, Oct 14 '16 at 09:08
Paolof76 I think this is right. There are two points I go through this list. I have been looking at docs and I think if I changed HashMap to ConcurrentHashMap it might fix this without any other code changes. — Robert3452, Oct 14 '16 at 09:14
Your code checks for null keys. Null keys are not supported in ConcurrentHashMap as far a I recall. — GPI, Oct 14 '16 at 09:34

Nicolas Filotto · Answer 1 · 2016-10-14T10:18:44.720

1

An HashMap is not a thread-safe collection which means that you are not suppose to share it with several threads otherwise you will get bugs hard to reproduce like ConcurrentModificationException or due to memory barriers.

Catching a ConcurrentModificationException is clearly a bad practice and should never be done since it must be seen as the guard of your HashMap that will notify you if your map has been modified concurrently which could finally lead to a broken map and/or worse to data loss.

You should rather use a thread-safe collection from the package java.util.concurrent. If you need a thread-safe Map, your best choice is to use a ConcurrentHashMap.

edited Oct 14 '16 at 10:18

answered Oct 14 '16 at 09:17

Nicolas Filotto

43,537
11
94
122

1

I agree with this post, but why do you mention List's thread safety. The issue here (the stacktrace I mean) is with thread safety of the map's keyset iteration. The only list being iterated here is totally safe (it is strictly local to the method invocation) and poses no issue. So I do agree, but this looks like "beside the point" to me ? – GPI Oct 14 '16 at 09:40
@GPI You are right, I have been confused by the question, fixed now – Nicolas Filotto Oct 14 '16 at 09:47
@GPI I also modified the title of the question which is why I got confused, thx for your remark – Nicolas Filotto Oct 14 '16 at 09:50
Yes, this is all clearer now. Thanks. Other answers have been mislead too. – GPI Oct 14 '16 at 09:54
1

Maybe you should also mention that using of a plain map poses visibility issues, on top of concurrent modification issues. The use of a concurrent map is, therefore, doubly recommandable. – GPI Oct 14 '16 at 09:57
1

Yes, exactly. Nothing guarantees, with a regular map without any synchonization, that any modification made by a thread will indeed be visible by other threads. And this is not purely theoretical, it does happen :-). – GPI Oct 14 '16 at 10:19

score 0 · Answer 2 · answered Oct 14 '16 at 08:59

0

You will be better off using synchronizedList: https://docs.oracle.com/javase/8/docs/api/java/util/Collections.html#synchronizedList-java.util.List-

answered Oct 14 '16 at 08:59

Peter Gelderbloem

512
3
12

Thanks for the link. I am looking into synchronizedMap. (I might turn m_OtherProcessors into a synchronizedMap) – Robert3452 Oct 14 '16 at 09:05
you will still need to use an intrinsic lock to iterate over your list safely – Nicolas Filotto Oct 14 '16 at 09:33
The list is strictily local to the method invocation. It poses no issue as far as thread safety goes. Using a thread safe implementation will not help. THe stack trace is about iteration on the map's keyset. -1. – GPI Oct 14 '16 at 09:53

score 0 · Answer 3 · answered Oct 14 '16 at 09:03

0

Hi it seems you have modification of m_OtherProcessors while looping over it.

There is two solution better than catching the exeption i can think of.

1- if m_OtherProcessors is a small object and mem is not a problem you could just make a local copy of your object such as HashMap<...> copy = new HashMap<..>(m_OtherProcessors). And run your function with this objet. ( if you want to avoid the possibility of crash during the copy which can be very unlikely you may insert a synchronize statment before copy like synchronized(m_OtherProcessors){HashMap<...> copy = new HashMap<..>(m_OtherProcessors)} )

2 - You just synchronized your method or arrayList, such synchronization will result in loss of performance but better reliability.

answered Oct 14 '16 at 09:03

Antoine Bergamaschi

104
1
5

Solution 1 will not work : copying the hashMap into a new one entails iteration throught its entries. THis iteration is not anymore safe than the one that causes the problem faced by the poster, however unlikely it sounds : if it did happen with the OP's code, it will happen here also, the loops would look and perform the same. SOlution 2 will not work either : there is no problem with the iteration on the list, which is strictly local to the method. -1. – GPI Oct 14 '16 at 09:52
Solution 1 work when the operation of creation of the new object is small enought to avoid modification of the same object in the time it is created. But as i have stated it is possible (with less chance ) that this error occurs so that adding synchronize statment on the creation of the object resolve the problem. For the second parts, thats completely wrong iteration in the list through m_OtherProcessors.keySet() do not involve iterator... so its not local as you suggest. – Antoine Bergamaschi Oct 14 '16 at 10:06
Sol. 1: syncrhonizing here will not be enough though, you should suggest synchronizing on the side that performs the modification too. Sol. 2 : i fail to see how, as I read your suggestion, sychronizing the arrayList would make the keySet iteration safer. The ArrayList looks perfectly safe to me : it is instanciated and referenced only inside the method, and it is never modified while being iterated on code. The map keySet is the issue. As far as the exception goes, even without an arrayList at all you could still run into the issue. It simply is unrelated. – GPI Oct 14 '16 at 10:18
Sol 1; the newly created object will not be link to the old by structure ( only object inside will remain the same ), so the newly created object became a function local object, so you can not in any way have the concurrence problem any more. Sol 2. Synchronizing hashmap means create a synchronized object ( as suggested by other response ). The m_OtherProcessors object don't come from the method so its not local, keyset is a field from the hashmap created only once, so synchronizing it usage will result in removing the exeption – Antoine Bergamaschi Oct 14 '16 at 11:17
Sol1: agreed, but my point is about locking `m_OtherProcessors` : you should lock here, AND in the method wich adds entries. Not only here. Sol2: "synchronizing its usage" meaning synchronizing the **keySet**. But you suggest "synchronized your **method** or **arrayList**", neither of which will do any good. Synchronizing the **method** does not prevent concurrent write to `m_OtherProcessors` (which is why the code fails). Synchronizing of the **arrayList** will not change anything on the **keySet**, right ? – GPI Oct 14 '16 at 12:31

score 0 · Answer 4 · answered Oct 14 '16 at 09:09

0

concurrent modification exception is caused when you perform data modification during traversing/reading your collection.

one easiest way to prevent from it; use iterator to traverse a collection.

ex.

Iterator<String> iter = myArrayList.iterator();

    while (iter.hasNext()) {
        String str = iter.next();

        if (someCondition)
            iter.remove();
    }

answered Oct 14 '16 at 09:09

Piyush Mittal

1,860
1
21
39

This is true, but incomplete. In a multi-threaded scenario, this will not help : hashmap's derived iterators are not safe for removal in case of concurrent access. You may end up with stale data, invisible data, block your code inside an infinite loop... All kinds of bad thing can happen. Plus, the stacktrace does not mention that the issue lies inside the removal of elements of a list, but inside the iteration of hashmap. Therefore, this answer, although stating something true, is inappropriate to this question. -1. – GPI Oct 14 '16 at 09:51
in multi-threaded scenario OP can use synchronized block or locking mechanism its totally dependent on project to which mechanism to follow. – Piyush Mittal Oct 14 '16 at 09:57
Yes, and the OP has a concurrent modification exception coming from multiple threads, as is made clear by the stack trace, not a concurrent removal from a single thread issue that your post correctly adresses, making it a valid answer, just not for this question. – GPI Oct 14 '16 at 10:00

Removing items from HashMap gives ConcurrentModificationException

4 Answers4