List.Add() Thread Safety

List.Add() thread safety

Behind the scenes lots of things happen, including reallocating buffers and copying elements. That code will cause danger. Very simply, there are no atomic operations when adding to a list, at the least the "Length" property needs to be updates, and item needs to be put in at the right location, and (if there's a separate variable) the index needs to be updated. Multiple threads can trample over each other. And if a grow is required then there is lots more going on. If something is writing to a list nothing else should be reading or writing to it.

In .NET 4.0 we have concurrent collections, which are handily threadsafe and don't require locks.

Java ArrayList.add() method thread safe for purely parallel adding?

No, it's not thread-safe. Wrap your list using Collections.synchronizedList(), or use explicit synchronization when accessing the list.

Synchronized List - add multiple threads and remove one thread

It appears to me that List is not the class you're looking for. Java actually has a Queue class (and a thread safe implementation of it).

Then, looking further into your code, I can see that it's very wasteful;
you're creating a copy of the List, removing the object from the original and then printing the copy.
That seems like a CopyOnWriteArrayList but less thread safe to me. This can be useful, but the Queue solution would be best suitable in this case and more performant.

This is what your code would look like after changing it to use BlockingQueue:

private static boolean flagStop = false;

//synchronized list
private static Queue<Object> queue = new BlockingQueue<Object>();

//thread which check once per second the size of queue
//and print elements from list and try to remove them safely
private static Thread printQueueThread = new Thread(() -> {
    Object currentObject;
    while(!flagStop) {
            while((object = queue.poll()) != null) {
                System.out.println(currentObject);
            }
        }
        try { 
            Thread.sleep(1_000); 
        } catch (Exception ignored) {
        }
    }
});

static{
    printQueueThread.start();
}

//add elements in queue
//this is accessed by multiple threads, by few times per second.
public static void addElement(Object obj){
    queue.offer(obj);
}

On the other hand, you're also using a boolean to tell your thread whether it should stop or not. That can cause synchronization issues. You should really replace it with AtomicBoolean instead - a thread safe implementation of boolean.
Changing boolean flagStop = false to AtomicBoolean flagStop = new AtomicBoolean() and while (!flagStop) to while(!flagStop.get()).

I'm not really sure as to why your class is also all static, but that's generally bad practice. I would avoid that and use instances.
I would also suggest implementing Runnable, making the code more clear and adding a null check in the addElement.
With all the corrections your final class would look like this:

public class MyPrintQueue implements Runnable {
    private AtomicBoolean shouldStop;

    //synchronized list
    private Queue<Object> queue;
    
    //thread which check once per second the size of queue
    //and print elements from list and try to remove them safely
    private Thread printQueueThread;
    
    public MyPrintQueue() {
        this.queue = new BlockingQueue<>();
        this.printQueueThread = new Thread(this);
    }

    @Override
    public void run() {
        Object currentObject;
        while(!flagStop.get()) {
                while((object = queue.poll()) != null) {
                    System.out.println(currentObject);
                }
            }
            try { 
                Thread.sleep(1_000); 
            } catch (Exception ignored) {
                // Ignored
            }
        }
    }

    public void startThread() {
        printQueueThread.start();
    }

    public void stop() {
        flagStop.set(true);
    }

    //add elements in queue
    //this is accessed by multiple threads, by few times per second.
    public void addElement(Object obj){
        if (obj == null)
            return;
        queue.offer(obj);
    }
}

---

That being said, your code doesn't seem completely thread safe to me. I might be wrong, but you could remove the Thread.sleep call and create a TestClass which calls addElement() in a while true to check if any exception is ever thrown.
If you don't want to use Queue and must use a List, you should then replace Collections.synchronizedList with a CopyOnWriteArrayList<>(), effectively changing your code to this

private static boolean flagStop = false;

//synchronized list
private static List<Object> queue = new CopyOnWriteArrayList<>();

//thread which check once per second the size of queue
//and print elements from list and try to remove them safely
private static Thread printQueueThread = new Thread(() -> {

    while(!flagStop){

        if(!queue.isEmpty()){
            while (queue.size() > 0) {
                System.out.println(queue.get(0));
                queue.remove(0);
            }
        }

        try{ Thread.sleep(1_000); }catch(Exception ignored){}

    }

});

static{
    printQueueThread.start();
}

//add elements in queue
//this is accessed by multiple threads, by few times per second.
public static void addElement(Object obj){
    queue.add(obj);
}

Note that the code above is still static (!) and does not implement AtomicBoolean.
Also take a look at the following question and its answers which seem to be related to yours: Is externally synchronized ArrayList thread safe if its fields are not volatile?

Adding to a list in a Parallel.ForEach loop in a threadsafe manner

Is this because my NewListofObjects.Add(newobj) method is not threadsafe?

Correct. It is not threadsafe.

Any instance members are not guaranteed to be thread safe.

That's from MSDN referring to List<T> (scroll to the section titled "Thread Safety").

If so, how can I make it threadsafe?

Use a concurrent collection, like ConcurrentBag<T>. Note that you lose the ability to keep track of the order that items were inserted.

Why does List.Add from multiple threads lead to varying results?

If you check the source code of List you will see that internally it operates on array. The Add method expands array size and inserts new item:

// Adds the given object to the end of this list. The size of the list is
// increased by one. If required, the capacity of the list is doubled
// before adding the new element.
//
public void Add(T item) 
{
   if (_size == _items.Length) EnsureCapacity(_size + 1);
      _items[_size++] = item;
   _version++;
 }

Now imagine you have array with size of 10 and 2 threads inserts at the same time - both expands array to 11 and one thread inserts at index 11 and other overwrites item at index 11. And that's why you get list count of 11 not 12 and you will loose one item.

Concurrent threads adding to ArrayList at same time - what happens?

There is no guaranteed behavior for what happens when add is called concurrently by two threads on ArrayList. However, it has been my experience that both objects have been added fine. Most of the thread safety issues related to lists deal with iteration while adding/removing. Despite this, I strongly recommend against using vanilla ArrayList with multiple threads and concurrent access.

Vector used to be the standard for concurrent lists, but now the standard is to use the Collections synchronized list.

Also I highly recommend Java Concurrency in Practice by Goetz et al if you're going to be spending any time working with threads in Java. The book covers this issue in much better detail.

Are lists thread-safe?

Lists themselves are thread-safe. In CPython the GIL protects against concurrent accesses to them, and other implementations take care to use a fine-grained lock or a synchronized datatype for their list implementations. However, while lists themselves can't go corrupt by attempts to concurrently access, the lists's data is not protected. For example:

L[0] += 1

is not guaranteed to actually increase L[0] by one if another thread does the same thing, because += is not an atomic operation. (Very, very few operations in Python are actually atomic, because most of them can cause arbitrary Python code to be called.) You should use Queues because if you just use an unprotected list, you may get or delete the wrong item because of race conditions.

---

Related Topics