LinkedBlockingQueue.take() seems to be busy all the time

Question

I'm trying to implement a service in OSGi which should wait for incoming data from another bundle and process the data when it receives it. I'm using a LinkedBlockingQueue, because I don't know how many packets I will receive. My code looks like this:

public class MyClass {

protected static LinkedBlockingQueue<JSONObject> inputQueue = new LinkedBlockingQueue<JSONObject>();
private ExecutorService workerpool = Executors.newFixedThreadPool(4);

public void startMyBundle() {
    start();
}

protected void start() {
    new Thread(new Runnable() {
        public void run() {
            while(true){
                workerpool.execute(new Runnable() {
                    public void run() {
                        try {
                            process(inputQueue.take());
                        } catch (InterruptedException e) {
                            System.out.println("thread was interrupted.");
                        }
                    }
                });
            }
        }
    }).start();
}

public void transmitIn(JSONObject packet) {
    try {
        inputQueue.put(packet);
    } catch (InterruptedException e) {

    }
}

protected  void process(JSONObject packet) {
    //Some processing
}

When I'm running this, and only send one packet to the service, the packet is first processed as it should, but then my processor uses all its capacity and most of the time I get an OutOfMemoryError looking like this:

java.lang.OutOfMemoryError thrown from the UncaughtExceptionHandler in thread "[Timer] - Periodical Task (Bundle 46) (Bundle 46)"

What can be the cause of this?

Gray · Accepted Answer · 2013-07-30T15:24:04.860

You are getting an out of memory exception because of these lines of code:

while(true){
   workerpool.execute(new Runnable() {
   ...

This spins forever creating new Runnable instances and adding them into the thread-pool's task queue. These go into an unbounded queue and quickly fill up the memory.

I think you want a 4 threads that are calling inputQueue.take() in a while (true) loop.

for (int i = 0; i < 4; i++) {
    workerpool.execute(new Runnable() {
        public void run() {
            while (!Thread.currentThread().isInterrupted()) {
                process(inputQueue.take());
            }
        }
    });
}
// remember to shut the queue down after you've submitted the last job
workerpool.shutdown();

Also, you don't need a Thread to submit the tasks into the thread-pool. That is an non-blocking operation so can be done by the caller directly.

score 0 · Answer 2 · answered Jul 30 '13 at 14:18

This bit of code is the culprit:

protected void start() {
    new Thread(new Runnable() {
        public void run() {
            while(true){
                workerpool.execute(new Runnable() {
                    public void run() {
                        try {
                            process(inputQueue.take());
                        } catch (InterruptedException e) {
                            System.out.println("thread was interrupted.");
                        }
                    }
                });
            }
        }
    }).start();
}

What it does is create a background task the adds an infinite number of Runnable to the ExecutorService work queue. This eventually causes an OOME.

What you meant to do, I suppose, was:

protected void start() {
    for (int i = 0; i < 4; ++i) {
        workerpool.execute(new Runnable() {
            public void run() {
                while (true) {
                    try {
                        process(inputQueue.take());
                    } catch (InterruptedException e) {
                        //killed, exit.
                        return;
                    }
                }
            }
        });
    }
}

I.e. run 4 workers on the ExecutorService that wait for input.

Peter Kriens · Answer 3 · 2013-07-31T07:14:12.460

Ok, bit pedantic but since this is an OSGi tagged question ...

Cleanup — You're creating a thread and executor service but never clean this up. In general, you want an activate/deactivate pair of methods and not leave any remains after deactivate. From a cohesion point of view you like to see this in one object and not require a central point to manage this. Declarative Services is ideal for this pattern.
Sharing — In general you want to share an Executor with others, it is best to get an Executor from the service registry. This will allow the deployer to tune the number of threads based on the usage of all bundles in the system.

One more thing, Boris gave a correct solution but it is not very efficient since it always occupies 4 threads and an unbounded LinkedQueue. Worse, the code walks like a service, it talks like a service, but does not seem to be used as a service. I think we can do better since the queue + executor is a bit double up and in OSGi this should be a service.

@Component
public class JSONPackageProcessor implement TransmitIn {
  Executor executor;

  @Reference void setExecutor(Executor e) { this.executor = e; }

  public void transmitIn( final JSONPacket packet ) {
    executor.execute(new Runnable() {
       public void run() { 
         try { process(packet); } 
         catch(Throwable t) { log(packet, t); }
       }
    }
  }

  void process( JSONPacket packet ) { ... }
}

This needs no cleanup assuming process(...) always ends 'soon'. In this model the flow is not throttled as you did with the (arbitrary?) 4 worker threads in the pool. The Executor's internal queue is used to buffer. You can limit this as follows:

  Semaphore throttle= new Semaphore(4)

  public void transmitIn( final JSONPacket packet ) throws Exception {
    throttle.acquire();
    executor.execute(new Runnable() {
       public void run() { 
         try { process(packet); } 
         catch(Throwable t) { log(packet, t); }
         finally { throttle.release(); }
    }
  }

You could even configure this through Configuration Admin quite easily:

 @Activate void configure( Map<String,Object> map) throws Exception {
   if ( map.containsKey("throttle") )
     throttle = new Semaphore( map.get("throttle"));
 }

The beauty of this code is that most error cases are logged, concurrency before/after relations are correct because the guarantees you get in OSGi. This code can actually work as is (no guarantees for some typos, have not actually ran it).

LinkedBlockingQueue.take() seems to be busy all the time

3 Answers3