java.util.ConcurrentModificationException - ArrayList

Question

START EDIT please scroll down for the updated code END OF EDIT

I've google and searched around SO for why this exception is occurring and I understand that it is caused by an object is reading a list and meanwhile an item was removed from the list.

I've changed my code accordingly to the suggestions I've found but from time to time I still get this exception and it is crashing my app. And it looks randomly, I try to replicate the exception and 90% of the time I don't get the exception and not always following the same procedure, which makes it hard to debug.

I'm using the observer pattern. Sometimes it happens with the unregister method, some othertimes with the register, other times with a method from the notify... it's pretty random to where it happens.

I'm using an android asynctask to download few bytes from my server and the observer pattern is to update the GUI when needed.

Here's my code:

@Override
    public void register(final Observer newObserver) {
        Log.d(TAG, "(Register) Observer registred: " + newObserver.toString());
        observers.add(newObserver);

        Log.d(TAG, "(Register) Number of registered observers: " + observers.size());

    }

    @Override
    public void unregister(final Observer observer) {

        int indexObersver = observers.indexOf(observer);

        // Avoid java.util.ConcurrentModificationException 
        // at java.util.ArrayList$ArrayListIterator.next(ArrayList.java)

        if(indexObersver >= 0)
        {
            observers.remove(indexObersver);
            Log.d(TAG, "(Unregister) Unregistered Observer: " + observer.toString());
            Log.d(TAG, "(Unregister) Now we have: " + observers.size() + " observers");
        }
        else
        {
            Log.d(TAG, "(Unregister) Registered Observer not found");
        }
    }

    @Override
    public void notifyObserverNewLocalBackup(BackupInfo backupInfo) {

        // Avoid java.util.ConcurrentModificationException 
        // at java.util.ArrayList$ArrayListIterator.next(ArrayList.java)

        for( Iterator< Observer > it = observers.iterator(); it.hasNext() ; )
//      for(Observer observer : observers)
        {
            Observer observer = it.next();
            observer.notifyNewLocalBackup(backupInfo);
        }

    }

    @Override
    public void notifyObserverNewRemoteBackup(ArrayList<PhoneBackup> phoneBackups) {

        // Avoid java.util.ConcurrentModificationException 
        // at java.util.ArrayList$ArrayListIterator.next(ArrayList.java)

//      for(Observer observer : observers)
        for( Iterator< Observer > it = observers.iterator(); it.hasNext() ; )
        {
            Observer observer = it.next();
            observer.notifyNewRemoteBackup(phoneBackups);
        }
    }

    @Override
    public void notifyObserverDownloadCompleted(PhoneBackup phoneBackup) {

        // Avoid java.util.ConcurrentModificationException 
        // at java.util.ArrayList$ArrayListIterator.next(ArrayList.java)

//      for(Observer observer : observers)
        for( Iterator< Observer > it = observers.iterator(); it.hasNext() ; )
        {
            Observer observer = it.next();
            observer.notifyDownloadCompleted(phoneBackup);
        }

    }

    @Override
    public void notifyObserverUploadCompleted(boolean isSucccess) {

        // Avoid java.util.ConcurrentModificationException 
        // at java.util.ArrayList$ArrayListIterator.next(ArrayList.java)

//      for(Observer observer : observers)
        for( Iterator< Observer > it = observers.iterator(); it.hasNext() ; )
        {
            Observer observer = it.next();
            observer.notifyUploadCompleteted(isSucccess);
        }
    }

Now last time I got the excption it happened on notifyObserverNewRemoteBackup method at line Observer observer = it.next();

06-12 04:31:58.394: W/dalvikvm(31358): threadid=1: thread exiting with uncaught exception (group=0x418fcce0)
06-12 04:31:58.629: E/AndroidRuntime(31358): FATAL EXCEPTION: main
06-12 04:31:58.629: E/AndroidRuntime(31358): Process: com.mypackage.android.design.appdesgin, PID: 31358
06-12 04:31:58.629: E/AndroidRuntime(31358): java.util.ConcurrentModificationException
06-12 04:31:58.629: E/AndroidRuntime(31358):    at java.util.ArrayList$ArrayListIterator.next(ArrayList.java:573)
06-12 04:31:58.629: E/AndroidRuntime(31358):    at com.mypackage.android.design.appdesgin.asynctasks.ObserverSubjectManager.notifyObserverNewRemoteBackup(ObserverSubjectManager.java:99)
06-12 04:31:58.629: E/AndroidRuntime(31358):    at com.mypackage.android.design.appdesgin.asynctasks.BackupsHandler$1.success(BackupsHandler.java:318)
06-12 04:31:58.629: E/AndroidRuntime(31358):    at com.mypackage.android.design.appdesgin.asynctasks.BackupsHandler$1.success(BackupsHandler.java:1)
06-12 04:31:58.629: E/AndroidRuntime(31358):    at retrofit.CallbackRunnable$1.run(CallbackRunnable.java:45)
06-12 04:31:58.629: E/AndroidRuntime(31358):    at android.os.Handler.handleCallback(Handler.java:733)
06-12 04:31:58.629: E/AndroidRuntime(31358):    at android.os.Handler.dispatchMessage(Handler.java:95)
06-12 04:31:58.629: E/AndroidRuntime(31358):    at android.os.Looper.loop(Looper.java:136)
06-12 04:31:58.629: E/AndroidRuntime(31358):    at android.app.ActivityThread.main(ActivityThread.java:5081)
06-12 04:31:58.629: E/AndroidRuntime(31358):    at java.lang.reflect.Method.invokeNative(Native Method)
06-12 04:31:58.629: E/AndroidRuntime(31358):    at java.lang.reflect.Method.invoke(Method.java:515)
06-12 04:31:58.629: E/AndroidRuntime(31358):    at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:791)
06-12 04:31:58.629: E/AndroidRuntime(31358):    at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:607)
06-12 04:31:58.629: E/AndroidRuntime(31358):    at dalvik.system.NativeStart.main(Native Method)

---------------------- EDIT -------------------------------

I've followed Anubian Noob suggestion and I implemented a synchronized list but I'm still getting the exception.

Here's my updated code:

// Singleton
    public synchronized static ObserverSubjectManager getInstance()
    {
        if(instance == null)
        {
            instance = new ObserverSubjectManager();

            return instance;
        }
    return instance;
}


private ObserverSubjectManager()
{
//      observers = new ArrayList<>();  



    observers = Collections.synchronizedList(new ArrayList<Observer>());
}


@Override
public void register(final Observer newObserver) {
    Log.d(TAG, "(Register) Observer registred: " + newObserver.toString());

    synchronized (observers) {
        observers.add(newObserver);
    }


    Log.d(TAG, "(Register) Number of registered observers: " + observers.size());

}

@Override
public void unregister(final Observer observer) {

    synchronized (observers) 
    {
        int indexObersver = observers.indexOf(observer);

        if(indexObersver >= 0)
        {
            observers.remove(indexObersver);
            Log.d(TAG, "(Unregister) Unregistered Observer: " + observer.toString());
            Log.d(TAG, "(Unregister) Now we have: " + observers.size() + " observers");
        }
        else
        {
            Log.d(TAG, "(Unregister) Registered Observer not found");
        }
    }


}

@Override
public void notifyObserverNewLocalBackup(final BackupInfo backupInfo) {

    synchronized (observers) 
    {
        for(Observer observer : observers)
        {
            observer.notifyNewLocalBackup(backupInfo);
        }
    }


}

@Override
public void notifyObserverNewRemoteBackup(final ArrayList<PhoneBackup> phoneBackups) {

    synchronized (observers) 
    {
        for(Observer observer : observers)
        {
            observer.notifyNewRemoteBackup(phoneBackups);
        }
    }
}

@Override
public void notifyObserverDownloadCompleted(final PhoneBackup phoneBackup) {

    synchronized (observers) 
    {
        for(Observer observer : observers)
        {
            observer.notifyDownloadCompleted(phoneBackup);
        }
    }
}

@Override
public void notifyObserverUploadCompleted(final boolean isSucccess) {

    synchronized (observers) 
    {
        for(Observer observer : observers)
        {
            observer.notifyUploadCompleteted(isSucccess);
        }
    }
}

Stacktrace:

06-12 05:12:49.359: W/dalvikvm(31735): threadid=1: thread exiting with uncaught exception (group=0x418fcce0)
06-12 05:12:49.426: E/AndroidRuntime(31735): FATAL EXCEPTION: main
06-12 05:12:49.426: E/AndroidRuntime(31735): Process: com.mypackage.android.design.appdesgin, PID: 31735
06-12 05:12:49.426: E/AndroidRuntime(31735): java.util.ConcurrentModificationException
06-12 05:12:49.426: E/AndroidRuntime(31735):    at java.util.ArrayList$ArrayListIterator.next(ArrayList.java:573)
06-12 05:12:49.426: E/AndroidRuntime(31735):    at com.mypackage.android.design.appdesgin.asynctasks.ObserverSubjectManager.notifyObserverDownloadCompleted(ObserverSubjectManager.java:126)
06-12 05:12:49.426: E/AndroidRuntime(31735):    at com.mypackage.android.design.appdesgin.asynctasks.BackupsHandler$2.success(BackupsHandler.java:336)
06-12 05:12:49.426: E/AndroidRuntime(31735):    at com.mypackage.android.design.appdesgin.asynctasks.BackupsHandler$2.success(BackupsHandler.java:1)
06-12 05:12:49.426: E/AndroidRuntime(31735):    at retrofit.CallbackRunnable$1.run(CallbackRunnable.java:45)
06-12 05:12:49.426: E/AndroidRuntime(31735):    at android.os.Handler.handleCallback(Handler.java:733)
06-12 05:12:49.426: E/AndroidRuntime(31735):    at android.os.Handler.dispatchMessage(Handler.java:95)
06-12 05:12:49.426: E/AndroidRuntime(31735):    at android.os.Looper.loop(Looper.java:136)
06-12 05:12:49.426: E/AndroidRuntime(31735):    at android.app.ActivityThread.main(ActivityThread.java:5081)
06-12 05:12:49.426: E/AndroidRuntime(31735):    at java.lang.reflect.Method.invokeNative(Native Method)
06-12 05:12:49.426: E/AndroidRuntime(31735):    at java.lang.reflect.Method.invoke(Method.java:515)
06-12 05:12:49.426: E/AndroidRuntime(31735):    at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:791)
06-12 05:12:49.426: E/AndroidRuntime(31735):    at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:607)
06-12 05:12:49.426: E/AndroidRuntime(31735):    at dalvik.system.NativeStart.main(Native Method)

Answer 1

To follow up @Rogue's comment, I would look for any instances where any of your notify (notifyDownloadCompleted(), etc.) callback implementations unregister an observer. What can easily happen is that:

1) You're iterating over a collection. While in that iteration, you call a method on one of the registered observers.

2) That registered observer, in the notify callback, calls through to unregister itself from further notifications.

3) Since you're still in that iteration loop, this will cause a ConcurrentModificationException as you cannot modify a collection while iterating over it.

You could fix this by doing a reverse loop:

for (int i = collection.size() - 1; i >= 0; i--) {
    collection.get(i).notifyDownloadCompleted();
}

Although you could technically still run into some edge cases there, but not an exception.

Answer 2

The problem is that you're accessing your ArrayList from another thread, which means when you modify it you get that exception. An easy fix is to replace your ArrayList with a CopyOnWriteArrayList (which is a lot slower), or to use Collections.synchronizedList().

To make a synchronized list:

List<Observer> list = Collection.synchronizedList(new ArrayList<Observer>);

Answer 3

If you are not accessing the collection from multiple threads, but only want to avoid problems when changing the collection while iterating over it, probably the easiest way is to iterate over a copy of your collection instead:

for (Observer observer : new ArrayList<>(observers)) {
  observer.notifyNewLocalBackup(backupInfo);
}

This implies a certain overhead for creating the copy, of course.

You can also use a CopyOnWriteArrayList, which covers the case of access from concurrent threads, too.