Java Concurrency Issue with Iterator and Nested Hashtable

Question

Good morning, hoping to get some help from someone that might know a bit more about Java than myself. i am coming from a .NET background and have been tasked with tracking down a issue in a vendor supplied solution. I believe i have found it but would like a second , third or forth opinion if possible.

what i think is happening is that line number 108 (indicated in code block below) is modifying the Iterator (memIter) that was declared external to the while loop. They are modifying it by changing the instance that was declared internal to the loop and not the original object and i believe it throws because "next" is called on the second iteration on the modified collection/hastbale. i have found a number of threads on this site that point to this (http://stackoverflow.com/questions/602636/concurrentmodificationexception-and-a-hashmap) but because its modifying a collection (sorry if these are .net terms) within a collection (its removing a member from the property hastable of an item in the iterator) i would assume that the same logic would apply but its not my space. Also if my assumption is correct can somone please supply the correct implementation?

STACK

java.util.ConcurrentModificationException stack trace: java.util.ConcurrentModificationException at java.util.HashMap$HashIterator.nextEntry(HashMap.java:793) at java.util.HashMap$ValueIterator.next(HashMap.java:822) at xxx.xxxxx.xx.xxxxxxx.end(RoleOrganizer.java:108) at (xxxxxxxxx.java:568) at xxx.xxxx.xxxxxxx.handleRequest(xxxxHandler.java:74) at com.xxxxx.server.JavaInstanceMethod.execute(JavaInstanceMethod.java:33) at xx.xxxxxxx.execute(AppServer.java:1469) at xxx.xx.executeRequest(xxxxxjava:1269) at xxx.xxxxx.server.xxxx.doGet(xxxxx.java:350) at javax.servlet.http.HttpServlet.service(HttpServlet.java:690) at javax.servlet.http.HttpServlet.service(HttpServlet.java:803) at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:290) at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:206) at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:233) at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:175) at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:128) at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:102) at org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:109) at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:286) at org.apache.jk.server.JkCoyoteHandler.invoke(JkCoyoteHandler.java:190) at org.apache.jk.common.HandlerRequest.invoke(HandlerRequest.java:283) at org.apache.jk.common.ChannelSocket.invoke(ChannelSocket.java:767) at org.apache.jk.common.ChannelSocket.processConnection(ChannelSocket.java:697) at org.apache.jk.common.ChannelSocket$SocketConnection.runIt(ChannelSocket.java:889) at org.apache.tomcat.util.threads.ThreadPool$ControlRunnable.run(ThreadPool.java:690) at java.lang.Thread.run(Thread.java:619)

Code Below with stack error line number 108 indicated

Line 87 anpublic void end()

{

Iterator iter = new ArrayList(this.m_member.getRoles()).iterator();

while (iter.hasNext())

{

  UserType rt = (UserType)iter.next();
  if (!this.m_roleMap.containsKey(rt.getGID()))
  {
    this.m_member.removeRole(ut);
  }
}
iter = this.m_roleMap.values().iterator();

Line 99
while (iter.hasNext())

{
  UserType ut = (UserType)iter.next();
  if (ut.isUnique())
  {
    Iterator memIter = this.m_member.doTask().lookUpMembers().iterator();
    while (memIter.hasNext())
    {

Line 108
StoreMember mem = (StoreMember)memIter.next();

      if (mem.doWork() != this.m_member.getId())
      {
        if ((mem.hasRole(ut)) && (!mem.isFormer()))
        {
          mem.removeRole(ut);
        }
      }
    }
  }

Answer 1

You cannot modify a collection while iterating over it. The solution is to use the Iterator object, and call remove on it directly.

Using a ConcurrentHashMap helps, because it does not throw a ConcurrentModificationException. From ConcurrentHashMap:

Retrieval operations (including get) generally do not block, so may overlap with update operations (including put and remove). Retrievals reflect the results of the most recently completed update operations holding upon their onset. For aggregate operations such as putAll and clear, concurrent retrievals may reflect insertion or removal of only some entries. Similarly, Iterators and Enumerations return elements reflecting the state of the hash table at some point at or since the creation of the iterator/enumeration. They do not throw ConcurrentModificationException. However, iterators are designed to be used by only one thread at a time.

Answer 2

what i think is happening is that line number 108 (indicated in code block below) is modifying the Iterator

I suspect that you've misunderstood under which circumstances you get a ConcurrentModificationException.

A ConcurrentModificationException is not due to concurrent modification of the iterator, but of the underlying collection.

From the documentation:

This exception may be thrown by methods that have detected concurrent modification of an object when such modification is not permissible.

For example, it is not generally permissible for one thread to modify a Collection while another thread is iterating over it.

Answer 3

It certainly sounds like it's a problem of changing the collection while you iterate over it, although it's tricky to follow exactly what's going on without seeing doTask and lookUpMembers. (Note that the same logic would apply in .NET.)

Two possible fixes (only apply one!):

• Copy the result of lookUpMembers either within lookUpMembers itself or where you use it. Create a new list from it, and then it won't matter if you remove items from the original collection.
• Create a list of roles to remove within the loop and then loop again separately to remove them.

Personally I'd probably choose the first, because it means you should only need to make the change once. The result of calling doTask().lookUpMembers() doesn't sound like it's directly backed by the original collection. Either way, you should clearly document what you're doing.