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

"if (this. remove ItemEvent) { this.itemList. add (this.item);" ? — Fildor
– Fildor, Commented Feb 2, 2017 at 10:31
Using synchronized(this)and the synchronized keyword on methods do not use the same synchronization object. — Fildor
– Fildor, Commented Feb 2, 2017 at 10:35
Also, you are leaking the critical resource. Synchronization only happens while returning the reference to the list. Access through that reference is then not synchronized anymore. You can avoid this by returning a copy of the list or expanding your API so that it is not necessary to give away direct access to the list. — Fildor
– Fildor, Commented Feb 2, 2017 at 10:40

Andy Turner · Accepted Answer · 2017-02-02 10:55:23Z

6

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".

edited Feb 2, 2017 at 10:55

answered Feb 2, 2017 at 10:42

Andy Turner

141k11 gold badges169 silver badges263 bronze badges

Sign up to request clarification or add additional context in comments.

4 Comments

Fildor Over a year ago

Would you agree that having a method getItemList isn't much of a good idea in the first place? It is an invitation to errors like that. (Assuming the method does not return a copy).

Andy Turner Over a year ago

@Fildor yes, I would.

Ashish Ratan Over a year ago

isn't it a performance issue using ItemTask object as synchronized

Andy Turner Over a year ago

@AshishRatan it might be; but there is no point in worrying if code is performant if it isn't correct first.

glee8e · Accepted Answer · 2017-02-02 11:17:44Z

0

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.

edited Feb 2, 2017 at 11:17

answered Feb 2, 2017 at 10:45

glee8e

6,4795 gold badges34 silver badges52 bronze badges

5 Comments

Ashish Ratan Over a year ago

But I am updating my list only on a specific condition, and I made that volatile as well. when the boolean true add otherwise not.

glee8e Over a year ago

@AshishRatan But there is nothing that will guarantee that these two operations won't take place concurrently. Actually, because of the freaking contention you are making, this is very likely to happen even just in a few tries.

Ashish Ratan Over a year ago

Ok, then what condition will be safe? apart from getItemList() method, what may be other factors for the issue?

glee8e Over a year ago

@AshishRatan 1. As stated above, access to that list should be synchronized or protected by locks. Here only two thread is accessing the list. One thread updating the ItemTask, the other receiving and executing user command, so no need for ReadWriteLock. Whenever the main thread want to print the list, it should acquire a lock. Whenever the ItemTask thread want to modify, it should acquire that lock too. 2. No, because there isn't any other iteration in your code.

Ashish Ratan Over a year ago

What i could understand from the case is: a thread update the list, and go for sleep, but in my main method, the place i am accessing this list is running immediate and trying to access the resource. Could it be the reason behind this exception? I am confused, why it shoild happen? I made the changes to the refenece and these are saved on object, it should take time to properly applied to the refrence when the thread wakes up.

Mahesh · Accepted Answer · 2017-02-02 12:18:41Z

0

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.

answered Feb 2, 2017 at 12:18

Mahesh

3121 silver badge14 bronze badges

1 Comment

Ashish Ratan Over a year ago

java.util.ArrayList$Itr.checkForComodification(ArrayList.java:901)]. is summary of your answer. Now the part is coming for printing the list Inside the itemTask, I am just wanted to understand the mechanism for synchronized block working in java. for a good answer you can go with @Andy's answer

Collectives™ on Stack Overflow

ConcurrentModificationException in Multiple Threading access

3 Answers 3

4 Comments

5 Comments

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

4 Comments

5 Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related