0

I've been having some thread related problems recently with a consumer that takes points. Here is the original, which works fine except for taking up a lot of cpu constantly checking the queue. The idea is that cuePoint can be called casually and the main thread keeps going.

import java.util.List;
import java.util.ArrayList;
import java.util.concurrent.ArrayBlockingQueue;
import java.util.concurrent.BlockingQueue;


public class PointConsumer implements Runnable {
    public static final int MAX_QUEUE_SIZE=500;

    BlockingQueue<Point> queue;

    public PointConsumer (){
        this.queue=new ArrayBlockingQueue<Point>(MAX_QUEUE_SIZE);
    }

     public void cuePoint(Point p){
        try{
            this.queue.add(p);
        }
        catch(java.lang.IllegalStateException i){}
    }
     public void doFirstPoint(){
        if(queue.size()!=0){
            Point p=queue.poll();
            //operation with p that will take a while
        }
    }

    public void run() {
        while(true){
                  doFirstPoint();
        }
    }

}

I tried to fix the cpu issue by adding notify() every time the cue function is called, and re-working doFirstPoint() to something like this:

public void doFirstPoint(){

    if(queue.size()!=0){
            //operation with p that will take a while
    }
    else{
        try{
            wait();
        }
        catch(InterruptedException ie){}
    }
}

However, I found out that notify() and wait() only work in synchronized functions. When I make doFirstPoint and cuePoint synchronized, the main thread which has called cuePoint will be kept waiting.

I had a few ideas to get around this, including making the thread an object and having it be directly notified, but I wasn't sure if that would cause more problems than it would fix, be very bad form, or simply not work. Is there a simple solution to this problem that I'm missing?

AnimeFan666
  • 15
  • 1
  • 4

2 Answers2

11

The point of BlockingQueue is that you don't have to write this code yourself.

Just call take() instead, which will wait until an object is inserted onto the queue, or use poll but with a timeout, so that it only returns null if the timeout elapses.

EDIT: Just to clarify the answer - as it's in the comments - not only does this mean you can remove the wait/notify code; you also remove the size check as the queue does that for you.

Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • 3
    40 seconds too late :/ Yeah, call take() instead of poll() and get rid of the wait notify. Also, get rid of the queue.size() != 0 before the poll call that you currently have. Take() will block until the queue size is > 0. – Matt Wonlaw Mar 09 '11 at 21:09
  • Oh wow - now I feel really dumb. I have to start reading the documentation before messing around with new classes. This worked perfectly. Will accept as soon as it lets me. – AnimeFan666 Mar 09 '11 at 21:13
3

Just to add on what's been said, it's important to note the difference between put() and add(). If your queue is full, points you try and insert may never actually be inserted in the queue, because an IllegalStateException will be thrown, while put() will wait, if necessary, to insert the point.

Documentation for add() states

Adds the specified element to this queue if it is possible to do so immediately, returning true upon success, else throwing an IllegalStateException.

while put states

Adds the specified element to this queue, waiting if necessary for space to become available

emilio
  • 314
  • 3
  • 16