NoSuchElementException occurs when Iterating through Java ArrayList concurrently

Question

I have a method similar to the one below:

public void addSubjectsToCategory() {
    final List<Subject> subjectsList = new ArrayList<>(getSubjectList());
    for (final Iterator<Subject> subjectIterator =
            subjectsList.iterator(); subjectIterator.hasNext();) {
         addToCategory(subjectIterator.next().getId());
    } 
}

When this runs concurrently for the same user (another instance), sometimes it throws NoSuchElementException. As per my understanding, sometimes subjectIterator.next() get executed when there are no elements in the list. This occurs when being accessed only. Will method synchronization solve this issue?

The stack trace is:

java.util.NoSuchElementException: null
at java.util.ArrayList$Itr.next(Unknown Source)
at org.cmos.student.subject.category.CategoryManager.addSubjectsToCategory(CategoryManager.java:221)

This stack trace fails at the addToCategory(subjectIterator.next().getId()); line.

Answer 1

The basic rule of iterators is that underlying collection must not be modified while the iterator is being used.

If you have a single thread, there seems to be nothing wrong with this code as long as getSubjectsList() does not return null OR addToCategory() or getId() have some strange side-effects that would modify the subjectsList. Note, however, that you could rewrite the for-loop somewhat nicer (for(Subject subject: subjectsList) ...).

Judging by your code, my best guess is that you have another thread which is modifying subjectsList somewhere else. If this is the case, using a SynchronizedList will probably not solve your problem. As far as I know, synchronization only applies to List methods such as add(), remove() etc., and does not lock a collection during iteration.

In this case, adding synchronized to the method will not help either, because the other thread is doing its nasty stuff elsewhere. If these assumptions are true, your easiest and safest way is to make a separate synchronization object (i.e. Object lock = new Object()) and then put synchronized (lock) { ... } around this for loop as well as any other place in your program that modifies the collection. This will prevent the other thread from doing any modifications while this thread is iterating, and vice versa.