NoSuchElementException occurs when Iterating through Java ArrayList concurrently

subjectIterator.hasNext();) {

--- Imagine a thread switch occurs here, at this point, between the call to hasNext() and next() methods.

addToCategory(subjectIterator.next().getId());

What could happen is the following, assuming you are at the last element in the list:

• thread A calls hasNext(), the result is true;
• thread switch occurs to thread B;
• thread B calls hasNext(), the result is also true;
• thread B calls next() and gets the next element from the list; now the list is empty because it was the last one;
• thread switch occurs back to thread A;
• thread A is already inside the body of the for loop, because this is where it was interrupted, it already called hasNext earlier, which was true;
• so thread A calls next(), which fails now with an exception, because there are no more elements in the list.

So what you have to do in such situations, is to make the operations hasNext and next behave in an atomic way, without thread switches occurring in between. A simple synchronization on the list solves, indeed, the problem:

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

Note, however, that there may be performance implications with this approach. No other thread will be able to read or write from/to the same list until the iteration is over (but this is what you want). To solve this, you may want to move the synchronization inside the loop, just around hasNext and next. Or you may want to use more sophisticated synchronization mechanisms, such as read-write locks.

It sounds like another thread is calling the method and grabbing the last element while another thread is about to get the next. So when the other thread finishes and comes back to the paused thread there is nothing left. I suggest using an ArrayBlockingQueue instead of a list. This will block threads when one is already iterating.

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

There is a bit of a wrinkle that you may have to sort out. The ArrayBlockingQueue will block if it is empty or full and wait for a thread to either insert something or take something out, respectively, before it will unblock and allow other threads to access.

You can use Collections.synchronizedList(list) if all you need is a simple invocation Sycnchronization. But do note that the iterator that you use must be inside the Synchronized block.

As I get you are adding elements to a list which might be under reading process. Imagine the list is empty and your other thread is reading it. These kinds of problems might lead into your problem. You could never be sure that an element is written to your list which you are trying to read , in this approach.

I was surprised not to see an answer involving the use of a CopyOnWriteArrayList or Guava's ImmutableList so I thought that I would add such an answer here.

Firstly, if your use case is such that you only have a few additions relative to many reads, consider using the CopyOnWriteArrayList to solve the concurrent list traversal problem. Method synchronization could solve your issue, but CopyOnWriteArrayList will likely have better performance if the number of concurrent accesses "vastly" exceeds the number of writes, as per that class's Javadoc.

Secondly, if your use case is such that you can add everything to your list upfront in a single-threaded manner and only then do you need iterate across it concurrently, then consider Guava's ImmutableList class. You accomplish this by first using a standard ArrayList or a LinkedList or a builder for your ImmutableList. Once your single-threaded data entry is complete, then you instantiate your ImmutableList using either ImmutableList.copyOf() or ImmutableList.build(). If your use case will allow for this write/read pattern, this will probably be your most performant option.

Hope that helps.

I would like to make a suggestion that would probably solve your problem, considering that this is a concurrency issue.

If making the method addSubjectsToCategory() synchronized solves your problem, then you have located where your concurrency issue is. It is important to locate where the problem occurs, otherwise the information you provided is useless to us, we can't help you.

IF using synchronized in your method solves your problem, then consider this answer as educational or as a more elegant solution. Otherwise, share the code where you implement your threading environment, so we can have a look.

public synchronized void addSubjectsToCategory(List subjectsList){
  Iterator iterator = subjectsList.iterator();
  while(iterator.hasNext())
    addToCategory(iterator.next().getId());
}

or

//This semaphore should be used by all threads. Be careful not to create a
//different semaphore each time.
public static Semaphore mutex = new Semaphore(1);

public void addSubjectsToCategory(List subjectsList){
  Iterator<Subject> iterator = subjectsList.iterator();
  mutex.acquire();
  while(iterator.hasNext())
    addToCategory(iterator.next().getId());
  mutex.release();
}

Synchronized is clean, tidy and elegant. You have a really small method and creating locks, imho is unnecessary. Synchronized means that only 1 thread will be able to enter the method at a time. Which means, you should use it only if you want 1 thread active each time.

If you actually need parallel execution, then your problem is not thread-related, but has something to do with the rest of your code, which we can not see.