java - Thread safety when iterating through an ArrayList using foreach

Question

I've got an ArrayList which is being instantiated and populated on the background thread (I use it to store the Cursor data). At the same time it can be accessed on the main thread and iterated through using foreach. So this obviously may result in throwing an exception.

My question is what's the best practice to make this class field thread-safe without copying it every time or using flags?

class SomeClass {

    private final Context mContext;
    private List<String> mList = null;

    SomeClass(Context context) {
        mContext = context;
    }

    public void populateList() {
        new Thread(new Runnable() {
            @Override
            public void run() {
                mList = new ArrayList<>();

                Cursor cursor = mContext.getContentResolver().query(
                        DataProvider.CONTENT_URI, null, null, null, null);
                try {
                    while (cursor.moveToNext()) {
                        mList.add(cursor.getString(cursor.getColumnIndex(DataProvider.NAME)));
                    }
                } catch (Exception e) {
                    Log.e("Error", e.getMessage(), e);
                } finally {
                    if (cursor != null) {
                        cursor.close();
                    }
                }
            }
        }).start();
    }

    public boolean searchList(String query) { // Invoked on the main thread
        if (mList != null) {
            for (String name : mList) {
                if (name.equals(query) {
                    return true;
                }
            }
        }

        return false;
    }
}

Answer 1

In general it is a very bad idea to operate concurrently on a datastructure that is not thread-safe. You have no guarantee that the implementation will not change in the future, which may severly impact the runtime behavior of the application, i.e. java.util.HashMap causes infinite loops when being concurrently modified.

For accessing a List concurrently, Java provides the java.util.concurrent.CopyOnWriteArrayList. Using this implementation will solve your problem in various ways:

• it is thread safe, allowing concurrent modifications
• iterating over snapshots of the list is not affected by concurrent add operations, allowing concurrently adding and iterating
• it's faster than synchronization

Alternatively, if not using a copy of the internal array is a strict requirement (which I can't imagine in your case, the array is rather small as it only contains object references, which can be copied in memory quite efficiently), you may synchronize the access on the map. But that would require the Map to be initialized properly, otherwise your code may throw a NullPointerException because the order of thread-execution is not guaranteed (you assume the populateList() is started before, so the list gets initialized. When using a synchronized block, choose the protected block wisely. In case you have the entire content of the run() method in a synchronized block, the reader thread has to wait until the results from the cursor are processed - which may take a while - so you actually loose all concurrency.

If you decide to go for the synchronized block, I'd make the following changes (and I don't claim, they are perfectly correct):

Initialize the list field so we can synchronize access on it:

private List<String> mList = new ArrayList<>(); //initialize the field

Synchronize the modification operation (add). Do not read the data from the cursor inside the synchronization block because if its a low latency operation, the mList could not be read during that operation, blocking all other threads for quite a while.

//mList = new ArrayList<>(); remove that line in your code
String data = cursor.getString(cursor.getColumnIndex(DataProvider.NAME)); //do this before synchronized block!
synchronized(mList){
  mList.add(data);
}

The read iteration must be inside the synchronization block, so no element gets added, while being iterated over:

synchronized(mList){ 
  for (String name : mList) {
    if (name.equals(query) {
      return true;
    }
  }
}

So when two threads operate on the list, one thread can either add a single element or iterate over the entire list at a time. You have no parallel execution on these parts of the code.

Regarding the synchronized versions of a List (i.e. Vector, Collections.synchronizedList()). Those might be less performant because with synchronization you actually lose prallel execution as only one thread may run the protected blocks at a time. Further, they might still be prone to ConcurrentModificationException, which may even occur in a single thread. It is thrown, if the datastructure is modified between iterator creation and iterator should proceed. So those datastructures won't solve your problem.

I do not recommend manualy synchronization as well, because the risk of simply doing it wrong is too high (synchronizing on the wrong or different monitory, too large synchronization blocks, ...)

TL;DR

use a java.util.concurrent.CopyOnWriteArrayList