ConcurrentModificationException in Multiple Threading access

Question

Just started learning Multithreading and stuck on a situation for concurrent modification.

Here is my Java Class

package ashish.demo.threading.basic;

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

/**
 * Created by ashishratan on 2/2/17.
 */
public class ItemTask implements Runnable {

    private volatile boolean shutdown;
    private List<Item> itemList = new ArrayList<Item>();
    private volatile Item item;
    private volatile boolean addItemEvent;
    private volatile boolean removeItemEvent;

    @Override
    public void run() {
        while (!this.shutdown) {
            try {
                synchronized (this) {
                    if (this.item != null) {
                        this.item.setProductName("Created By:: " + Thread.currentThread().getName());
                    }
                    if (this.addItemEvent) {
                        this.itemList.add(this.item);
                        this.item=null;
                        this.addItemEvent = false;
                        this.statusDisplay();

                    }
                    if (this.removeItemEvent) {
                        this.itemList.add(this.item);
                        this.removeItemEvent = false;
                        this.statusDisplay();
                    }
                }
                Thread.sleep(100);
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }

        System.out.println("Shutting down...");
    }


    public void addItem(Item item) {
        this.item = item;
        this.addItemEvent = true;
    }

    public synchronized List<Item> getItemList() {
        this.statusDisplay();
        return itemList;
    }

    public void setItemList(List<Item> itemList) {
        this.itemList = itemList;
    }

    public synchronized void shutdownHook() {
        this.statusDisplay();
        this.shutdown = true;
        System.out.println(this.getItemList());
    }

    private synchronized void statusDisplay() {

        System.out.println(Thread.currentThread());
        System.out.println("Total Items In Stock are " + this.itemList.size());
    }
}

Runner Class

    package ashish.demo.threading;

    import ashish.demo.threading.basic.Item;
    import ashish.demo.threading.basic.ItemTask;

    import java.util.Scanner;

    public class Main {

        public static void main(String[] args) {

            ItemTask itemTask = new ItemTask();
            Thread thread =null;

            for (int i = 0; i < 500; i++) {
                thread=new Thread(itemTask);
                thread.setName("ItemTask-Thread-"+(i+1));
                thread.setPriority(Thread.MAX_PRIORITY);
                thread.start();
            }
            System.out.println("Please Enter Number (0) to exit");
            Scanner scanner = new Scanner(System.in);
            int i = scanner.nextInt();
            while (i>0){
                itemTask.addItem(new Item(1,12.0f,"Product "+i,(byte)12));
                System.out.println(itemTask.getItemList()); // Line #26, Exception
                System.out.println("Please Enter Number (0) to exit");
                i = scanner.nextInt();
            }
            System.out.println("EXIT");
            itemTask.shutdownHook();
        }
    }


    package ashish.demo.threading.basic;

    import java.io.Serializable;

    /**
     * Created by ashishratan on 2/2/17.
     */
    public class Item implements Serializable {

        private Integer orderId;
        private Float price;
        private String productName;
        private byte category;

        public Item(Integer orderId, Float price, String productName, byte category) {
            this.orderId = orderId;
            this.price = price;
            this.productName = productName;
            this.category = category;
        }

        public Item() {
        }

        public Integer getOrderId() {
            return orderId;
        }

        public void setOrderId(Integer orderId) {
            this.orderId = orderId;
        }

        public Float getPrice() {
            return price;
        }

        public void setPrice(Float price) {
            this.price = price;
        }

        public String getProductName() {
            return productName;
        }

        public void setProductName(String productName) {
            this.productName = productName;
        }

        public byte getCategory() {
            return category;
        }

        public void setCategory(byte category) {
            this.category = category;
        }

        @Override
        public String toString() {
            return "Item{" +
                    "orderId=" + orderId +
                    ", price=" + price +
                    ", productName='" + productName + '\'' +
                    ", category=" + category +
                    '}';
        }


        @Override
        public boolean equals(Object o) {
            if (this == o) return true;
            if (!(o instanceof Item)) return false;

            Item item = (Item) o;

            if (getCategory() != item.getCategory()) return false;
            if (getOrderId() != null ? !getOrderId().equals(item.getOrderId()) : item.getOrderId() != null) return false;
            if (getPrice() != null ? !getPrice().equals(item.getPrice()) : item.getPrice() != null) return false;
            return getProductName() != null ? getProductName().equals(item.getProductName()) : item.getProductName() == null;
        }

        @Override
        public int hashCode() {
            int result = getOrderId() != null ? getOrderId().hashCode() : 0;
            result = 31 * result + (getPrice() != null ? getPrice().hashCode() : 0);
            result = 31 * result + (getProductName() != null ? getProductName().hashCode() : 0);
            result = 31 * result + (int) getCategory();
            return result;
        }
    }

Exception Trace

    Please Enter Number (0) to exit
    3
    Thread[main,5,main]
    Total Items In Stock are 0
    []
    Please Enter Number (0) to exit
    Thread[ItemTask-Thread-455,10,main]
    Total Items In Stock are 1
    6
    Thread[main,5,main]
    Total Items In Stock are 1
    Thread[ItemTask-Thread-464,10,main]
    Total Items In Stock are 2
    Exception in thread "main" java.util.ConcurrentModificationException
        at java.util.ArrayList$Itr.checkForComodification(ArrayList.java:901)
        at java.util.ArrayList$Itr.next(ArrayList.java:851)
        at java.util.AbstractCollection.toString(AbstractCollection.java:461)
        at java.lang.String.valueOf(String.java:2994)
        at java.io.PrintStream.println(PrintStream.java:821)
        at ashish.demo.threading.Main.main(Main.java:26)
    12

Answer 1

Program is not throwing Concurrent Modification Exception consistently if input are printed slowly. But it is throwing it quite regularly if input is given very fast.

As per my understanding, Root cause: Though getItemList method in ItemTask is synchronized it just returns the collection to Main class. Iteration of the collection is done in Main method which is not synchronized and in mean time if other thread puts any element in collection, iterator validates if there is any modification to collection and if there is any modification it throws exception [evident from stack trace at java.util.ArrayList$Itr.checkForComodification(ArrayList.java:901)].

If you move printing of itemList to Itemtask in getItemList method or new synchronize method in Itemtask to print list, error will not occur at least it didn't occurred for me.

Answer 2

You were printing the item list, which involved iterating the list with a Iterator, while there was a structural modification happening (adding the item) in a ItemTask thread. The iterator consider this bad, and thus stopped the world with an exception. This is known as fail-fast behavior. You will have to use a synchronized list(which is bad) or make a lock/mutex that every thread that want to modify or read should acquire.

BTW, there is some extreme contention happening out there because you used as many as 500 threads to update the state of one single object.

Answer 3

The advice in Java Concurrency In Practice is: "Beware of implicit iteration". You have implicit iteration on the line:

System.out.println(itemTask.getItemList());

because this list needs to be iterated in order to convert it to a string.

The fact that itemTask.getItemList() is synchronized is irrelevant - that monitor is only held for the duration of the call to itemTask.getItemList(): once the result of that is returned, the monitor is no longer held, meaning that the monitor isn't held when you pass that result to System.out.println.

In order to ensure that you have exclusive access to the item list while you print it, explicitly synchronize on itemTask:

synchronized (itemTask) {
  System.out.println(itemTask.getItemList());
}

This will correctly hold the monitor for the duration of the System.out.println call; the modifications in the itemTask cannot happen at the same time, because those take place in a synchronized (this) block, where "this" is the itemTask that you are externally synchronizing on.

An alternative to this would be to return a defensive copy of the list from the getItemList method:

public synchronized List<Item> getItemList() {
    this.statusDisplay();
    return new ArrayList<>(itemList);
}

This would remove the need for external synchronization on the call to getItemList, which is a safer design, because you're not relying upon clients of your class to "do the right thing".