1

Edit: Appearently the question has been marked a duplicate of this even though my question is about how to safely work with a list on one thread while another thread is modifying it, and the linked question is about how to remove during iterating.

Problem: I have a List<Object> objects in a class. This class also contains a getObjects() method which returns the object list.

I have two threads working with this list. One thread is adding objects every now and then, and the other thread is doing stuff with them and removing it from the list once it's done. The problem occurs when I'm iterating over the list in thread two, while thread one adds an object to the list at the same time.

I have attempted to counter this, by having thread two only work with a copy of the list, and not the actual one. Using List<Object> newList = getObjects();. Here I presumed that since newList is a different list(with the same objects though), that I could safely iterate over it and work with the objects. However it appears that this still causes the exception, because (I assume) the = operator just makes newList a reference to the objects list. And that I'm still iterating over the objects list.

Question: So my question is: How do I safely work with the objects from the list in thread two?

Code: Even though I doubt it'll be of use for answering the question, here's the code(I removed all unneccesary clutter thats irrelevant to this question).

public class One
{
    List<Object> objects;

    public One()
    {
        objects = new ArrayList<Object>();
    }
    public void addObject(Object object)
    {
        objects.add(object);
    }
    public List<Object> getObjects()
    {
        return objects;
    }
}

public class Two
{
    private One one;
    public Two(One one)
    {
        this.one = one;
    }
    public void processObjects()
    {
        List<Object> newList = one.getObjects();
        for(Object object : newList) //Causes error because during iteration thread 1 adds an object to the objects list.
        {
          //dostuff with the object.
        }
        one.getObjects().removeAll(newList);
    }
}
Community
  • 1
  • 1
JohnCake
  • 109
  • 8
  • A simple declaration of a list and an enhanced for loop is even more "reduced" than what you provided. `class One` doesn't need to exist to reproduce the problem – OneCricketeer Aug 21 '16 at 01:33
  • 1
    Using an implementation of `BlockingQueue` would be better in this case, where one thread pushes objects in, and the other thread pops and works on them. It will also ensure safe publication of objects between the threads. If you still want to use a `List`, take a look at `CopyOnWriteArrayList` – rohitvats Aug 21 '16 at 01:34
  • 1
    @Makoto The "duplicate" is not really a duplicate of this one. It refers to **single** threaded code. Although I assume that there are others that are *really* a duplicate of this one, and involve `CopyOnWriteArrayList` or maybe some ReadWriteLock trickery (I did not yet search for one). – Marco13 Aug 21 '16 at 01:39
  • 2
    To point out one basic (important) error: `List newList = one.getObjects();` does **NOT** create a copy of the list. It just creates a new reference to the **SAME** list. With something like `List newList = new ArrayList(one.getObjects());`, it would create a real new list, **but for multithreaded code, even this will not be sufficient**! – Marco13 Aug 21 '16 at 01:40
  • @Marco13 I've tested a bit, and so far creating the new list seems to be working as I get no more exceptions. But since you said this isn't sufficient for multi-threaded code, is there a short explanation as to why not or why I shouldn't be using this? – JohnCake Aug 21 '16 at 01:49
  • 2
    Yes, creating the copy may cause the exceptions to become rare (or even seem to disappear). But it is still not guaranteed that the other thread may not add a new element **while** the copy is created, which may still lead to an inconsistent state. The "best" solution here depends on many factors (size of the list, how often it is modified or read, performance requirements etc). A simple solution would be to make the list a `objects = new CopyOnWriteArrayList();`, but this might not be appropriate when the list is large and/or modified frequently. – Marco13 Aug 21 '16 at 01:54
  • 1
    Creating a copy of the list is not sufficient in multi-threaded code, because the second thread might never see the objects added by the first thread, because you are not using any kind of synchronization. The Java memory model only guarantees memory visibility between threads if you use some form of synchronization. If you change `objects = new ArrayList();` to `objects = new CopyOnWriteArrayList();`, it will work, because `CopyOnWriteArrayList` ensures memory visibility, and you wouldn't have to create a copy in the second thread. – rohitvats Aug 21 '16 at 01:55
  • That works good enough for me. Thanks for the help! Don't think I can mark a comment as answer though. – JohnCake Aug 21 '16 at 02:13
  • There are several (many) related questions, but unfortunately, hardly a nice, canonical answer... – Marco13 Aug 21 '16 at 02:34
  • Use a ReentrantReadWriteLock to protect your read and write operations. This allows you to perform multiple reads on the List but when a writeLock is acquired no one else can acquire a readLock. – garnulf Aug 21 '16 at 02:52

0 Answers0