Correct way to access a queue (LinkedBlockingQueue) from ExecutorService

Question

I have just realised LinkedBlockingQueue is a good solution in my case - I have a queue which is filled from time to time (but in very short time intervals). I want my ExecutorService to check as often as posiible for any objects appearing in this queue.

I am not sure what is the correct use of LinkedBlockingQueue now. Earlier, my code looked like:

public void launch(){

    ListeningExecutorService pool = 
            MoreExecutors.listeningDecorator(Executors.newFixedThreadPool(threadNumber));

    while(true){
        if(!commandsQueue.isEmpty()){
            final ListenableFuture<String> future = pool.submit(new CommandWorker(commandsQueue.poll()));
            // [...]  
        }           
    }       
}

I am thinking about sth like:

public void launch(){
            ListeningExecutorService pool = 
            MoreExecutors.listeningDecorator(Executors.newFixedThreadPool(threadNumber));

    while(true){
        Command newCommand = commandsQueue.take(); 
        final ListenableFuture<String> future = pool.submit(new CommandWorker(newCommand));
            // [...]  

    }       
}

The aim is, naturally, to make my ListeningExecutorService intercept new object as fast as possible. Is this a good way?

I am rather part-and-partial to the Pub/Sub model, where you have workers subscribe to some Topic, and producers publish into that topic. But, this should work just fine. The JavaDoc doesn't mention any thread-safety issues, but it is part of the `java.util.concurrent`, and given the fact it has `Blocking` in the name I'd suspect it's written to handle those. Plus, it doesn't appear you are using `peek()` and it looks like you are de-queing from a single thread anyhow. Do you have any concerns about the app shutting down mid-processing? — CodeChimp, Feb 20 '14 at 19:27
@CodeChimp This is indeed a threadsafe class. Pub-Sub is useful in a one-to-many producer/consumer environment, or in a many-to-many. In a one-to-one this is really the ideal way to handle it, because the queue will empty out items that cannot accidentally be consumed multiple times. — InfernalRapture, Feb 20 '14 at 19:30
@InfernalRapture OPs description is using a pool, thus I assumed at least a one-to-many. Obviously an in-memory Queue will greatly outperform a JMS topic any day. — CodeChimp, Feb 20 '14 at 19:32
@CodeChimp In a queuing situation like this I'd wager it's probably many-to-one, and while the Command thread might want to process commands in parallel, we may well have a vested interest in preventing any other subscribers from reading the command topic. — InfernalRapture, Feb 20 '14 at 19:44

InfernalRapture · Accepted Answer · 2014-02-20T20:19:33.423

2

You're working too hard.

A blocking queue BLOCKS if there's nothing to read. Therefore if there is no command the command thread will simply pause and wait for the next command to be issued.

Your take command pops the first item off the queue. If there's no first item, the thread is paused. The if statement is unnecessary.

public void launch(){
        ListeningExecutorService pool = 
        MoreExecutors.listeningDecorator(Executors.newFixedThreadPool(threadNumber));

    while(true){
        Command newCommand = commandsQueue.take(); 
        final ListenableFuture<String> future = pool.submit(new CommandWorker(newCommand));
    }       
}

edited Feb 20 '14 at 20:19

answered Feb 20 '14 at 19:23

InfernalRapture

572
7
19

Yes, I meant to paste a code without this `if()` statement, just have edited the post. Thank you! – Marta Karas Feb 20 '14 at 19:27
Then yes, this is the right way to approach this problem. – InfernalRapture Feb 20 '14 at 19:27
I am not 100% sure if we catch up to the same intention =), but in my app this works correctly only if there is `Command newCommand = commandsQueue.take(); ` (not: `.pool()`). Please edit your post if you agree. – Marta Karas Feb 20 '14 at 20:08
Absolutely correct, Poll will return null if the queue is empty, Take is the blocking method. – InfernalRapture Feb 20 '14 at 20:19

score 2 · Answer 2 · answered Feb 20 '14 at 19:31

2

Why do you use the queue at all? Instead of "commandsQueue.put(command)", do "pool.submit(new CommandWorker(command))" directly.

answered Feb 20 '14 at 19:31

Alexei Kaigorodov

13,189
1
21
38

That's a pretty good question, but I can think of a scenario we wouldn't want to. I don't think the ExecutorService garauntees that it behaves in a FIFO manner, does it? – InfernalRapture Feb 20 '14 at 19:39
I have read somewhere that ExecutorService **does not** guarantee a FIFO manner. I need it - that is why I use `LinkedBlockingQueue` in my app. Anyway, thank you for your attention - I am a 1-day-cocnurrence-user, and find such suggestions informative, anyway :) – Marta Karas Feb 20 '14 at 19:52
1

@Marciszka You did not mention in the question that you need some order. Please describe. And since you eventually put tasks in an Executor, your solution does not preserves order either. – Alexei Kaigorodov Feb 21 '14 at 07:25
@Alexei Kaigorodov, big thank you for your willingness to point out me being miastaken. I reconsidered my application logic and removed the queue at all (passing my commands directly to a pool). – Marta Karas Feb 21 '14 at 11:01

Correct way to access a queue (LinkedBlockingQueue) from ExecutorService

2 Answers2