4

My below recursive function throws a ConcurrentModificationException on the 'continue' statement. I looked at a few posts on ConcurrentModificationException and all of the problems seem to be with removing an element from an element, but I am not removing any element in my function.

My function is below : 

    public static void getRootedTreeHelper(Node n, boolean[] marked, String spacing){ 

        System.out.println(spacing + "in helper with " + n.getId());
        marked[n.getId()] = true;
        if(n.children.isEmpty())
            return;
        else{
            for(Node child : n.children){
                if(marked[child.getId()])
                    continue; // ConcurrentModificationException is thrown here. 
                else{
                    n.addChild(child);
                    spacing = spacing + "\t";
                    getRootedTreeHelper(child, marked, spacing);
                }
            }
        }
    }

As requested : The relevant portions of Node Class are shown below

public class Node {

    private int id;
    ArrayList<Node> children;

    public Node(int id) {
        this.id = id;
        children = new ArrayList<Node>();
    }

    /**
     * add node n to this node's children
     * @param n
     */
    public void addChild(Node n) {
        children.add(n);
    }

    // getters and setters()
    public int getId() {
        return id;
    }
    public void setId(int id) {
        this.id = id;
    }
}

Does anyone have any ideas?

Edit solved : Rather than using a for each loop to iterate through all the children, I use a for loop.

Rohan
  • 1,312
  • 3
  • 17
  • 43
  • Show your Node class – Venkatesh Nov 20 '14 at 21:06
  • What type is n.children? Can you add the stacktrace? – flob Nov 20 '14 at 21:08
  • 5
    The same `ConcurrentModificationException` can also happen when you add to a list while iterating. – Jörn Horstmann Nov 20 '14 at 21:08
  • Relevant parts of Node class are shown now. @flob, n.children returns an array list of Node – Rohan Nov 20 '14 at 21:12
  • Question, why did you feel you needed to create your own `Node` class and not use a subclass of `org.w3c.dom.Node`? – hfontanez Nov 20 '14 at 21:16
  • You show the exception being thrown on the `continue` statement, which doesn't make sense. Are you sure it's not being thrown when you call `n.addChild`? – Vince Nov 20 '14 at 21:17
  • @hfontanez school project – Rohan Nov 20 '14 at 21:20
  • @Rohan, you will have to use two Nodes, the original and a new Node. You will have to make modifications to the new node and iterate over the original node. – hfontanez Nov 20 '14 at 21:23
  • @hfontanez there is always a good reason as `org.w3c.dom.Node` is just an interface, is dependent to a `org.w3c.dom.Document` (we are speaking XML?), has quite a lot of potentially unneeded methods, has way to many implementations, might not fit his special usecase, ... besides from needing a special node implementation for any kind of tree or graph.. :-) – flob Nov 20 '14 at 21:39
  • @flob "there is always a good reason"? You probably mean "there could be good reasons", but definitely not always. Otherwise, there would be no need for the subclasses `org.w3c.dom`provides for it. I was merely trying to engage the OP, that's all. I wasn't trying to arise a philosophical discussion about the matter. – hfontanez Nov 21 '14 at 00:06
  • @hfontanez I took into account the current context and that the poster had quite a small node class which didn't seem to need all that functionality.. So let me rephrase it: If you are not implementing the W3C Dom, you can always find a good reason not to use `org.w3c.dom.Node` as a starting point for your node. Even if you take a look at the Java Class Library there are many other implementations of a node. – flob Nov 21 '14 at 07:13

2 Answers2

4

If you take a look at the ArrayLists implementation of an Iterator, it shows that during Iterator.next() it checks if the size of the collection has changed.

if (i >= elementData.length)
  throw new ConcurrentModificationException();

Even using a synchronized version with Collections.synchronizedList(n.children) wouldn't help as it still uses the same Iterator.

So if you need to have concurrent access that modifies the collection you have some choices:

  • use Iterator.remove() to remove the current element,
  • use a version that allows concurrent modification like a ConcurrentLinkedQueue or ConcurrentLinkedDeque or
  • use another List for writing changes than for iterating.

You could try a LinkedList - i haven't read the source fully but a quick glance at its Iterator seems like it is immune to additions while iterating.

flob
  • 3,760
  • 2
  • 34
  • 57
2

From ConcurrentModificationException Javadoc:

Note that this exception does not always indicate that an object has been concurrently modified by a different thread. (...) For
example, if a thread modifies a collection directly while it is
iterating over the collection with a fail-fast iterator, the iterator will throw this exception.

The bug is adding a child into the collection while also iterating over it.

The fault is only detected by the iterator, when it is incremented in the for loop.

Luke Usherwood
  • 3,082
  • 1
  • 28
  • 35
  • This is the case. The problem is that, because he created his own rudimentary `Node` class, he doesn't have this facility. – hfontanez Nov 20 '14 at 21:19
  • This can be caused with any [failing] re-entrant access while iterating; threads are not required. – user2864740 Nov 20 '14 at 21:35