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

Jade Tang
  • 321
  • 4
  • 23
  • 1
    You should definitely declare `mainList` as `volatile`; otherwise it's possible that this won't do what you want it to. – Dawood ibn Kareem Aug 30 '14 at 01:42
  • @DavidWallace To be clear, the `volatile` keyword is used to indicate that a variable's value will be modified by different threads. Making the variable `volatile` doesn't affect whether or not the code can be considered "thread-safe". – Alex Lockwood Aug 30 '14 at 01:52
  • It is not thread safe because when you have multiple threads you will have a `ConcurrentModificationException` on `mainList.add(r.nextInt(200))`. – Luiggi Mendoza Aug 30 '14 at 01:59
  • @AlexLockwood in most cases I would agree with you, but in THIS PARTICULAR CASE, the lack of `volatile` is what makes this code not thread safe. If you study the code carefully, you'll see why. – Dawood ibn Kareem Aug 30 '14 at 01:59
  • It might be useful to state what this program should achieve. It is not clear at all, as the assignment of the new list in the 'cmd' thread is odd. – Michael Easter Aug 30 '14 at 02:06
  • @JadeTang Although not correct you are lucky that you are not getting concurrentmodificationexception because you are assigning mainList a new Arraylist object and in the other thread you are iterating through it. This exception will be thrown if you are structurally modifying and iterating at the same time. – Manjunath Aug 30 '14 at 17:42
  • I put some extract info about this code@MichaelEaster – Jade Tang Sep 01 '14 at 04:53

3 Answers3

2

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.

Dolda2000
  • 25,216
  • 4
  • 51
  • 92
  • When you say "Main thread fetches `mainList` reference", do you mean it copies the reference to the stack in preparation for the method call? – Dawood ibn Kareem Aug 30 '14 at 18:55
  • @DavidWallace: Indeed. At some point it has to be fetched from memory to a register (or, to speak in JVM bytecode, a point where a [`getfield`](http://docs.oracle.com/javase/specs/jvms/se7/html/jvms-6.html#jvms-6.5.getfield) instruction is executed). Whether this happens before or after the random number has been calculated is up to the compiler, but in either case there's a time window where it has been copied from memory. – Dolda2000 Aug 30 '14 at 18:57
0

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.

Enno Shioji
  • 26,542
  • 13
  • 70
  • 109
  • I don't think this is quite right. There's only one thread working with `mainList` at any one time, so there's no need to synchronise it. The only thing that stops this from being thread-safe is that `mainList` is not declared as `volatile`. You don't actually need a thread-safe list in this case. – Dawood ibn Kareem Aug 30 '14 at 01:45
  • 1
    @DavidWallace: "synchronization facilities" include `volatile`. Also, even if mainList is declared as volatile, because the "switch" of reference consists of two operation, the same array list maybe operated on by more than one thread and thus is not thread safe. – Enno Shioji Aug 30 '14 at 01:50
  • "synchronization facilities" refers generally to things that can be used to synchronize operations and includes stuff like CAS as well. – Enno Shioji Aug 30 '14 at 01:52
0

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.

squarewav
  • 383
  • 2
  • 8