CODEWITHSUNDEEP
javaandroidmultithreading
TerryProbert
TerryProbert

Reputation: 1134

Synchronizing threads to avoid ArrayIndexOutOfBoundsException

I have two seperate threads:

My logic thread calls ArrayList.remove(), so I would think when the drawing thread comes to draw there could be a chance of crashing because the index no longer exists.

code sample:

drawingThread extends Thread {
    logicThread = new LogicThread;
    logicThread.start();

    public void run(){
        while(running) {
            for(int i=0; i<npc.size(); i++){
                npc.get(i).callDraw();
            }
        }
        // stop logicThread when out of gameloop
        logicThread.running = false;
}}

LogicThread extends Thread {
    public void run(){
        while(running){
            for(int i=0; i<npc.size();i++){
                if(npc.get(i).isDead()){
                    npc.remove(i);
                }
                npc.trimToSize();
            }
            Collection.sort(npc);
        }
}}

Which would be the correct way to prevent an exception, syncronized or trycatch? Also are there any benefits from using one over the other?

synchronized(logicThread) { 
    for(int i=0; npc.size(); i++) {
        npc.callDraw(); 
}}

or

try {
    npc.callDraw();
} catch(Exception e) {}

Upvotes: 1

Views: 763

Answers (5)

Spyder
Spyder

Reputation: 1902

Synchronizing over the entire draw / logic blocks will negate the benefits of threading, and catching the exception could lead to inconsistent UI (not to mention harder to manage code).

When iterating over a collection that another thread might modify, copy it first!

List drawList = new ArrayList(npc);
for(int i=0; i<drawList.size(); i++){
    drawList.get(i).callDraw();
}

You probably still need to synchronize the copy operation; the risks are far lower but there's still a race condition that will lead to nulls in your copy. Collections.synchronizedList() can turn a regular list into a synchronized list but at the expense of some speed on all operations.

If performance with synchronizedList() becomes a problem you can just manually synchronize the copy and remove operations.

Upvotes: 1

Stephen O&#39;Connor
Stephen O&#39;Connor

Reputation: 1475

Do you need threads for this? If you call the drawing loop followed by the logic loop (serially) do you get an adequate frame rate? Assuming you have a double buffered display. Threads should usually be used where there is something asynchronous going on (like waiting for a server to respond) which you can't control. In this case you have control of when and in which order things happen.

Upvotes: 1

Dean Povey
Dean Povey

Reputation: 9446

You should absolutely synchronize access, but you need to do it in both threads, and you need to synchronize on the shared objects, e.g.

synchronized(npc) {
   // Do something that accesses or modifies npc
}

Looking at your specific example, I would suggest that you probably don't want do it this way since you will need to hold a lock around the for loop. You probably need to move the synchronization into the shared npc object.

Is there any reason you can't just note in the first thread which npcs have died, and then remove them from your list once you exit the for loop. It's much better to avoid a separate thread and the synchronization if you can.

Upvotes: 1

SirVaulterScoff
SirVaulterScoff

Reputation: 704

Just for note: use iterators to remove items from collection:

while (it.hasNex()) {
  if(...) {
    it.remove();

Catching IndexOutOfBound exception is valid if it's acceptable. Another way - is to create array copy in drawing thread, which will guarantee that you don't get IndexOutOfBound. You can just add a check isDead() inside drawing loop

Upvotes: 3

Thirler
Thirler

Reputation: 20760

You should not catch an ArrayIndexOutOfBoundsException, as this is an unchecked exception, this should be used for programming errors and only be thrown when one has occurred (note that this is a little bit controversial, but the above is what Effective Java, Bloch tells us).

If the action you are taking takes little time, use synchronization. If the loop takes a lot of time, use synchronization to copy the list and then iterate over the copy instead of the original.

Another problem you might run into (depending on the type of list you are using), is the ConcurrentModificationException, which occurs when you remove an element from a list that is being iterated.

Also note that if you do not synchronize shared objects when using it in the two threads, that you can have weird memory effects (such as seeing incomplete objects). Java Concurrency in practice by Goetz is a great book that teaches more on this widely misunderstood concept.

An alternative solution to using a synchronized block is using a CopyOnWriteArrayList, which will prevent the ConcurrentModificationException and the memory effects. Note that in order to use the 'copy effect' you need to use the iterators to the remove the elements.

Upvotes: 1

Related Questions