How to correctly use synchronized?

Question

This piece of code:

synchronized (mList) {
    if (mList.size() != 0) {
        int s = mList.size() - 1;
        for (int i = s; i > 0; i -= OFFSET) {
            mList.get(i).doDraw(canv);
        }
        getHead().drawHead(canv);
    }
}

Randomly throws AIOOBEs. From what I've read, the synchronized should prevent that, so what am I doing wrong?

Edits:

AIOOBE = Array Index Out Of Bounds Exception The code's incomplete, cut down to what is needed. But to make you happy, OFFSET is 4, and just imagine that there is a for-loop adding a bit of data at the beginning. And a second thread reading and / or modifying the list.

Edit 2:

I've noticed it happens when the list is being drawn and the current game ends. The draw-thread hasn't drawn all elements when the list is emptied. Is there a way of telling the game to wait with emtying the list untill it's empty?

Edit 3:

I've just noticed that I'm not sure if this is a multi-threading problem. Seems I only have 2 threads, one for calculating and drawing and one for user input.. Gonna have to look into this a bit more than I thought.

Answer 1

What you're doing looks right... but that's all:

1. It doesn't matter on what object you synchronize, it needn't be the list itself.
2. What does matter is if all threads always synchronize on the same object, when accessing a shared resource.
3. Any access to SWING (or another graphic library) must happen in the AWT-Thread.

To your edit:

I've noticed it happens when the list is being drawn and the current game ends. The draw-thread hasn't drawn all elements when the list is emptied. Is there a way of telling the game to wait with emtying the list untill it's empty?

I think you mean "...wait with emptying the list until the drawing has completed." Just synchronize the code doing it on the same lock (i.e., the list itself in your case).

Again: Any access to a shared resource must be protected somehow. It seems like you're using synchronized just here and not where you're emptying the list.

Answer 2

The safe solution is to only allow one thread to create objects, add and remove them from a List after the game has started.

I had problems myself with random AIOOBEs erros and no synchornize could solve it properly plus it was slowing down the response of the user.

My solution, which is now stable and fast (never had an AIOOBEs since) is to make UI thread inform the game thread to create or manipulate an object by setting a flag and coordinates of the touch into the persistent variables.

Since the game thread loops about 60 times per second this proved to be sufficent to pick up the message from the UI thread and do something.

This is a very simple solution and it works great!

Answer 3

My suggestion is to use a BlockingQueue and I think you are looking for this solution also. How you can do it? It is already shown with an example in the javadoc :)

 class Producer implements Runnable {
   private final BlockingQueue queue;
   Producer(BlockingQueue q) { queue = q; }
   public void run() {
     try {
       while (true) { queue.put(produce()); }
     } catch (InterruptedException ex) { ... handle ...}
   }
   Object produce() { ... }
 }

 class Consumer implements Runnable {
   private final BlockingQueue queue;
   Consumer(BlockingQueue q) { queue = q; }
   public void run() {
     try {
       while (true) { consume(queue.take()); }
     } catch (InterruptedException ex) { ... handle ...}
   }
   void consume(Object x) { ... }
 }

 class Setup {
   void main() {
     BlockingQueue q = new SomeQueueImplementation();
     Producer p = new Producer(q);
     Consumer c1 = new Consumer(q);
     Consumer c2 = new Consumer(q);
     new Thread(p).start();
     new Thread(c1).start();
     new Thread(c2).start();
   }
 }

The beneficial things for you are, you need not to worry about synchronizing your mList. BlockingQueue offers 10 special method. You can check it in the doc. Few from javadoc:

BlockingQueue methods come in four forms, with different ways of handling operations that cannot be satisfied immediately, but may be satisfied at some point in the future: one throws an exception, the second returns a special value (either null or false, depending on the operation), the third blocks the current thread indefinitely until the operation can succeed, and the fourth blocks for only a given maximum time limit before giving up.

To be in safe side: I am not experienced with android. So not certain whether all java packages are allowed in android. But at least it should be :-S, I wish.

Answer 4

You are getting Index out of Bounds Exception because there are 2 threads that operate on the list and are doing it wrongly. You should have been synchronizing at another level, in such a way that no other thread can iterate through the list while other thread is modifying it! Only on thread at a time should 'work on' the list.

I guess you have the following situation:

//piece of code that adds some item in the list

synchronized(mList){
    mList.add(1, drawableElem);
    ...
}

and

//code that iterates you list(your code simplified)

synchronized (mList) {
    if (mList.size() != 0) {
        int s = mList.size() - 1;
        for (int i = s; i > 0; i -= OFFSET) {
            mList.get(i).doDraw(canv);
        }
        getHead().drawHead(canv);
    }
}

Individually the pieces of code look fine. They seam thread-safe. But 2 individual thread-safe pieces of code might not be thread safe at a higher level! It's just you would have done the following:

Vector v = new Vector();
if(v.length() == 0){    v.length() itself is thread safe!
  v.add("elem");        v.add() itself is also thread safe individually!
}

BUT the compound operation is NOT!

Regards, Tiberiu