Multithread ConcurrentModificationException

Question

I have searched the web for a while now trying to resolve this issue, but have had no success.

In my application, I have a large set of messages that I am attempting to encrypt using a basic commutative encryption scheme. Since the sets are large numbers of BigIntegers, I am attempting to multithread the encryptions to increase performance.

Basically, I take the large set of messages and split it up into subsets that are passed to an encryption thread to do a subset of the encryptions. Then I attempt to extract each subset and aggregate them into the original large set after the threads have all done their parts. When I iterate over the threads and pull out each of their encryptions, the error is occurring when I attempt to actually addAll of the encryptions to the list of all encryptions and the error it throws is the java.util.ConcurrentModificationException error.

I have attempted to use synchronization, but it isn't helping.

Here is the function call:

protected Set<BigInteger> multiEncrypt(BigInteger key, HashSet<BigInteger> messageSet) {
    ArrayList<BigInteger> messages = new ArrayList<BigInteger>(messageSet);
    Set<BigInteger> encryptions = Collections.synchronizedSet(new HashSet<BigInteger>());
    int cores = Runtime.getRuntime().availableProcessors();
    int numMessages = messages.size();
    int stride = numMessages/cores;

    //create all the threads and run them
    ArrayList<EncryptThread> threads = new ArrayList<EncryptThread>();
    for (int thread = 0; thread < cores; thread++) {
        int start = thread*stride;
        //don't want to go over the end
        int stop = ((thread+1)*stride >= messages.size()) ? messages.size()-1 : (thread+1)*stride;
        List<BigInteger> subList = messages.subList(start, stop);
        EncryptThread t = new EncryptThread(encryptionScheme.getPrime(), key, subList);
        t.start();
        threads.add(t);
    }
    //pull out the encryptions
    synchronized(encryptions){
        for (int i=0; i < threads.size()-1; i++) {
            EncryptThread thread = threads.get(i);
            ArrayList<BigInteger> these = thread.getEncryptions();
            encryptions.addAll(these); //<-- Erroring Here
            thread.finish();
        }
    }

And here are the relevant parts of the EncryptThread class I wrote to do the encryptions:

/**
 * Constructor
 */
public EncryptThread(BigInteger prime, BigInteger key, List<BigInteger> messages) {
    //need a new encryption scheme object for each thread
    encryptionScheme = new EncryptionScheme(prime);
    encryptions = new ArrayList<BigInteger>();
    this.key = key;
    this.messages = messages;
    wait = true;
}

@Override
public void run() {
    encryptMessages(key, messages);
    while(wait);
}

/**
 * Used to encrypt a set of messages
 * @param key
 * @param messages
 * @return
 */
public void encryptMessages(BigInteger key, List<BigInteger> messages) {
    System.out.println("Encrypting stuff");
    for (BigInteger m : messages) {
        BigInteger em = encryptionScheme.encrypt(key, m);
        encryptions.add(m);
    }
}

public ArrayList<BigInteger> getEncryptions() {
    return encryptions;
}

    //call this after encryptions have been pulled to let the thread finish
public void finish() {
    wait = false;
}

}

I am not new to Java, but I am new to multi threading in java and so I would appreciate any and all advice. Thanks in advance!

EDIT: As per the suggestions, I added a simple locking mechanism to the EncryptThread class, which makes the thread wait to return the encryptions until they are all done and it works now.

public void encryptMessages(BigInteger key, List<BigInteger> messages) {
    System.out.println("Encrypting stuff");
    this.lock = true;
    for (BigInteger m : messages) {
        BigInteger em = encryptionScheme.encrypt(key, m);
        //deals with when we have to mark chaff at S2
        if (shift) {
            em.shiftLeft(1);
            if(shiftVal != 0) em.add(BigInteger.ONE);
        }
        encryptions.add(m);
    }
    this.lock = false;
}

public ArrayList<BigInteger> getEncryptions() {
    while(lock);
    return encryptions;
}

EDIT #2 So I ended up using a solution which was suggested to me by someone from my lab. I got rid of the lock and wait booleans, and the finish() function in the EncryptThread class, and instead added a simple thread.join() loop between the start and getEncryption loops:

    //create all the threads
    ArrayList<EncryptThread> threads = new ArrayList<EncryptThread>();
    for (int thread = 0; thread < cores; thread++) {
        int start = thread*stride;
        //don't want to go over the end
        int stop = ((thread+1)*stride >= messages.size()) ? messages.size()-1 : (thread+1)*stride;
        List<BigInteger> subList = messages.subList(start, stop);
        EncryptThread t = new EncryptThread(encryptionScheme.getPrime(), key, subList, shiftVal);
        t.start();
        threads.add(t);

    }
    //wait for them to finish
    for( EncryptThread thread: threads) {
        try {
            thread.join();
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
    }
    //pull out the encryptions
    for (int i=0; i < threads.size()-1; i++) {
        EncryptThread thread = threads.get(i);
        encryptions.addAll(thread.getEncryptions());
    }

I think my main confusion was that I thought a thread class couldn't have its methods called on it after it had finished running. But the above works fine.

Answer 1

If you look at the documentation on List's addAll, it says the following:

Appends all of the elements in the specified collection to the end of this list, in the order that they are returned by the specified collection's iterator (optional operation). The behavior of this operation is undefined if the specified collection is modified while the operation is in progress. (Note that this will occur if the specified collection is this list, and it's nonempty.)

You can see your List being modified while addAll is using it's iterator in your encryptMessages method that one of your threads you spawned is currently executing.

for (BigInteger m : messages) {
    BigInteger em = encryptionScheme.encrypt(key, m);
    encryptions.add(m); // <-- here
}

I didn't look through all of your code fully, but some of the stuff here is not thread safe. You might do well using a CopyOnWriteArrayList instead of a regular ArrayList to avoid the ConcurrentModificationException, that's if, you are okay with not having everything added to the list in the addAll call, if you aren't, you also then need to be waiting for the threads to finish. You probably want to instead just use tasks with an ExecutorService. There's other improvements to make as well probably.

In additional, the goto book everyone mentions to learn how to write thread safe programs in Java is Concurrency in Practice, I'd recommend that if you are new to concurrency in Java.

Answer 2

ConcurrentModificationException happens when you modify a Collection while you're iterating over it. It has very little to do with multi threading, since you can easily create a single threaded example:

ArrayList<String> myStrings = new ArrayList<>();
myStrings.add("foo");
myStrings.add("bar");
for(String s : myStrings) {
   myStrings.add("Hello ConcurrentModificationException!");

Answer 3

you are starting your threads here.

for (int thread = 0; thread < cores; thread++) {
        int start = thread*stride;
        //don't want to go over the end
        int stop = ((thread+1)*stride >= messages.size()) ? messages.size()-1 : (thread+1)*stride;
        List<BigInteger> subList = messages.subList(start, stop);
        EncryptThread t = new EncryptThread(encryptionScheme.getPrime(), key, subList);
        t.start();
        threads.add(t);
    }

Well. Then you have to wait for all threads to get complete , before start aggregate in this block.

//pull out the encryptions
synchronized(encryptions){
    for (int i=0; i < threads.size()-1; i++) {
        EncryptThread thread = threads.get(i);
        ArrayList<BigInteger> these = thread.getEncryptions();
        encryptions.addAll(these); //<-- Erroring Here
        thread.finish();
    }
}

you are blocking threads which are accessing encryptions only. but the thread you have created is not accessing the set . mean time it will keep on add to its own array List these . So when you call encryptions.addAll(these); these is accessed by two threads ( thread owning encryptions and the thread owning these

And the other answers provided detail about why Concurrent exception in addAll.

You have to wait until all the threads get complete thier work.

You can do this using ExecutorService

Change your starting thread as

ExecutorService es = Executors.newFixedThreadPool(cores);
for(int i=0;i<5;i++)
    es.execute(new Runnable() { /*  your task */ }); //EncryptThread instance
es.shutdown();
boolean finshed = es.awaitTermination(1, TimeUnit.MINUTES);

Then process your adding back process.

ExecutorService es = Executors.newFixedThreadPool(cores);
for (int thread = 0; thread < cores; thread++) {
        int start = thread*stride;
        //don't want to go over the end
        int stop = ((thread+1)*stride >= messages.size()) ? messages.size()-1 : (thread+1)*stride;
        List<BigInteger> subList = messages.subList(start, stop);
        EncryptThread t = new EncryptThread(encryptionScheme.getPrime(), key, subList);
        es.execute(t);
        threads.add(t);
    }
es.shutdown();
boolean finshed = es.awaitTermination(1, TimeUnit.MINUTES);



//pull out the encryptions
    synchronized(encryptions){
        for (int i=0; i < threads.size()-1; i++) {
            EncryptThread thread = threads.get(i);
            ArrayList<BigInteger> these = thread.getEncryptions();
            encryptions.addAll(these); //<-- Erroring Here
            thread.finish();
        }
    }

Assumed, your EncryptThread is Thread right now. you might need to change to implements Runnable. and no other change in getEncryptions