CODEWITHSUNDEEP
javamultithreadingcollections
Rupesh
Rupesh

Reputation: 388

ArrayList is not Thread safe even with synchronized block?

I was trying Producer and Consumer problem using ArrayList(I know Arraylist isnt threadsafe), and I made sure that i put the list with synchronized keyword but still got into ConcurrentModificationException. This is the error

Starting Producer Please provide the job details: TestJob Job completed: TestJob Exception in thread "consumer" java.util.ConcurrentModificationException at java.util.ArrayList$Itr.checkForComodification(Unknown Source) at java.util.ArrayList$Itr.next(Unknown Source) at test.thread.Consumer.run(Consumer.java:25) at java.lang.Thread.run(Unknown Source)

Here's my code:

    package test.thread;

import java.util.List;
import java.util.Scanner;

public class Producer implements Runnable {

    private List<String> jobs = null;

    public Producer(List<String> jobs) {
        this.jobs = jobs;
    }

    @Override
    public void run() {

        System.out.println("Starting Producer");
        while(true)
        {
            synchronized (jobs) {
                try {
                if (jobs.isEmpty()) {


                        System.out.println("Please provide the job details: ");
                        Scanner scanner = new Scanner(System.in);
                        String job = scanner.nextLine();
                        jobs.add(job);
                        scanner.close();
                        jobs.notifyAll();
                        Thread.sleep(4000);

                } else {
                    jobs.wait();
                }
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }

            }
        }

    }

}


package test.thread;

import java.util.List;

public class Consumer implements Runnable {

    private List<String> jobs = null;

    public Consumer(List<String> list) {
        this.jobs = list;
    }

    @Override
    public void run() {

        while(true)
        {
            synchronized (jobs) {
                try {
                    if (jobs.isEmpty()) {

                        jobs.wait();

                    } else {
                        for (String job : jobs) {
                            System.out.println("Job completed: " + job);
                            jobs.remove(job);
                            Thread.sleep(2000);
                        }

                        jobs.notifyAll();
                    }
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }

            }

        }

    }

}



package test.thread;

import java.util.ArrayList;
import java.util.List;

public class TestThread {

    public static void main(String...strings )
    {

        List<String> list = new ArrayList<String>();
        Producer producer = new Producer(list);
        Consumer consumer = new Consumer(list);
        Thread prodThread = new Thread(producer, "producer");
        Thread consThread = new Thread(consumer, "consumer");
        prodThread.start();
        consThread.start();
    }




}

Upvotes: 0

Views: 467

Answers (6)

ankit249
ankit249

Reputation: 452

Can't remove elements while, please use the iterator method to remove.

Iterator<String> it = jobs.iterator();
while(it.hasNext()) {
      String job = it.next();
      ....
      it.remove();
      ...
}

This will avoid concurrentModificationException.

Upvotes: 0

Cootri
Cootri

Reputation: 3836

You should use CopyOwWriteArrayList in order to prevent ConcurrentModificationException while using list Iterator (for-each loop in your code uses it under the hood)

Upvotes: 0

Joachim Sauer
Joachim Sauer

Reputation: 308259

Although you get a ConcurrentModificationException your problem is not caused by multi-threading at all.

In fact this is the problematic code:

for (String job : jobs) {
    System.out.println("Job completed: " + job);
    jobs.remove(job);
    Thread.sleep(2000);
}

The for loop will use an Iterator over jobs and at the same time you directly (structurally) modify jobs (i.e. not via the Iterator). This means that the Iterator is no longer well-defined and it throws this exception to tell you about it.

If you need to remove elements from a collection while you iterate over it, you'll need to make the Iterator explicit:

Iterator<String> jobIterator = job.iterator();
while (jobIterator.hasNext()) {
  String job = jobIterator.next();
  // ... stuff ...
  jobIterator.remove(); // remove the last object that was returned by next()
}

Upvotes: 2

josefx
josefx

Reputation: 15656

Your concurrent modification is not caused by threading.

    for (String job : jobs) {
                        System.out.println("Job completed: " + job);
                        jobs.remove(job);
                        Thread.sleep(2000);
                    }

The line jobs.remove(job); modifies the ArrayList while you are iterating over it. Iterators only allow modification of the list when done using the Iterators remove method for this.

     for( Iterator<String> iter = jobs.iterator(); iter.hasNext(); )
     {
           String job = iter.next();
           iter.remove();
     }

Upvotes: 2

AdamSkywalker
AdamSkywalker

Reputation: 11619

You get ConcurrentModificationException not because of concurrency: you simply remove item in a loop:

for (String job : jobs) {
     System.out.println("Job completed: " + job);
     jobs.remove(job);

Use iterator to remove items.

Upvotes: 1

Andy Turner
Andy Turner

Reputation: 140534

You cannot remove items from a list that you are iterating with an enhanced for statement.

If you want to remove elements, you need an explicit iterator on which to call remove:

Iterator<String> it = jobs.iterator();
while (it.hasNext()) {
  String job = it.next();
  System.out.println("Job completed: " + job);
  it.remove();
  Thread.sleep(2000);
}

Of course, removing items from an ArrayList like this is very inefficient (it is O(n^2)), and error-prone since you have to explicitly synchronize. You should look into using some sort of BlockingQueue instead.

Upvotes: 2

Related Questions