CODEWITHSUNDEEP
javamultithreadingthread-safety
Jade Tang
Jade Tang

Reputation: 319

Is this piece of Java code thread safe?

public class TestConcurrentForList {

List<Integer> mainList = new ArrayList<Integer>();
ScheduledExecutorService scheduledExecutorService = Executors.newScheduledThreadPool(1);
Random r = new Random();

public void start() throws InterruptedException {
    Runnable cmd = new Runnable() {
        @Override
        public void run() {
            List<Integer> tempList = mainList;
            mainList = new ArrayList<Integer>();
            for (Integer i: tempList) {
                System.out.println("subThread:" + i);
            }
        }
    };
    scheduledExecutorService.scheduleAtFixedRate(cmd, 1, 1, TimeUnit.MILLISECONDS);
    while (true) {
        mainList.add(r.nextInt(200));
        Thread.sleep(100);
    }
}

public static void main(String[] args) {
    TestConcurrentForList tester = new TestConcurrentForList();
    try {
        tester.start();
    } catch (Exception e) {
        e.printStackTrace();
        System.err.println(e.getMessage());
    }
}

}

Part of our product code likes this, the main thread and subthread share the mainList. I run the test serval times but never reproduce the ConcurrentModificationException.

update：

thanks for all your replying，this code is actually a brief abstraction of our production code. What I wanna do actually is very simply:

the main thread hold a list to receive data from some source, when the list reaches a certain size the main thread pass the list to a sub thread which stores the data to a data base.

Maybe a more safer way to accomplish this is to extract the 

List<Integer> tempList = mainList;
mainList = new ArrayList<Integer>();

to the main thread, and pass the templist to sub thread. The code I list before is a legacy code, and I want to fix this code.

Upvotes: 1

Views: 132

Answers (3)

Dolda2000
Dolda2000

Reputation: 25855

As David Wallace points out, you need to at least declare mainList as volatile.

However, that alone does not actually make the code thread-safe. Even though you're switching the reference in the cmd thread, the main thread may have already fetched the reference before that happens and can then proceed to work on it at the same time as the cmd thread reads from it.

For example, this is a possible sequence of events:

  1. cmd thread fetches mainList reference and gets list A
  2. Main thread fetches mainList reference and also gets list A
  3. cmd thread creates the new list, B, and assigns it to mainList
  4. Main thread starts calculating a random number
  5. cmd thread starts iterating over list A
  6. Main thread adds its random number to list A
  7. cmd thread continues to iterate over the modified list, now in an inconsistent state due to concurrent modification

EDIT: At this point, I was planning to edit in a suggestion to do what you wanted, but I realized there could be several quite different things that you wanted to do with this code, so a single suggestion would just be a guess anyway. If you want a solution, I suggest you start a new question describing your goal in more detail.

Upvotes: 2

squarewav
squarewav

Reputation: 423

No, I do not think the code is thread safe because the main thread could call List.add while the pool thread is assigning a new value to mainList. If mainList where a primative, it might be sufficient to make it 'volatile'. But I don't think you can use 'volatile' with object references.

To make the assignment safe, you would need to synchronize on something while you make the assignment and then also wherever you try to touch mainList like:

  Object lock = new Object();
  ...
        synchronized (lock) {
            mainList = new ArrayList<Integer>();
        }
  ...
        synchronized (lock) {
            mainList.add(r.nextInt(200));
        }

This would ensure that the pool thread could not reassign mainList while the main thread was in the process of calling add().

But I'm not sure if you can get a ConcurrentModificationException if only the main thread modifies the list and the pool thread is only iterating through elements. And even if the pool thread did modify the list I'm still not sure if you could get a CME if the pool thread modified a new list that has not yet been assigned to mainList.

So if you are seeing a CME, I suspect your test does not really represent what is happening in production.

Upvotes: 0

Enno Shioji
Enno Shioji

Reputation: 26882

No it's not thread safe. You are not using any synchronization facilities around mainList. The fact that the code didn't throw ConcurrentModificationException does not imply that the code is thread-safe. It merely means you might have a race condition if it's thrown.

Upvotes: 0

Related Questions